[PointerLock] Add null check before dispatching click event (issue 2846993002 by chongz@chromium.org)

0 views
Skip to first unread message

cho...@chromium.org

unread,
Apr 27, 2017, 10:58:21 PM4/27/17
to scheib+...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Reviewers: scheib
CL: https://codereview.chromium.org/2846993002/

Message:
scheib@ PTAL, thanks!

Description:
[PointerLock] Add null check before dispatching click event

BUG=706802

Affected files (+25, -0 lines):
A third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerlock-remove-element-crash-manual.html
A third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerlock/pointerlock-remove-element-crash-manual-automation.js
M third_party/WebKit/Source/core/page/PointerLockController.cpp


Index: third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerlock-remove-element-crash-manual.html
diff --git a/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerlock-remove-element-crash-manual.html b/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerlock-remove-element-crash-manual.html
new file mode 100644
index 0000000000000000000000000000000000000000..e51faadf7cde3fea61743bbcef950f035820a9ea
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerlock-remove-element-crash-manual.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<title>Removing pointerLockElement on mouseup shouldn't crash</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+setup({explicit_timeout: true});
+document.addEventListener('mousedown', function() {
+ document.getElementById('lock').requestPointerLock();
+ document.addEventListener('mouseup', function() {
+ document.getElementById('lock').remove();
+ done();
+ });
+});
+</script>
+<p>Click anywhere to run the test. If a "PASS" result appears the test passes, otherwise it fails</p>
+<div id="lock"></div>
Index: third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerlock/pointerlock-remove-element-crash-manual-automation.js
diff --git a/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerlock/pointerlock-remove-element-crash-manual-automation.js b/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerlock/pointerlock-remove-element-crash-manual-automation.js
new file mode 100644
index 0000000000000000000000000000000000000000..e95f4f37047182805ffd465048f6589d7330ddbc
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerlock/pointerlock-remove-element-crash-manual-automation.js
@@ -0,0 +1,5 @@
+importAutomationScript('/pointerevents/pointerevent_common_input.js');
+
+function inject_input() {
+ return mouseClickInTarget('#lock');
+}
Index: third_party/WebKit/Source/core/page/PointerLockController.cpp
diff --git a/third_party/WebKit/Source/core/page/PointerLockController.cpp b/third_party/WebKit/Source/core/page/PointerLockController.cpp
index 6c00982d5fa06abe5040104b4f7495a1f8a4a626..f9aa43bb099a1c7c5ab1b8665362ec992695742c 100644
--- a/third_party/WebKit/Source/core/page/PointerLockController.cpp
+++ b/third_party/WebKit/Source/core/page/PointerLockController.cpp
@@ -140,6 +140,10 @@ void PointerLockController::DispatchLockedMouseEvent(

element_->DispatchMouseEvent(event, event_type, event.click_count);

+ // Event handlers may remove element.
+ if (!element_)
+ return;
+
// Create click events
if (event_type == EventTypeNames::mouseup) {
element_->DispatchMouseEvent(event, EventTypeNames::click,


sch...@chromium.org

unread,
Apr 28, 2017, 12:22:45 AM4/28/17
to cho...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

cho...@chromium.org

unread,
Apr 28, 2017, 10:59:06 AM4/28/17
to scheib+...@chromium.org, rby...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
rbyers@ PTAL at the wpt, thanks!

Can I put it under "wpt/pointerevents/pointerlock/" like this or should I create
a top level dir "wpt/pointerlock/"?

https://codereview.chromium.org/2846993002/

rby...@chromium.org

unread,
Apr 29, 2017, 12:11:32 PM4/29/17
to cho...@chromium.org, scheib+...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
On 2017/04/28 14:59:05, chongz wrote:
> rbyers@ PTAL at the wpt, thanks!

The way the test is described ("doesn't crash"), it's more of an implementation
test than an interop test. You could argue that this test should just live in
blink instead of WPT. Then again, validating the exact behavior of pointerlock
when it's target is removed on mouseup could still be a useful interop test - it
should just check the exact behavior instead of being described as "verifying
the browser doesn't crash". shceib@ WDYT?


> Can I put it under "wpt/pointerevents/pointerlock/" like this or should I
create
> a top level dir "wpt/pointerlock/"?

Since this test doesn't actually use pointerevents, it shouldn't be in the
pointerevents directory (the top-level directory indicates the primary spec
being tested, and the test results in that directory are used as part of
evaluating the interop/maturity status for that spec). There is already a
pointerlock directory in WPT, including with a test that's fairly similar to
this:
https://github.com/w3c/web-platform-tests/blob/66ffd47cfc514ac8a30f540187c25b86cdf7c42f/pointerlock/pointerlock_remove_target-manual.html.
But apparently we're not importing those tests into blink yet. I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=716766 for that (just
importing is pretty trivial - the manual tests won't run by default).




https://codereview.chromium.org/2846993002/

sch...@chromium.org

unread,
Apr 29, 2017, 7:22:46 PM4/29/17
to cho...@chromium.org, rby...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
On 2017/04/29 16:11:32, Rick Byers wrote:
> On 2017/04/28 14:59:05, chongz wrote:
> > rbyers@ PTAL at the wpt, thanks!
>
> The way the test is described ("doesn't crash"), it's more of an
implementation
> test than an interop test. You could argue that this test should just live in
> blink instead of WPT. Then again, validating the exact behavior of
pointerlock
> when it's target is removed on mouseup could still be a useful interop test -
it
> should just check the exact behavior instead of being described as "verifying
> the browser doesn't crash". shceib@ WDYT?

IMHO 'don't crash' tests are valid in WPT because everyone can crash... and it's
not overly implementation specific. Easy to see others making this mistake too.

If trying to expand this test to be larger... I suppose we could check the
target parent doesn't receive any events. (It never should, events are
dispatched with bubbles==false).

- ;) scheib

cho...@chromium.org

unread,
May 2, 2017, 2:47:39 PM5/2/17
to scheib+...@chromium.org, rby...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
rbyers@ I've renamed the test and moved it to "wpt/pointerlock/", PTAL again,
thanks!
Here is the CL to import "wpt/pointerlock/":
https://codereview.chromium.org/2853343002/
(I assume the bot can handle directory merging and won't conflict with this
patch)

Also I think my test is necessary as
https://github.com/w3c/web-platform-tests/blob/master/pointerlock/pointerlock_remove_target-manual.html
doesn't cover the "mouseup" case.

https://codereview.chromium.org/2846993002/

rby...@google.com

unread,
May 2, 2017, 5:44:04 PM5/2/17
to cho...@chromium.org, scheib+...@chromium.org, rby...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 3, 2017, 10:36:57 AM5/3/17
to cho...@chromium.org, scheib+...@chromium.org, rby...@chromium.org, rby...@google.com, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 3, 2017, 10:45:37 AM5/3/17
to cho...@chromium.org, scheib+...@chromium.org, rby...@chromium.org, rby...@google.com, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/426306)

https://codereview.chromium.org/2846993002/

cho...@chromium.org

unread,
May 3, 2017, 10:50:56 AM5/3/17
to scheib+...@chromium.org, rby...@chromium.org, rby...@google.com, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
On 2017/05/03 14:45:36, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
> chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/426306)

Hmm it seems that @google email won't work... rbyers@ do you mind stamping
again? Thanks!

https://codereview.chromium.org/2846993002/

cho...@chromium.org

unread,
May 4, 2017, 11:39:26 AM5/4/17
to scheib+...@chromium.org, rby...@chromium.org, rby...@google.com, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
rbyers@ can you stamp again with the @chromium account? Thanks!

https://codereview.chromium.org/2846993002/

rby...@chromium.org

unread,
May 4, 2017, 5:28:41 PM5/4/17
to cho...@chromium.org, scheib+...@chromium.org, rby...@google.com, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
On 2017/05/04 15:39:24, chongz wrote:
> rbyers@ can you stamp again with the @chromium account? Thanks!

Damn, sorry - multi-signin has been really buggy for me lately. LGTM

https://codereview.chromium.org/2846993002/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 4, 2017, 5:45:35 PM5/4/17
to cho...@chromium.org, scheib+...@chromium.org, rby...@chromium.org, rby...@google.com, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 4, 2017, 7:20:01 PM5/4/17
to cho...@chromium.org, scheib+...@chromium.org, rby...@chromium.org, rby...@google.com, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, nzol...@chromium.org
Reply all
Reply to author
Forward
0 new messages