#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...
Attachments (4)
Change History (16)
comment:1 by , 7 years ago
by , 7 years ago
Attachment: | v1-0001-TestUtils-add-setPrivateStaticField-method.patch added |
---|
by , 7 years ago
Attachment: | v1-0002-JOSMTestRules-add-assumeRevision-method-to-allow-.patch added |
---|
by , 7 years ago
Attachment: | v1-0003-PluginHandler.installDownloadedPlugins-check-down.patch added |
---|
by , 7 years ago
Attachment: | v1-0004-PluginDownloadTask-add-tests-covering-behaviour-w.patch added |
---|
comment:3 by , 7 years ago
Milestone: | → 18.01 |
---|
comment:4 by , 7 years ago
Component: | Plugin → Core |
---|
comment:7 by , 7 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 .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 by , 7 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 , 7 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 .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 by , 7 years ago
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 by , 7 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 , 7 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 JDialog
s. And JDialog
doesn't like headless mode. Hmm.
This will have to be addressed somehow.
Yes, please attach the patch. It's easier/faster for me.