RFR: 8242361: JavaFX Web View crashes with Segmentation Fault, when HTML contains Data-URIs [v2]

[Kevin Rushforth]

On Sun, 13 Dec 2020 16:09:16 GMT, Matthias Bläsing <github.com+2179736+matthiasblaesing at openjdk.org> wrote:

>> The code in WTF::scheduleDispatchFunctionsOnMainThread assumes, that
>> the java class com.sun.webkit.MainThread can be found be the JNI
>> function FindClass. This is only true if the class is loadable by the
>> system class loader.
>> 
>> One such case is when the OpenJFX modules are loaded from a new
>> ModuleLayer. To fix this, the reference to the class needs to be loaded
>> from when a JNI call from Java into native code is active. In that case
>> FindClass uses the classloader associated with that method.
>> 
>> The test code can be executed by running:
>> 
>> cd tests/manual/web/dataurl
>> ../../../../gradlew run
>
> Matthias Bläsing has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8242361: Add missing copyright headers and integrate test into systemTests

The fix looks good to me, although I want Arun to look at it, too.

The test looks pretty good with a few comments that I added.

modules/javafx.web/src/main/native/Source/WTF/wtf/java/MainThreadJava.cpp line 51:

> 49:     // initilization has to be done from a context where the class
> 50:     // com.sun.webkit.MainThread is accessible. When
> 51:     // scheduleDispatchFunctionsOnMainThread is invoced, the system class loader

Minor typo: invoced --> invoked

modules/javafx.web/src/main/native/Source/WTF/wtf/java/MainThreadJava.cpp line 64:

> 62:     // WebPage will be used by FindClass.
> 63:     //
> 64:     // WTF::initializeMainThread has a guard, so that initialization is only run

I guess you meant to say "is only run __once__"?

tests/system/src/test/java/test/com/sun/webkit/MainThreadTest.java line 60:

> 58:         final List<String> cmd = asList(
> 59:             workerJavaCmd,
> 60:             "-cp",appModulePath + "/mymod",

Minor: need space after `,`

tests/system/src/testapp7/java/mymod/myapp7/DataUrlWithModuleLayer.java line 102:

> 100:                     Platform.runLater(() -> {
> 101:                         String title = webview.getEngine().getTitle();
> 102:                         if("Executed".equals(title)) {

Minor: add space after `if`

tests/system/src/test/java/test/com/sun/webkit/MainThreadTest.java line 40:

> 38:  */
> 39: public class MainThreadTest {
> 40:     @Test

I recommend a `timeout = 15000` in case the launched application doesn't exit.

tests/system/src/testapp7/java/mymod/myapp7/DataUrlWithModuleLayerLauncher.java line 63:

> 61:         Class testClass = moduleClassLoader.loadClass("myapp7.DataUrlWithModuleLayer");
> 62:         Method launchMethod = appClass.getMethod("launch", Class.class, String[].class);
> 63:         launchMethod.invoke(null, new Object[]{testClass, args});

You might consider adding a `sleep(15000)` followed by `System.exit(1)` or some other mechanism to prevent running indefinitely so that if this process hangs it doesn't hang the test suite (which has happened in the past).

tests/system/src/testapp7/java/mymod/myapp7/DataUrlWithModuleLayer.java line 99:

> 97:             @Override
> 98:             public void run() {
> 99:                 while (true) {

Rather than a spin loop in a new thread, it's probably better to wait for the content to be loaded (checked using a listener on `WebEngine::getLoadWorker::stateProperty`) before doing the test, which you shouldn't need to do in a loop. In any case you will want a timeout so that if this process hangs it doesn't hang the test suite, which has happened in the past.

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

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