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

8236912: NullPointerException when clicking in WebView with Button 4 or Button 5 #85

Closed
wants to merge 1 commit into from

Conversation

@effad
Copy link

effad commented Jan 14, 2020

As documented in JDK-8236912, WebView did not check whether the idMap really contained a mapping for the given button, making it prone to errors, when things are extended (as has happened here).

The fix consists of two test cases that show the problem in unfixed WebViews and a fix which works analogously to the check whether the given event type is mapped.

Progress

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

Issue

JDK-8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

Approvers

  • Guru Hb (ghb - Reviewer)
  • Kevin Rushforth (kcr - Reviewer)
@bridgekeeper

This comment has been minimized.

Copy link

bridgekeeper bot commented Jan 14, 2020

👋 Welcome back rlichten! 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 Jan 14, 2020
@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Jan 14, 2020

Webrevs

@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 14, 2020

The PR title should match the title of the bug report in JBS exactly. Can you change it to:

8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

This will need two reviewers. I'll review it and also request @guruhb to review.

@kevinrushforth kevinrushforth self-requested a review Jan 14, 2020
@effad effad changed the title 8236912: Preventing NPE when clicking WebView with forward/back mouse buttons 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5 Jan 14, 2020
@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Jan 14, 2020

Mailing list message from Robert Lichtenberger on openjfx-dev:

How strange: Other commits to the master branch of my forked repository
(relating to another issue, already integrated into OpenJFX) are also
showing up here. But the change really only consists of 542ff60 which
contains the fix for JDK-8236912. My older contributions were not checked
into a separate branch (of my fork), maybe that was the problem. If there
is anything else I did wrong, please tell me ;-).

The Changes-Link and the Webrev correctly show that only two files are
changed.

Am Di., 14. Jan. 2020 um 13:02 Uhr schrieb Robert Lichtenberger <
rlichten at openjdk.java.net>:

As documented in JDK-8236912, WebView did not check whether the idMap
really contained a mapping for the given button, making it prone to errors,
when things are extended (as has happened here).

The fix consists of two test cases that show the problem in unfixed
WebViews and a fix which works analogously to the check whether the given
event type is mapped.

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

Commits:
- 542ff60: Fix null pointer exception when clicking into WebViews with
forward/back mouse button.
- 2109d5a: Merge remote-tracking branch 'upstream/master'
- acfa63b: Merge remote-tracking branch 'upstream/master'
- 7c5cf19: 8232524: Test cleanup: terminate background thread upon
failure.
- 7e80839: 8232524: SynchronizedObservableMap cannot be be protected for
copying/iterating
- 8ecf354: JDK-8232524 fixed.

Changes: https://git.openjdk.java.net/jfx/pull/85/files
Webrev: https://webrevs.openjdk.java.net/jfx/85/webrev.00
Issue: https://bugs.openjdk.java.net/browse/JDK-8236912
Stats: 18 lines in 2 files changed: 15 ins; 0 del; 3 mod
Patch: https://git.openjdk.java.net/jfx/pull/85.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/85/head:pull/85

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

@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Jan 14, 2020

Mailing list message from Robert Lichtenberger on openjfx-dev:

PR title changed.

Am Di., 14. Jan. 2020 um 13:07 Uhr schrieb Kevin Rushforth <
kcr at openjdk.java.net>:

On Tue, 14 Jan 2020 12:01:11 GMT, Robert Lichtenberger <
rlichten at openjdk.org> wrote:

As documented in JDK-8236912, WebView did not check whether the idMap
really contained a mapping for the given button, making it prone to errors,
when things are extended (as has happened here).

The fix consists of two test cases that show the problem in unfixed
WebViews and a fix which works analogously to the check whether the given
event type is mapped.

The PR title should match the title of the bug report in JBS exactly. Can
you change it to:

8236912: NullPointerException when clicking in WebView with Button 4 or
Button 5

This will need two reviewers. I'll review it and also request @guruhb to
review.

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

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

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

guruhb left a comment

+1, Looks good to me.

@openjdk openjdk bot removed the rfr label Jan 14, 2020
@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Jan 14, 2020

@effad 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:

8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

Reviewed-by: ghb, kcr
  • 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 19 commits pushed to the jfx14 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 jfx14 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 (@guruhb, @kevinrushforth) 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 Jan 14, 2020
@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 14, 2020

@effad - The extra commits and merge are no problem, provided that you want to get this into openjfx15.

However, if you intend to get this into openjfx14, as currently targeted in JBS, then you will need to:

  1. Rebase your JDK-8236912 branch on top of the upstream jfx14 branch
  2. Force-push your branch: git push --force origin JDK-8236912
  3. Edit this PR and change the target to the jfx14 branch
@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 14, 2020

NOTE that this PR is not yet ready for integration. In addition to needing one more reviewer (me), there is the open question of which branch it should be targeted to.

@effad I think that this should go into openjfx14, so the above steps I listed will allow for this.

@guruhb can you please cancel your review (click the "re-request review" icon next to your review) until the above is done?

@effad effad changed the base branch from master to jfx14 Jan 14, 2020
@effad

This comment has been minimized.

Copy link
Author

effad commented Jan 14, 2020

I've rebased my branch like this:

git fetch upstream
git rebase upstream/jfx14
git pull
git push

and changed the base branch of this PR to jfx14. Hope that was the correct way to do this ;-).

@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 14, 2020

Your branch still has commits from master after jfx14 was branched. This was the problem:

git pull

This pulled in additional changes from master that were already in your branch and effectively undid the rebase. You need to redo it, but this time instead of pulling changes after you rebase, you should simply do:

git push --force origin JDK-8236912
@effad effad force-pushed the effad:JDK-8236912 branch from 283c547 to 8c3f601 Jan 14, 2020
@effad

This comment has been minimized.

Copy link
Author

effad commented Jan 14, 2020

git push --force origin JDK-8236912

I did

git rebase upstream/jfx14
git push --force origin JDK-8236912

but that has messed up things further, didn't it?

Copy link
Contributor

guruhb left a comment

Partially reviewed, Need to re-check after the change of branch.

@openjdk openjdk bot added rfr and removed ready labels Jan 14, 2020
@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 14, 2020

Oh, I see the problem. In addition to rebasing you need to exclude any commits that are from the master branch and are not your. So what you really need to do is:

git rebase -i upstream/jfx14
<EDIT THE LIST OF COMMITS AND DROP ANY THAT ARE NOT YOURS>
git push --force origin JDK-8236912

Sorry for not anticipating this problem.

…ack mouse button.
@effad effad force-pushed the effad:JDK-8236912 branch from 8c3f601 to aa03cf1 Jan 14, 2020
@effad

This comment has been minimized.

Copy link
Author

effad commented Jan 14, 2020

Sorry for not anticipating this problem.
I am sorry for not knowing git well enough. Thank you for your patience and help ;-). I think the PR now contains what it should ;-).

@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 14, 2020

Yes, it looks fine now, so it is ready for review. And this is one thing that can be a bit tricky about using git with multiple branches.

Btw, I was going to suggest git rebase -i the first time (it's what I always do in such a situation), but thought I could save you a step. Lesson learned. :)

Copy link
Member

kevinrushforth left a comment

Looks good to me.

@guruhb can you re-review this when you get a chance?

@openjdk openjdk bot removed the rfr label Jan 17, 2020
@openjdk openjdk bot added the ready label Jan 17, 2020
@guruhb

This comment has been minimized.

Copy link
Contributor

guruhb commented Jan 24, 2020

+1, Looks good to me.

@guruhb
guruhb approved these changes Jan 24, 2020
@openjdk openjdk bot added the rfr label Jan 24, 2020
@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 24, 2020

@effad you can integrate this when ready. I will sponsor it.

@effad

This comment has been minimized.

Copy link
Author

effad commented Jan 27, 2020

/integrate

@openjdk openjdk bot added the sponsor label Jan 27, 2020
@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Jan 27, 2020

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

@kevinrushforth

This comment has been minimized.

Copy link
Member

kevinrushforth commented Jan 27, 2020

/sponsor

@openjdk openjdk bot closed this Jan 27, 2020
@openjdk openjdk bot added integrated and removed sponsor labels Jan 27, 2020
@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Jan 27, 2020

@kevinrushforth @effad The following commits have been pushed to jfx14 since your change was applied:

Your commit was automatically rebased without conflicts.

Pushed as commit aa6f3a4.

@openjdk openjdk bot removed the ready label Jan 27, 2020
@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Jan 27, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: aa6f3a4
Author: Robert Lichtenberger <rlichten at openjdk.org>
Committer: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-01-27 13:43:47 +0000
URL: https://git.openjdk.java.net/jfx/commit/aa6f3a4e

8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

Reviewed-by: ghb, kcr

! modules/javafx.web/src/main/java/javafx/scene/web/WebView.java
! modules/javafx.web/src/test/java/test/javafx/scene/web/WebViewTest.java

@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Jan 29, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: aa6f3a4
Author: Robert Lichtenberger <rlichten at openjdk.org>
Committer: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-01-27 13:43:47 +0000
URL: https://git.openjdk.java.net/jfx/commit/aa6f3a4e

8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

Reviewed-by: ghb, kcr

! modules/javafx.web/src/main/java/javafx/scene/web/WebView.java
! modules/javafx.web/src/test/java/test/javafx/scene/web/WebViewTest.java

@kevinrushforth kevinrushforth removed the rfr label Feb 19, 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.