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

Issue 1444183003: Cancel javascript: URL navigations if the frame was navigated. (Closed)

Created:
4 years, 1 month ago by dcheng
Modified:
3 years, 7 months ago
Reviewers:
haraken, Nate Chapin
CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, jochen (gone - plz use gerrit), loading-reviews_chromium.org, tyoshino+watch_chromium.org, Yuki
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't allow navigations in Document::detach. When navigating to a javascript: URL, Blink detaches the original Document. This process may detach plugin elements, causing a nested message loop to run. Document::detach() creates a ScriptForbiddenScope to prevent script from breaking invariants. Since plugins were detached synchronously, any script trying to execute in the nested message loop would be blocked. However, the fix for https://crbug.com/524120 defers plugin updates to happen outside the ScriptForbiddenScope. Thus, it is now possible to attach a *new* Document with a synchronous navigation while the old Document is being detached. BUG=546545 Committed: https://crrev.com/66ad73d642b9cf824f4b1f300811ed1ee6963da7 Cr-Commit-Position: refs/heads/master@{#360190}

Patch Set 1 #

Patch Set 2 : Approach #2 #

Patch Set 3 : Approach #2 #

Patch Set 4 : Approach #3 #

Total comments: 3

Patch Set 5 : Does not work #

Patch Set 6 : another version #

Patch Set 7 : Revert //chrome changes #

Patch Set 8 : Actually initialize new member (how did this work?) #

Patch Set 9 : more private #

Patch Set 10 : Revert change that is clearly not going to work #

Patch Set 11 : Constify #

Patch Set 12 : More comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -34 lines) Patch
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +20 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.cpp View 1 2 3 4 5 6 7 3 chunks +14 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/NavigationScheduler.h View 1 2 3 4 5 3 chunks +0 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/core/loader/NavigationScheduler.cpp View 1 2 3 4 5 3 chunks +2 lines, -14 lines 0 comments Download

Messages

Total messages: 29 (12 generated)
dcheng
Originally, I wanted to make a more comprehensive fix: performing deferred widget operations inside a ...
4 years, 1 month ago (2015-11-16 08:24:29 UTC) #2
haraken
LGTM https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode319 third_party/WebKit/Source/core/loader/FrameLoader.cpp:319: RefPtrWillBeRawPtr<DocumentLoader> documentLoader(m_frame->document()->loader()); Nit: It would be better to ...
4 years, 1 month ago (2015-11-16 08:36:00 UTC) #3
haraken
> https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp > File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): > > https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode319 > third_party/WebKit/Source/core/loader/FrameLoader.cpp:319: > RefPtrWillBeRawPtr<DocumentLoader> > documentLoader(m_frame->document()->loader()); ...
4 years, 1 month ago (2015-11-16 08:37:06 UTC) #4
Nate Chapin
https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode341 third_party/WebKit/Source/core/loader/FrameLoader.cpp:341: if (originalDocument != m_frame->document()) On 2015/11/16 08:24:29, dcheng wrote: ...
4 years ago (2015-11-16 19:16:34 UTC) #5
dcheng
On 2015/11/16 at 19:16:34, japhet wrote: > https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp > File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): > > https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode341 ...
4 years ago (2015-11-16 19:19:19 UTC) #6
dcheng
OK, new patch. Unfortunately, the loading stack is a house of cards that is on ...
4 years ago (2015-11-16 23:50:48 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1444183003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1444183003/200001
4 years ago (2015-11-17 01:35:01 UTC) #10
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/141516)
4 years ago (2015-11-17 02:17:04 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1444183003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1444183003/200001
4 years ago (2015-11-17 03:22:51 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years ago (2015-11-17 05:01:25 UTC) #16
Nate Chapin
LGTM Any hope of a test?
4 years ago (2015-11-17 17:47:10 UTC) #17
esprehn
On 2015/11/17 at 17:47:10, japhet wrote: > LGTM > > Any hope of a test? ...
4 years ago (2015-11-17 17:59:11 UTC) #18
dcheng
On 2015/11/17 at 17:59:11, esprehn wrote: > On 2015/11/17 at 17:47:10, japhet wrote: > > ...
4 years ago (2015-11-17 20:31:14 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1444183003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1444183003/200001
4 years ago (2015-11-17 21:04:14 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1444183003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1444183003/220001
4 years ago (2015-11-17 21:35:07 UTC) #26
commit-bot: I haz the power
Committed patchset #12 (id:220001)
4 years ago (2015-11-17 23:06:57 UTC) #27
commit-bot: I haz the power
4 years ago (2015-11-17 23:07:53 UTC) #28
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/66ad73d642b9cf824f4b1f300811ed1ee6963da7
Cr-Commit-Position: refs/heads/master@{#360190}

Powered by Google App Engine
This is Rietveld 408576698