Modify

Opened 3 months ago

Closed 3 months ago

Last modified 3 months 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:

Description

Creating this in parallel with https://github.com/openstreetmap/josm/pull/22 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 Changed 3 months ago by Don-vip

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

comment:2 Changed 3 months ago by Don-vip

In 13299/josm:

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

comment:3 Changed 3 months ago by Don-vip

Milestone: 18.01

comment:4 Changed 3 months ago by Don-vip

Component: PluginCore

comment:5 Changed 3 months ago by Don-vip

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 Changed 3 months ago by Don-vip

thanks! :)

comment:7 Changed 3 months ago by ris

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 .jar.new 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 Changed 3 months ago by Don-vip

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 Changed 3 months ago by ris

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 .jar.new file - it doesn't seem like a good idea given that a .jar.new 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 .jar.new 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 .jar.new files (in particular, ReadLocalPluginInformationTask) - they seem to maybe come from a time when it couldn't be just assumed that any .jar.new 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 .jar.new files should be totally disregarded everywhere but installDownloadedPlugins.

Attempting to make use of .jar.new and .jar files does also lead to some ambiguities when both a foo.jar and foo.jar.new 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 Changed 3 months ago by bastiK

Your analysis sounds correct to me. Under normal circumstances, the .jar.new 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 Changed 3 months ago by ris

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 Changed 3 months ago by ris

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
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.