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

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

Created:
4 years, 1 month ago by dcheng
Modified:
4 years, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2016-02-04 10:24:06 UTC) #30
kinuko
lgtm, though you'd probably want to wait for Nate's stamp :)
4 years, 1 month ago (2016-02-04 15:11:35 UTC) #31
Nate Chapin
lgtm
4 years, 1 month 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, 1 month ago (2016-02-04 19:11:17 UTC) #34
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 1 month ago (2016-02-04 19:17:31 UTC) #35
commit-bot: I haz the power
4 years, 1 month 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