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

8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute #47

Closed
wants to merge 4 commits into from

Conversation

@arun-Joseph
Copy link
Contributor

arun-Joseph commented Nov 20, 2019

Issue: Native part of WebView throws a DOMException and then, continues executing the rest of the function assuming that value is present. This causes the JVM to crash when retrieving the value.

Fix: Return from the function if exception was raised (code is similar to exception handling in WebKitLegacy/java/DOM/JavaTreeWalker.cpp)

This fix also needs to be applied to all function calls in WebKitLegacy/java/DOM functions which raises DOMError similar to createAttributeImpl().

Progress

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

Issue

JDK-8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute

Approvers

  • Kevin Rushforth (kcr - Reviewer) Note! Review applies to 7f6ed5b
  • Guru Hb (ghb - Reviewer)
@bridgekeeper
Copy link

bridgekeeper bot commented Nov 20, 2019

👋 Welcome back ajoseph! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Nov 20, 2019
@mlbridge
Copy link

mlbridge bot commented Nov 20, 2019

Copy link
Member

kevinrushforth left a comment

The proposed fix seems more like a workaround to me. There are dozens of very similar calls to raiseOnDOMError in this and other files, so I would think a more general solution is needed.

@arun-Joseph
Copy link
Contributor Author

arun-Joseph commented Nov 28, 2019

For calls to raiseOnDOMError() with argument of type ExceptionOr<Ref<T>>, the returned value is again passed through WTF::getPtr(). This doesn't modify the value returned, but removing it will require changing about 40 function calls.

Copy link
Member

kevinrushforth left a comment

I can confirm that this fixes the problem. The test case attached to the bug and the new unit test you added fail (crash) without your fix and throw the expected exception with your fix.

I don't quite understand your earlier comment about needing to change the caller when the type is ExceptionOr<Ref<T>>. The specific case in question is one where object passed to raiseDOMError is of type ExceptionOr<Ref<Attr>>, and your change works fine. What am I missing?

I added a comment about formatting (missing spaces around a + operator) in the test, but you can fix that before you integrate.

This will need a second reviewer, so I request @guruhb to review.

@openjdk openjdk bot removed the rfr label Dec 4, 2019
@openjdk
Copy link

openjdk bot commented Dec 4, 2019

@arun-Joseph This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute

Reviewed-by: kcr, ghb
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 29 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge master into your branch first.

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, @guruhb) 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).

@arun-Joseph
Copy link
Contributor Author

arun-Joseph commented Dec 5, 2019

In createAttributeImpl function in JavaDocument.cpp, the value returned from raiseOnDOMError is passed to WTF::getPtr function. Now there are two calls to WTF::getPtr, one from raiseOnDOMError in the return statement and another in createAttributeImpl, instead of just one. So, this approach has an unnecessary call to WTF::getPtr which, if needed, has to removed at the createAttributeImpl end. This doesn't modify the Attr object returned because WTF::getPtr returns the same pointer when called multiple times.

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 5, 2019

Thanks for the explanation. It seems fine the way it is, but if you want to file a follow-up issue, that would be fine, too.

@guruhb
guruhb approved these changes Jan 6, 2020
Copy link
Contributor

guruhb left a comment

+1, Looks good to me.

@arun-Joseph
Copy link
Contributor Author

arun-Joseph commented Jan 6, 2020

/integrate

@openjdk
Copy link

openjdk bot commented Jan 6, 2020

@arun-Joseph
Your change (at version 472d77c) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added sponsor ready and removed ready labels Jan 6, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 6, 2020

/sponsor

@openjdk openjdk bot closed this Jan 6, 2020
@openjdk openjdk bot added integrated and removed sponsor ready labels Jan 6, 2020
@openjdk
Copy link

openjdk bot commented Jan 6, 2020

@kevinrushforth @arun-Joseph The following commits have been pushed to master since your change was applied:

  • 3c4d68d: 8236626: Update copyright header for files modified in 2019
  • 580a2a9: 8087980: Add property to disable Monocle cursor
  • 3bbcbfb: 8225571: Port DND source to use GTK instead of GDK
  • 1952606: 8232811: Dialog's preferred size no longer accommodates multi-line strings
  • 4c6ebfb: 8236484: Compile error in monocle dispman
  • 8367e1a: 8130738: Add tabSize property to Text and TextFlow
  • 69e4ef3: 8235627: Blank stages when running JavaFX app in a macOS virtual machine
  • 5e0fb91: 8235364: Update copyright header for files modified in 2019
  • 935e99d: 8207957: TableSkinUtils should not contain actual code implementation
  • 4e005e4: 8227808: Make GTK3 libraries mandatory for building on Linux
  • 1140d34: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
  • d2d44b4: 8207759: VK_ENTER not consumed by TextField when default Button exists
  • fc539b5: 8223296: NullPointerException in GlassScene.java at line 325
  • 71ca899: 8220722: ProgressBarSkin: adds strong listener to control's width property
  • 07af89a: 8221334: TableViewSkin: must initialize flow's cellCount in constructor
  • 4ddf554: 8234916: [macos 10.15] Garbled text running with native-image
  • 46338d0: 8232524: SynchronizedObservableMap cannot be be protected for copying/iterating
  • a68347c: 8235150: IosApplication does not pass the required object in _leaveNestedEventLoopImpl
  • 1c27fbd: 8210955: DOMTest::testEventListenerCascade fails
  • 2d4096a: 8235151: Nonexistent notifyQuit method referred from iOS GlassHelper.m
  • 98035cb: 8211308: Support HTTP/2 in WebView
  • 6892fa1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
  • 1d670f1: 8200224: Multiple press event when JFXPanel gains focus
  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md
  • 4d3c723: 8234593: Mark LeakTest.testGarbageCollectability as unstable
  • 5a39824: 8234056: Upgrade to libxslt 1.1.34
  • 8bea7b7: 8229472: Deprecate for removal JavaBeanXxxPropertyBuilders constructors

Your commit was automatically rebased without conflicts.

Pushed as commit f1108b0.

@mlbridge
Copy link

mlbridge bot commented Jan 6, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: f1108b0
Author: Arun Joseph <ajoseph at openjdk.org>
Committer: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-01-06 20:58:28 +0000
URL: https://git.openjdk.java.net/jfx/commit/f1108b0a

8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute

Reviewed-by: kcr, ghb

! modules/javafx.web/src/main/native/Source/WebCore/bindings/java/JavaDOMUtils.h
! modules/javafx.web/src/test/java/test/javafx/scene/web/DOMTest.java

@arun-Joseph arun-Joseph deleted the arun-Joseph:8233747 branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.