On Sun, 13 Dec 2020 16:09:16 GMT, Matthias Bläsing
<github.com+2179736+matthiasblaes...@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
