RFR: 8264990: Fix segfault when accessing local storage in webview

[Kevin Rushforth]

On Thu, 8 Apr 2021 06:58:14 GMT, Matthias Bläsing <github.com+2179736+matthiasblaesing at openjdk.org> wrote:

> The functions from FileSystemJava are called from different threads the
> root problem manifests because the JNI FindClass function behaves
> differently when called from a context that is the ancestor of a java
> frame compared to when called in isolation.
> 
> A segmentation fault is observed when local storage of a webview is
> accessed. At that time a new native thread is spun up and that sets up
> the local storage, by calling into the JVM via
> WTF::FileSystem::makeAllDirectories. At that point GetFileSystemClass is
> invoked to get a referenc to the java implementation of the FileSystem.
> As this is is called from a new native thread (no java context
> available), JNI uses the system classloader to locate the class. This
> fails if the JavaFX modules are not on the boot module/class path.
> 
> Instead on relying on fetching the class reference everytime it is
> needed, this change fetches it once when the JavaFX library is loaded
> and stores it in the WTF namespace.
> 
> In addition to this it was observed, that there is no attachment to the
> JVM done when calling into the filesystem. No fault was observed, but
> the JNI specs indicate, that the JNIEnv interface is only valid when
> attached.

The fix looks OK to me, although I'd like @arun-Joseph to comfirm.

The test needs one change in how the resource (the `.html` file) is located, and I added a couple minor comments on the test. I'll test it after that.

modules/javafx.web/src/main/native/Source/WTF/wtf/java/FileSystemJava.cpp line 249:

> 247:     if (isHandleValid(handle)) {
> 248:         AttachThreadAsDaemonToJavaEnv autoAttach;
> 249:     JNIEnv* env = autoAttach.env();

Minor: indentation

modules/javafx.web/src/main/native/Source/WTF/wtf/java/JavaEnv.cpp line 144:

> 142:     // the right one
> 143:     static JGClass fileSystemRef(env->FindClass("com/sun/webkit/FileSystem"));
> 144:     WTF::comSunWebkitFileSystem = fileSystemRef;

Can you `ASSERT` that the returned class is not null?

tests/system/src/testapp7/java/mymod/myapp7/LocalStorageAccessWithModuleLayer.java line 81:

> 79:         webview.getEngine().getLoadWorker().stateProperty().addListener(
> 80:                 new ChangeListener<State>() {
> 81:                     public void changed(ObservableValue ov, State oldState, State newState) {

Suggestion: maybe use a lambda here to make the code more compact? (it's up to you whether to change it)

tests/system/src/testapp7/java/mymod/myapp7/LocalStorageAccessWithModuleLayer.java line 92:

> 90:                     }
> 91:                 });
> 92:         webview.getEngine().load(LocalStorageAccessWithModuleLayer.class.getResource("/LocalStorageAccess.html").toExternalForm());

This fails to find the resource when I run the test via gradle. I recommend putting it in the same package as the class and removing the leading `/` (e.g., see the FXML resources in `testapp6` or `testscriptapp1`).

tests/system/src/testapp7/java/mymod/myapp7/LocalStorageAccessWithModuleLayerLauncher.java line 64:

> 62:         // Hack to get the classes of this programm into a module layer
> 63:         List<Path> modulePaths = new ArrayList<>();
> 64:         for(String workerPath: System.getProperty("module.path").split(File.pathSeparator)) {

Minor: missing space after the `for` and also before the `:`

-------------

PR: https://git.openjdk.java.net/jfx/pull/458