Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#15742 closed enhancement (fixed)

[PATCH] PluginHandler.installDownloadedPlugins: check downloaded plugin is valid *before* we delete any existing one

Reported by: ris Owned by: team
Priority: normal Milestone: 18.01
Component: Core Version:
Keywords: download corrupt plugin jar delete Cc:


Creating this in parallel with for the lovely pre-merge CI.

Small one here - noticed that the current behaviour of PluginHandler.installDownloadedPlugins could prove to be more than a little annoying - only checking for corruption of the newly downloaded plugin after it's already deleted an existing one. Someone getting repeatedly corrupted downloads or having limited internet connectivity would lose their probably-previously-working plugin.

Also added a couple of tests covering this, requiring some additions to the underlying test infrastructure, including .assumeRevision(...) to JOSMTestRules which should come in handy when testing any functionality around version-checking.

Not sure if you actually want me to bother posting all the .patch files here too seeing as it's just as easy to get them out of git...

Change History (16)

comment:1 by Don-vip, 6 years ago

Yes, please attach the patch. It's easier/faster for me.

comment:2 by Don-vip, 6 years ago

In 13299/josm:

see #15742 - AppVeyor: do not upload javadoc artifact, too big

comment:3 by Don-vip, 6 years ago

Milestone: 18.01

comment:4 by Don-vip, 6 years ago

Component: PluginCore

comment:5 by Don-vip, 6 years ago

Resolution: fixed
Status: newclosed

In 13300/josm:

fix #15742 - check downloaded plugin is valid *before* we delete any existing one (patch by ris)

comment:6 by Don-vip, 6 years ago

thanks! :)

comment:7 by ris, 6 years ago

Something I'm still a little confused about around this code - why is PluginHandler.installDownloadedPlugins(...) invoked in no-warnings mode in most cases, and the single place it is called with warnings enabled (MainApplication.updateAndLoadEarlyPlugins(...)), its use *appears* to be redundant (it is called after a call to a PluginDownloadTask, which itself should have called installDownloadedPlugins. Now maybe that second call *isn't* redundant, because a failure causing the plugin download task to terminate early would have left a number of files around, not-yet installed. *But*, if we care about that in updateAndLoadEarlyPlugins, why don't we care about that in any of the other places we do a PluginDownloadTask?

Perhaps this is just oversight to do with how the code evolved but it would be enlightening to know if this is deliberate since there don't seem to be any comments explaining the thinking here.

comment:8 by Don-vip, 6 years ago

I have no idea. When I don't understand a piece of code I use the blame feature of Trac to look for all past commits.

comment:9 by ris, 6 years ago

Yeah I've been finding that most of the plugin code hasn't really been touched for ~5 years, and the commits are quite coarse. (This is why I tend to submit my changes as a number of smaller patches - more detailed paper trail)

Another thing which confused me was why PluginPreferencesModel.refreshLocalPluginVersion(...) would ever want to refresh from a file - it doesn't seem like a good idea given that a file either hasn't finished downloading, or the installation was not completed for some reason (corrupted download?). Also, the only place it is ever called is after the completed execution of a PluginDownloadTask, which should hav alreadye renamed any files it was able to. The further I investigated into the past here it seems that refreshLocalPluginVersion was added at a time before PluginHandler.installDownloadedPlugins(...) was automatically called at the end of the PluginDownloadTask.

So, to me, this casts into doubt all places where we take heed of files (in particular, ReadLocalPluginInformationTask) - they seem to maybe come from a time when it couldn't be just assumed that any files haven't been renamed to .jar for a reason. But I would propose that now that we should be able to rely on installDownloadedPlugins running at the end of any PluginDownloadTask, surely we can make that assumption (?). And that perhaps files should be totally disregarded everywhere but installDownloadedPlugins.

Attempting to make use of and .jar files does also lead to some ambiguities when both a foo.jar and file exist - e.g. in such a case ReadLocalPluginInformationTask would appear to pull information from either one or the other randomly (scanPluginFiles will overwrite the first one encountered with information from the second, and the order they will be encountered is undefined).

Anyone with more knowledge of the workings of the plugin system, I'd welcome input.

comment:10 by bastiK, 6 years ago

Your analysis sounds correct to me. Under normal circumstances, the file exists only for the time of download, so there should be no need to read metadata from that file in PluginPreferencesModel. In the past, the file probably persisted until the next restart, hence it would have made sense to read and display the information from that file after update.

Personally, I'm in "do not touch a running system"-mode, but there is certainly a lot of potential to improve the plugin code and you seem like a person diligent enough to take it on. :)

comment:11 by ris, 6 years ago

I wasn't quite considering redesigning it - was actually looking at doing an example implementation of application-side plugin security a la #15624, which I actually don't think would be too tricky at all. I just need to straighten out the facts of what I can and cannot rely on. If I can more or less rely on all plugins that get downloaded going through installDownloadedPlugins before any part of josm tries to use them, that simplifies things considerably.

Of course, in the process of figuring out some of the redundancies that lie around here, it may allow me to rip a few bits of unnecessary code out that are needlessly confusing. Or perhaps just document them. Something that leaves it a less confusing corner of code.

comment:12 by ris, 6 years ago

Of course I'm sitting here thinking "oh it'll all be fine I'll just write a bunch of tests for the flows" but I now realize - all the plugin-management ui flows are dependent on JDialogs. And JDialog doesn't like headless mode. Hmm.

This will have to be addressed somehow.

Modify Ticket

Change Properties
Set your email in Preferences
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment

E-mail address and name can be saved in the Preferences .
Note: See TracTickets for help on using tickets.