Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 1659013003: Don't reset LoadEventProgress if frame unload has already started. (Closed)

Created:
4 years ago by dcheng
Modified:
4 years ago
Reviewers:
kinuko, Nate Chapin
CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Disable sub frame loads more reliably in frame detach. Previously, guarding against attaching a new frame in frame detach was done by inspecting LoadEventProgress. There are certain operations, such as document.open(), that can cause LoadEventProgress to never advance, causing the guard to get skipped. BUG=577105 Committed: https://crrev.com/a4bcdcb1f8df4f7427e208201501a9d8e41e386b Cr-Commit-Position: refs/heads/master@{#373581}

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : test #

Total comments: 5

Patch Set 4 : Better fix #

Total comments: 1

Patch Set 5 : Fix unit test and adjust comments. #

Total comments: 2

Patch Set 6 : Components fix #

Patch Set 7 : One more disabler #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -6 lines) Patch
A third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html View 1 2 1 chunk +25 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/frames/open-then-unload-expected.txt View 1 2 1 chunk +11 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.cpp View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h View 1 2 3 4 5 2 chunks +10 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebLocalFrameImpl.cpp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 37 (15 generated)
dcheng
https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html File third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html (right): https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html#newcode10 third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html:10: window[0].document.open(); At this point, LoadEventProgress gets reset to LoadEventNotRun ...
4 years ago (2016-02-03 03:13:29 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659013003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659013003/40001
4 years ago (2016-02-03 03:13:54 UTC) #5
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years ago (2016-02-03 04:55:29 UTC) #7
dcheng
I found a better (and more reliable way) to do this. We should still probably ...
4 years ago (2016-02-03 08:42:17 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659013003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659013003/60001
4 years ago (2016-02-03 08:42:50 UTC) #11
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/175501)
4 years ago (2016-02-03 09:42:47 UTC) #13
Mariusz Mlynski
On 2016/02/03 03:13:29, dcheng wrote: > https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp#newcode2776 > third_party/WebKit/Source/core/dom/Document.cpp:2776: m_loadEventProgress = > UnloadEventHandled; > This ...
4 years ago (2016-02-03 10:21:35 UTC) #14
kinuko
This looks reasonable to me, but it looks it breaks some test like WebViewTest.AddFrameInCloseURLUnload... where ...
4 years ago (2016-02-03 12:23:33 UTC) #15
dcheng
On 2016/02/03 at 12:23:33, kinuko wrote: > This looks reasonable to me, but it looks ...
4 years ago (2016-02-03 22:56:52 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659013003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659013003/80001
4 years ago (2016-02-03 22:57:13 UTC) #18
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32_ng/builds/153994) mac_chromium_compile_dbg_ng on ...
4 years ago (2016-02-03 23:14:57 UTC) #20
Nate Chapin
https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right): https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h#newcode116 third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:116: explicit SubframeLoadingDisabler(Node* root) Do we have to have constructors ...
4 years ago (2016-02-03 23:25:14 UTC) #21
dcheng
Also uploading a fix for the component build. https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right): https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h#newcode116 third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:116: explicit ...
4 years ago (2016-02-03 23:33:31 UTC) #22
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659013003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659013003/100001
4 years ago (2016-02-03 23:37:59 UTC) #24
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years ago (2016-02-04 02:36:58 UTC) #26
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659013003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659013003/120001
4 years ago (2016-02-04 08:54:07 UTC) #28
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years ago (2016-02-04 10:24:06 UTC) #30
kinuko
lgtm, though you'd probably want to wait for Nate's stamp :)
4 years ago (2016-02-04 15:11:35 UTC) #31
Nate Chapin
lgtm
4 years ago (2016-02-04 19:08:27 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659013003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659013003/120001
4 years ago (2016-02-04 19:11:17 UTC) #34
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years ago (2016-02-04 19:17:31 UTC) #35
commit-bot: I haz the power
4 years ago (2016-02-04 19:18:35 UTC) #37
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/a4bcdcb1f8df4f7427e208201501a9d8e41e386b
Cr-Commit-Position: refs/heads/master@{#373581}

Powered by Google App Engine
This is Rietveld 408576698