Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8242361: JavaFX Web View crashes with Segmentation Fault, when HTML contains Data-URIs #360

Closed
wants to merge 6 commits into from

Conversation

@matthiasblaesing
Copy link
Collaborator

@matthiasblaesing matthiasblaesing commented Nov 24, 2020

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8242361: JavaFX Web View crashes with Segmentation Fault, when HTML contains Data-URIs

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/360/head:pull/360
$ git checkout pull/360

…ontains Data-URIs

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
@bridgekeeper bridgekeeper bot added the oca label Nov 24, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 24, 2020

Hi @matthiasblaesing, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user matthiasblaesing" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@matthiasblaesing
Copy link
Collaborator Author

@matthiasblaesing matthiasblaesing commented Nov 25, 2020

/signed

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 25, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 25, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 27, 2020

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 12, 2020

I'll plan to look at this next week. I have a general question and a comment:

  1. Would it be possible to turn the test into an automated one? Possibly using some of the same techniques that the ModuleLauncherTest does?
  2. All of the new files need a proper copyright header (including the new build.gradle).
@matthiasblaesing
Copy link
Collaborator Author

@matthiasblaesing matthiasblaesing commented Dec 13, 2020

1. Would it be possible to turn the test into an automated one? Possibly using some of the same techniques that the ModuleLauncherTest does?

Thank you for the pointer to ModuleLauncherTest indeed that was an inspiration for integration. I updated this PR with an integrated test. The new test is derived from the original manual test, but reworked to be automatically checkable. It was verified to fail without the fix and ran clean after.

2. All of the new files need a proper copyright header (including the new `build.gradle`).

Sorry about that - should be fixed in the updated version.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

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.

- The text and formatting was adjusted according to the raised concerns.
- The test was modified with a 15000ms timeout as is the case for other
  tests in the systemTests project
- A safeguard was added in the launched child JVM, that terminates the
  child after 15000ms is it did not exit normally
- The detection of a successful test was moved to a ChangeLister on the
  load worker state. It is assumed, that the worker success state is
  similar to the DOMContentLoaded event. The latter is fired after
  synchronous javascript was executed and that would be late enough to
  "see" the DOM update of the title.

It was validated, that the test still fails without the fix and succeeds
with it.
@kevinrushforth kevinrushforth self-requested a review Dec 15, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

The updates look good. Two additional comments on the test.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 16, 2020

One more thing: can you enable GitHub actions test execution for your repo? See this message from the Skara bot.

Copy link
Member

@arun-Joseph arun-Joseph left a comment

Fix and test looks good. Added a minor typo fix.

- Shifted error codes of the value "1"
- Move title fetching to SUCCEEDED state
- Fix typo
- enable github actions on contributor repository
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@matthiasblaesing This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: kcr, ajoseph

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 11 new commits pushed to the master branch:

  • f2928d9: 8256983: GitHub actions: specify the version of each platform OS and compiler
  • ebb59e9: 8201568: zForce touchscreen input device fails when closed and immediately reopened
  • 97d655f: 8256012: Fix build of Monocle for Linux
  • e1adfa9: 8257758: Allow building of JavaFX native libs for Apple Silicon
  • 1a8652a: 8257719: JFXPanel scene fails to render correctly on HiDPI after fix for JDK-8199592
  • 7fa02fe: 8257897: Fix webkit build for XCode 12
  • 794ffc0: 8231372: JFXPanel fails to render if setScene called on Swing thread
  • e7dbdfc: 8233678: [macos 10.15] System menu bar does not work initially on macOS Catalina
  • ca0d3d0: 8256821: TreeViewSkin/Behavior: misbehavior on switching skin
  • 00a8646: 8247576: Labeled/SkinBase: misbehavior on switching skin
  • ... and 1 more: https://git.openjdk.java.net/jfx/compare/bd510896e56eb844c07f4abf80150ea8c0d5e284...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arun-Joseph) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Dec 17, 2020
@matthiasblaesing
Copy link
Collaborator Author

@matthiasblaesing matthiasblaesing commented Dec 17, 2020

/integrate

@openjdk openjdk bot added the sponsor label Dec 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@matthiasblaesing
Your change (at version 7857164) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 17, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@kevinrushforth @matthiasblaesing Since your change was applied there have been 12 commits pushed to the master branch:

  • fb8e0cd: 8253356: JavaFX Terminology Refresh
  • f2928d9: 8256983: GitHub actions: specify the version of each platform OS and compiler
  • ebb59e9: 8201568: zForce touchscreen input device fails when closed and immediately reopened
  • 97d655f: 8256012: Fix build of Monocle for Linux
  • e1adfa9: 8257758: Allow building of JavaFX native libs for Apple Silicon
  • 1a8652a: 8257719: JFXPanel scene fails to render correctly on HiDPI after fix for JDK-8199592
  • 7fa02fe: 8257897: Fix webkit build for XCode 12
  • 794ffc0: 8231372: JFXPanel fails to render if setScene called on Swing thread
  • e7dbdfc: 8233678: [macos 10.15] System menu bar does not work initially on macOS Catalina
  • ca0d3d0: 8256821: TreeViewSkin/Behavior: misbehavior on switching skin
  • ... and 2 more: https://git.openjdk.java.net/jfx/compare/bd510896e56eb844c07f4abf80150ea8c0d5e284...master

Your commit was automatically rebased without conflicts.

Pushed as commit e61b923.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants