Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(504)

Issue 1714063002: Protect the provisional loader from detaching during prepareForCommit (Closed)

Created:
4 years, 10 months ago by Charlie Harrison
Modified:
4 years, 10 months ago
Reviewers:
Nate Chapin, dcheng
CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, nasko, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Protect the provisional loader from detaching during prepareForCommit This patch fixes a crash caused by a frame losing both its DocumentLoaders. When the main document loader is detached, various subresources can hook into 'abort' and try to stop the provisional load that is about to commit. This causes both document loaders to be detached, and the frame to be in a non-detached limbo state. BUG=394055 Committed: https://crrev.com/1d95c6320dce15e12545ac8856ab9f0e77aa12ed Cr-Commit-Position: refs/heads/master@{#377900}

Patch Set 1 #

Total comments: 1

Patch Set 2 : use TemporaryChange #

Patch Set 3 : Added a layout test, fixed bugs with initial fix #

Total comments: 7

Patch Set 4 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -9 lines) Patch
A third_party/WebKit/LayoutTests/http/tests/loading/stop-load-at-commit.html View 1 2 1 chunk +22 lines, -0 lines 0 comments Download
A + third_party/WebKit/LayoutTests/http/tests/loading/stop-load-at-commit-expected.txt View 1 2 2 chunks +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.h View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 chunks +17 lines, -7 lines 0 comments Download

Messages

Total messages: 24 (6 generated)
Charlie Harrison
Nate, PTAL. I couldn't reproduce this with a layout test, but we have the clusterfuzz ...
4 years, 10 months ago (2016-02-19 16:45:45 UTC) #2
Charlie Harrison
cc nasko
4 years, 10 months ago (2016-02-19 17:02:58 UTC) #3
dcheng
https://codereview.chromium.org/1714063002/diff/1/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1714063002/diff/1/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1075 third_party/WebKit/Source/core/loader/FrameLoader.cpp:1075: m_protectProvisionalLoader = false; Shouldn't this be true? Also, maybe ...
4 years, 10 months ago (2016-02-19 18:34:40 UTC) #5
dcheng
Also, can we write a test to cover this? Hopefully we can just get away ...
4 years, 10 months ago (2016-02-19 18:37:02 UTC) #6
Charlie Harrison
On 2016/02/19 18:37:02, dcheng wrote: > Also, can we write a test to cover this? ...
4 years, 10 months ago (2016-02-19 19:04:37 UTC) #7
Charlie Harrison
As usual this ended up being more complex than expected. We can't assert that there ...
4 years, 10 months ago (2016-02-19 23:43:52 UTC) #8
dcheng
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1007 third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if (!m_protectProvisionalLoader) Would it be slightly more robust to ...
4 years, 10 months ago (2016-02-22 18:12:07 UTC) #9
Charlie Harrison
Sorry, I thought I submitted this a few days ago. https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1007 ...
4 years, 10 months ago (2016-02-24 17:09:15 UTC) #10
dcheng
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1007 third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if (!m_protectProvisionalLoader) On 2016/02/24 at 17:09:15, csharrison wrote: > ...
4 years, 10 months ago (2016-02-24 21:22:59 UTC) #11
Charlie Harrison
On 2016/02/24 at 21:22:59, dcheng wrote: > https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp > File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): > > https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode1007 ...
4 years, 10 months ago (2016-02-25 00:41:09 UTC) #12
Charlie Harrison
On 2016/02/25 at 00:41:09, csharrison wrote: > On 2016/02/24 at 21:22:59, dcheng wrote: > > ...
4 years, 10 months ago (2016-02-25 00:41:27 UTC) #13
Charlie Harrison
On 2016/02/25 at 00:41:27, csharrison wrote: > On 2016/02/25 at 00:41:09, csharrison wrote: > > ...
4 years, 10 months ago (2016-02-25 00:45:29 UTC) #14
dcheng
lgtm
4 years, 10 months ago (2016-02-25 18:40:54 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714063002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714063002/60001
4 years, 10 months ago (2016-02-25 21:14:04 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/29184)
4 years, 10 months ago (2016-02-25 22:14:31 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714063002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714063002/60001
4 years, 10 months ago (2016-02-26 15:14:53 UTC) #21
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 10 months ago (2016-02-26 16:41:54 UTC) #22
commit-bot: I haz the power
4 years, 10 months ago (2016-02-26 16:43:22 UTC) #24
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/1d95c6320dce15e12545ac8856ab9f0e77aa12ed
Cr-Commit-Position: refs/heads/master@{#377900}

Powered by Google App Engine
This is Rietveld 408576698