Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#14901 closed enhancement (fixed)

restrict plugin classpath

Reported by: bastiK Owned by: team
Priority: normal Milestone: 17.06
Component: Core Version:
Keywords: plugins Cc:

Description

Currently, we have one joined classpath for all plugins. This means a plugin can load classes from all other plugins the user has enabled.

It would be cleaner, if access was restricted to JOSM core and all dependencies (required plugins). This is implemented in the attached patch, a separate ClassLoader for each plugin is created: It first looks up a class in JOSM core, then refers to the class loader of dependent plugins and finally in the plugin jar.

One consequence is that you can include different conflicting versions of the same library in different plugins. In general, we should aim to share libraries by creating a library plugin, but in certain situations this can be impractical or impossible.

Attachments (2)

plugin-classloader.patch (6.8 KB) - added by bastiK 6 years ago.
WIP
plugin-classloader.2.patch (6.9 KB) - added by bastiK 6 years ago.

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by bastiK

Attachment: plugin-classloader.patch added

WIP

comment:1 Changed 6 years ago by Don-vip

Have you tested it with Java 9 ? The classpath changes a lot with this version.

comment:2 Changed 6 years ago by bastiK

It appears to work (build 9-ea+166). Haven't they made it backward compatible for the most part?

comment:3 Changed 6 years ago by Don-vip

OK I'll give it a try :)

comment:4 Changed 6 years ago by Don-vip

It doesn't work on my setup, crash at startup:

Revision:12318
Is-Local-Build:true
Build-Date:2017-06-05 22:27:44

Identification: JOSM/1.5 (12318 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1703 (15063)
Memory Usage: 370 MB / 4088 MB (158 MB allocated, but free)
Java version: 9-ea+172, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080, \Display1 1920x1080, \Display2 1280x1024
Maximum Screen Size: 1920x1080
VM arguments: [--add-exports=java.base/sun.security.util=ALL-UNNAMED, --add-exports=java.base/sun.security.x509=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-exports=jdk.deploy/com.sun.deploy.config=ALL-UNNAMED, --add-modules=java.se.ee, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-exports=java.desktop/com.apple.eawt=ALL-UNNAMED, -Dfile.encoding=UTF-8]
Program arguments: [--debug]

Plugins:
+ DxfImport
+ ImportImagePlugin
+ apache-commons
+ apache-http
+ canvec_helper
+ ejml
+ epci-fr (32699)
+ geotools
+ gson
+ imagery-xml-bounds
+ jna
+ jts
+ log4j (32699)
+ o5m
+ opendata
+ pbf
+ public_transport_layer
+ tag2link
+ utilsplugin2

Last errors/warnings:
- E: Handled by bug report queue: java.lang.AssertionError
- W: No configuration settings found.  Using hardcoded default values for all pools.


=== REPORTED CRASH DATA ===
BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: main (1)
java.lang.AssertionError
	at org.openstreetmap.josm.plugins.PluginHandler.loadPlugins(PluginHandler.java:835)
	at org.openstreetmap.josm.plugins.PluginHandler.loadLatePlugins(PluginHandler.java:882)
	at org.openstreetmap.josm.gui.MainApplication.loadLatePlugins(MainApplication.java:435)
	at org.openstreetmap.josm.gui.MainApplication.main(MainApplication.java:362)

Changed 6 years ago by bastiK

Attachment: plugin-classloader.2.patch added

comment:5 in reply to:  4 Changed 6 years ago by bastiK

Replying to Don-vip:

It doesn't work on my setup, crash at startup:

Yes, there was a bug - I've updated the patch.

comment:6 Changed 6 years ago by Don-vip

Milestone: 17.06

OK it works, even with transitive dependencies and Java 9.

Some minor style remarks:

  1. Can we get rid of label statement DEPENDENCIES: ?
  2. Don't forget to document all public APIs ;)

comment:7 in reply to:  6 Changed 6 years ago by bastiK

Replying to Don-vip:

OK it works, even with transitive dependencies

Yes, it should work.

and Java 9.

Not that much different in terms of used API, so if it fails in Java 9, then I would expect it to fail with or without the patch.

Some minor style remarks:

  1. Can we get rid of label statement DEPENDENCIES: ?

Label statements make the code shorter and easier to understand, what's wrong with them?

comment:8 Changed 6 years ago by Don-vip

I found them harder to understand, personal taste :) If it's too complicated, no problem.

comment:9 Changed 6 years ago by bastiK

Resolution: fixed
Status: newclosed

In 12322/josm:

fixed #14901 - restrict plugin classpath

Separate class loader for each plugin instead of unified class loader.

comment:10 Changed 6 years ago by bastiK

In GeoToolsPlugin.java, the now deprecated PluginHandler.getPluginClassLoader() is used. I'm not sure what it does and if it is still working, but this call should probably be replaced by GeoToolsPlugin.class.getClassLoader().

comment:11 Changed 6 years ago by bastiK

In 12323/josm:

see #14901 - some cleanup

comment:12 Changed 6 years ago by stoecker

I believe Integration test fail because of this. The videomapping plugin. But I didn't find the reason why it fails now and did not fail before.

comment:13 Changed 6 years ago by Don-vip

slf4j is required by vlcj, but was provided only through OsmRecPlugin. It proves the system works as expected. As both plugins are rarely used, the best solution is to include slf4j in videomapping plugin.

comment:14 Changed 6 years ago by Don-vip

Done in [o33376:33377].

comment:15 Changed 6 years ago by Don-vip

GeoTools plugin updated as well.

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.