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

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

Created:
3 years, 7 months ago by dcheng
Modified:
3 years, 7 months 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 ...
3 years, 7 months 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
3 years, 7 months ago (2016-02-03 03:13:54 UTC) #5
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 7 months 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 ...
3 years, 7 months 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
3 years, 7 months 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)
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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
3 years, 7 months ago (2016-02-03 23:37:59 UTC) #24
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 7 months 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
3 years, 7 months ago (2016-02-04 08:54:07 UTC) #28
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 7 months ago (2016-02-04 10:24:06 UTC) #30
kinuko
lgtm, though you'd probably want to wait for Nate's stamp :)
3 years, 7 months ago (2016-02-04 15:11:35 UTC) #31
Nate Chapin
lgtm
3 years, 7 months 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
3 years, 7 months ago (2016-02-04 19:11:17 UTC) #34
commit-bot: I haz the power
Committed patchset #7 (id:120001)
3 years, 7 months ago (2016-02-04 19:17:31 UTC) #35
commit-bot: I haz the power
3 years, 7 months 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