Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815803003:
Make Pageshow event asynchronous in EventQueueScope (Closed)
DescriptionMake Pageshow event asynchronous in EventQueueScope
As per spec Pageshow event must be fired asynchonously all the time, but to be compatible with other browsers blink used to fire Pageshow synchronously.
And thus Pageshow event was able to modify nodes when it should not.
This CL makes Pageshow fired asynchronously when it is in EventQueueScope to solve the problem.
BUG=660269, 543424
Review-Url: https://codereview.chromium.org/2815803003
Cr-Commit-Position: refs/heads/master@{#464347}
Committed: https://chromium.googlesource.com/chromium/src/+/f8de91ba0d69c4f2b64407c5a218774e554e0c9a
Patch Set 1 #Patch Set 2 : Delete unnecessary change #
Total comments: 4
Messages
Total messages: 23 (11 generated)
The CQ bit was checked by yuzus@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yuzus@chromium.org changed reviewers: + kochi@chromium.org, yosin@chromium.org
PTAL :)
lgtm https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:429: if (ScopedEventQueue::Instance()->ShouldQueueEvents() && document_) { Nice finding. I've not known |(ScopedEventQueue::Instance()->ShouldQueueEvents()|. (^_^)b
yosin@chromium.org changed reviewers: + tkent@chromium.org
+tkent@ for OWNERS review https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Range/insertNode-trigger-onpageshow-crash.html (right): https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Range/insertNode-trigger-onpageshow-crash.html:1: <!DOCTYPE html> Could you use gTest instead of layout test to verify we defer "pageshow" event? Instead of indirectly checking it? For example: TEST_F(...) { ... build HTML ... ... install event handler to check "pageshow" event dispatched after ScopedEventQueue ... { ScopedEventQueue scope; ... dispatch "pageshow" event ... EXPECT_TRUE(... scope has queued "pageshow" event" ...) } EXPECT_TRUE(... "pageshow" event dispatched ... "); }
https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:427: // asynchronously. However to be compatible with other browsers blink fires Did you check behavior of latest versions of other browsers? If some other browsers dispatch it asynchronously, it should be safe to switch to unconditional asynchronous dispatch.
On 2017/04/13 at 00:56:16, yosin wrote: > +tkent@ for OWNERS review > > https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/dom/Range/insertNode-trigger-onpageshow-crash.html (right): > > https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/dom/Range/insertNode-trigger-onpageshow-crash.html:1: <!DOCTYPE html> > Could you use gTest instead of layout test to verify we defer "pageshow" event? > Instead of indirectly checking it? > > For example: > TEST_F(...) { > ... build HTML ... > ... install event handler to check "pageshow" event dispatched after ScopedEventQueue ... > { > ScopedEventQueue scope; > ... dispatch "pageshow" event ... > EXPECT_TRUE(... scope has queued "pageshow" event" ...) > } > EXPECT_TRUE(... "pageshow" event dispatched ... "); > } I'm going to address this later on in a different patch. Thanks!
https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:427: // asynchronously. However to be compatible with other browsers blink fires On 2017/04/13 at 01:26:16, tkent wrote: > Did you check behavior of latest versions of other browsers? > > If some other browsers dispatch it asynchronously, it should be safe to switch to unconditional asynchronous dispatch. We had offline discussion. It's too difficult to reproduce synchronous pageshow dispatch even in Chrome. So, investigating other browsers behavior is also too hard. We may go ahead with the current approach. lgtm
I think this is a safe change (call onpageshow event asynchronously when necessary) to make. LGTM.
On 2017/04/13 at 01:26:16, tkent wrote: > https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): > > https://codereview.chromium.org/2815803003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:427: // asynchronously. However to be compatible with other browsers blink fires > Did you check behavior of latest versions of other browsers? > > If some other browsers dispatch it asynchronously, it should be safe to switch to unconditional asynchronous dispatch. It is hard to see whether PageShow event itself is implemented as async/sync since Load event, which happens before PageShow event, is implemented as async in most cases. For now I'm gonna leave this as it is.
The CQ bit was checked by yuzus@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yuzus@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492069934872620,
"parent_rev": "0ce68a3d93044122537ef22d6490319bac338c88", "commit_rev":
"f8de91ba0d69c4f2b64407c5a218774e554e0c9a"}
Message was sent while issue was closed.
Description was changed from ========== Make Pageshow event asynchronous in EventQueueScope As per spec Pageshow event must be fired asynchonously all the time, but to be compatible with other browsers blink used to fire Pageshow synchronously. And thus Pageshow event was able to modify nodes when it should not. This CL makes Pageshow fired asynchronously when it is in EventQueueScope to solve the problem. BUG=660269,543424 ========== to ========== Make Pageshow event asynchronous in EventQueueScope As per spec Pageshow event must be fired asynchonously all the time, but to be compatible with other browsers blink used to fire Pageshow synchronously. And thus Pageshow event was able to modify nodes when it should not. This CL makes Pageshow fired asynchronously when it is in EventQueueScope to solve the problem. BUG=660269,543424 Review-Url: https://codereview.chromium.org/2815803003 Cr-Commit-Position: refs/heads/master@{#464347} Committed: https://chromium.googlesource.com/chromium/src/+/f8de91ba0d69c4f2b64407c5a218... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f8de91ba0d69c4f2b64407c5a218... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
