Protect the provisional loader from detaching during prepareForCommit

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}

[Charlie Harrison, 2016-02-19 16:45:45]

csharrison@chromium.org changed reviewers:
+ japhet@chromium.org

[Charlie Harrison, 2016-02-19 16:45:45]

Nate, PTAL. I couldn't reproduce this with a layout test, but we have the
clusterfuzz repro which is probably good enough. Let me know what you think.

[Charlie Harrison, 2016-02-19 17:02:58]

cc nasko

[dcheng, 2016-02-19 18:34:39]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-02-19 18:34:40]

https://codereview.chromium.org/1714063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1714063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1075:
m_protectProvisionalLoader = false;
Shouldn't this be true?

Also, maybe use TemporaryChange for this.

[dcheng, 2016-02-19 18:37:02]

Also, can we write a test to cover this? Hopefully we can just get away with
writing a simtest, but worst case scenario, we should be able to demonstrate
this failure mode in a http layout test too.

[Charlie Harrison, 2016-02-19 19:04:37]

On 2016/02/19 18:37:02, dcheng wrote:
> Also, can we write a test to cover this? Hopefully we can just get away with
> writing a simtest, but worst case scenario, we should be able to demonstrate
> this failure mode in a http layout test too.

Yeah I'll use TemporaryChange, looks perfect for this case. I'll try to write a
simtest to repro. I tried to write an http layout test but couldn't get it to
reliably repro or even have reliable results. A repro script can be found in the
crbug, but it sometimes requires an extra navigation to trigger.

[Charlie Harrison, 2016-02-19 23:43:52]

As usual this ended up being more complex than expected. We can't assert that
there will be a provisional loader at the point I did because of
navigation-abort-detaches-frame.html, where a similar operation detaches the
frame in the abort callback. In this case we don't want to protect the
provisional loader for the duration of the scope.

I hacked around this by setting the protect bool to false when we're unloading
the frame, but it feels dirty. Trybots are looking ok so far...

[dcheng, 2016-02-22 18:12:07]

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if
(!m_protectProvisionalLoader)
Would it be slightly more robust to put this check into DocumentLoader?

Actually, I wonder if it makes sense to move this tracking into DocumentLoader
completely: then we don't need to make sure all the entry points to
stopLoading() and detachDocumentLoader() remember to check this bool: the
DocumentLoader itself can just make it a no-op and return immediately. WDYT?

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1074: // stop(), which
will in turn detach the provisional document loader.
Nit: window.stop()

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1083: // TODO(dcheng):
Investigate if this can be moved above the check that
Nit: clean up this TODO as well, it doesn't really apply if the provisional
document loader can't go away.

[Charlie Harrison, 2016-02-24 17:09:15]

Sorry, I thought I submitted this a few days ago.

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if
(!m_protectProvisionalLoader)
On 2016/02/22 at 18:12:07, dcheng wrote:
> Would it be slightly more robust to put this check into DocumentLoader?
> 
> Actually, I wonder if it makes sense to move this tracking into DocumentLoader
completely: then we don't need to make sure all the entry points to
stopLoading() and detachDocumentLoader() remember to check this bool: the
DocumentLoader itself can just make it a no-op and return immediately. WDYT?

I'm not sure. My first attempt is giving me crashes deep in
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld (service
worker related). I suspect there are edge cases where we want to detach the
provisional document loader. We just don't want to do it in |stopAllLoaders|.

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1074: // stop(), which
will in turn detach the provisional document loader.
On 2016/02/22 at 18:12:07, dcheng wrote:
> Nit: window.stop()

Done.

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1083: // TODO(dcheng):
Investigate if this can be moved above the check that
On 2016/02/22 at 18:12:07, dcheng wrote:
> Nit: clean up this TODO as well, it doesn't really apply if the provisional
document loader can't go away.

Done.

[dcheng, 2016-02-24 21:22:59]

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if
(!m_protectProvisionalLoader)
On 2016/02/24 at 17:09:15, csharrison wrote:
> On 2016/02/22 at 18:12:07, dcheng wrote:
> > Would it be slightly more robust to put this check into DocumentLoader?
> > 
> > Actually, I wonder if it makes sense to move this tracking into
DocumentLoader completely: then we don't need to make sure all the entry points
to stopLoading() and detachDocumentLoader() remember to check this bool: the
DocumentLoader itself can just make it a no-op and return immediately. WDYT?
> 
> I'm not sure. My first attempt is giving me crashes deep in
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld (service
worker related). I suspect there are edge cases where we want to detach the
provisional document loader. We just don't want to do it in |stopAllLoaders|.

Hmm, mind posting a stack on here? I'm kind of curious what it looks like.

[Charlie Harrison, 2016-02-25 00:41:09]

On 2016/02/24 at 21:22:59, dcheng wrote:
>
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if
(!m_protectProvisionalLoader)
> On 2016/02/24 at 17:09:15, csharrison wrote:
> > On 2016/02/22 at 18:12:07, dcheng wrote:
> > > Would it be slightly more robust to put this check into DocumentLoader?
> > > 
> > > Actually, I wonder if it makes sense to move this tracking into
DocumentLoader completely: then we don't need to make sure all the entry points
to stopLoading() and detachDocumentLoader() remember to check this bool: the
DocumentLoader itself can just make it a no-op and return immediately. WDYT?
> > 
> > I'm not sure. My first attempt is giving me crashes deep in
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld (service
worker related). I suspect there are edge cases where we want to detach the
provisional document loader. We just don't want to do it in |stopAllLoaders|.
> 
> Hmm, mind posting a stack on here? I'm kind of curious what it looks like.

Received signal 11 SEGV_MAPERR 000000000068
#0 0x7f489927c9de base::debug::StackTrace::StackTrace()
#1 0x7f489927c51f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f488d1b0330 <unknown>
#3 0x7f488d1aa404 __GI___pthread_mutex_lock
#4 0x7f48993c28db base::internal::LockImpl::Lock()
#5 0x7f489925c653 base::Lock::Acquire()
#6 0x7f489925c353 base::AutoLock::AutoLock()
#7 0x7f48993fd284 base::ThreadCheckerImpl::EnsureThreadIdAssigned()
#8 0x7f48993fd309 base::ThreadCheckerImpl::CalledOnValidThread()
#9 0x7f48993bce4d base::SupportsUserData::GetUserData()
#10 0x7f489a1a7c6c content::ServiceWorkerNetworkProvider::FromDocumentState()
#11 0x7f489bccd2a9 content::RenderFrameImpl::createServiceWorkerProvider()
#12 0x7f4893ef592b blink::FrameLoaderClientImpl::createServiceWorkerProvider()
#13 0x7f48870031f0 blink::ServiceWorkerContainerClient::from()
#14 0x7f4886ffeb2c blink::ServiceWorkerContainer::ServiceWorkerContainer()
#15 0x7f4886ffcabb blink::ServiceWorkerContainer::create()
#16 0x7f4886ff5b24 blink::NavigatorServiceWorker::serviceWorker()
#17 0x7f4886ff5859 blink::NavigatorServiceWorker::from()
#18 0x7f4886ff570b blink::NavigatorServiceWorker::from()
#19 0x7f4893ef10c2
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
#20 0x7f4888f09a81 blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
#21 0x7f4887f96026 blink::ScriptController::windowProxy()
#22 0x7f4887f95f80 blink::ScriptController::initializeMainWorld()
#23 0x7f4887f9737f blink::ScriptController::updateDocument()
#24 0x7f4888d67da7 blink::LocalDOMWindow::installNewDocument()
#25 0x7f4888ee9338 blink::DocumentLoader::createWriterFor()
#26 0x7f4888ee90af blink::DocumentLoader::ensureWriter()
#27 0x7f4888ee7b60 blink::DocumentLoader::commitData()
#28 0x7f4888ee97e0 blink::DocumentLoader::processData()
#29 0x7f4888ee967f blink::DocumentLoader::dataReceived()
#30 0x7f4888cd804e blink::RawResource::appendData()
#31 0x7f4888d016eb blink::ResourceLoader::didReceiveData()
#32 0x7f489a1cf188 content::WebURLLoaderImpl::Context::OnReceivedData()
#33 0x7f489a1cfb43 content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
#34 0x7f489a165dce content::ResourceDispatcher::OnReceivedData()

I'll upload this as another draft CL so you can take a look at the approach.

[Charlie Harrison, 2016-02-25 00:41:27]

On 2016/02/25 at 00:41:09, csharrison wrote:
> On 2016/02/24 at 21:22:59, dcheng wrote:
> >
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):
> > 
> >
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if
(!m_protectProvisionalLoader)
> > On 2016/02/24 at 17:09:15, csharrison wrote:
> > > On 2016/02/22 at 18:12:07, dcheng wrote:
> > > > Would it be slightly more robust to put this check into DocumentLoader?
> > > > 
> > > > Actually, I wonder if it makes sense to move this tracking into
DocumentLoader completely: then we don't need to make sure all the entry points
to stopLoading() and detachDocumentLoader() remember to check this bool: the
DocumentLoader itself can just make it a no-op and return immediately. WDYT?
> > > 
> > > I'm not sure. My first attempt is giving me crashes deep in
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld (service
worker related). I suspect there are edge cases where we want to detach the
provisional document loader. We just don't want to do it in |stopAllLoaders|.
> > 
> > Hmm, mind posting a stack on here? I'm kind of curious what it looks like.
> 
> Received signal 11 SEGV_MAPERR 000000000068
> #0 0x7f489927c9de base::debug::StackTrace::StackTrace()
> #1 0x7f489927c51f base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7f488d1b0330 <unknown>
> #3 0x7f488d1aa404 __GI___pthread_mutex_lock
> #4 0x7f48993c28db base::internal::LockImpl::Lock()
> #5 0x7f489925c653 base::Lock::Acquire()
> #6 0x7f489925c353 base::AutoLock::AutoLock()
> #7 0x7f48993fd284 base::ThreadCheckerImpl::EnsureThreadIdAssigned()
> #8 0x7f48993fd309 base::ThreadCheckerImpl::CalledOnValidThread()
> #9 0x7f48993bce4d base::SupportsUserData::GetUserData()
> #10 0x7f489a1a7c6c content::ServiceWorkerNetworkProvider::FromDocumentState()
> #11 0x7f489bccd2a9 content::RenderFrameImpl::createServiceWorkerProvider()
> #12 0x7f4893ef592b blink::FrameLoaderClientImpl::createServiceWorkerProvider()
> #13 0x7f48870031f0 blink::ServiceWorkerContainerClient::from()
> #14 0x7f4886ffeb2c blink::ServiceWorkerContainer::ServiceWorkerContainer()
> #15 0x7f4886ffcabb blink::ServiceWorkerContainer::create()
> #16 0x7f4886ff5b24 blink::NavigatorServiceWorker::serviceWorker()
> #17 0x7f4886ff5859 blink::NavigatorServiceWorker::from()
> #18 0x7f4886ff570b blink::NavigatorServiceWorker::from()
> #19 0x7f4893ef10c2
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> #20 0x7f4888f09a81
blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
> #21 0x7f4887f96026 blink::ScriptController::windowProxy()
> #22 0x7f4887f95f80 blink::ScriptController::initializeMainWorld()
> #23 0x7f4887f9737f blink::ScriptController::updateDocument()
> #24 0x7f4888d67da7 blink::LocalDOMWindow::installNewDocument()
> #25 0x7f4888ee9338 blink::DocumentLoader::createWriterFor()
> #26 0x7f4888ee90af blink::DocumentLoader::ensureWriter()
> #27 0x7f4888ee7b60 blink::DocumentLoader::commitData()
> #28 0x7f4888ee97e0 blink::DocumentLoader::processData()
> #29 0x7f4888ee967f blink::DocumentLoader::dataReceived()
> #30 0x7f4888cd804e blink::RawResource::appendData()
> #31 0x7f4888d016eb blink::ResourceLoader::didReceiveData()
> #32 0x7f489a1cf188 content::WebURLLoaderImpl::Context::OnReceivedData()
> #33 0x7f489a1cfb43
content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
> #34 0x7f489a165dce content::ResourceDispatcher::OnReceivedData()
> 
> I'll upload this as another draft CL so you can take a look at the approach.

BTW this is using the new layout test.

[Charlie Harrison, 2016-02-25 00:45:29]

On 2016/02/25 at 00:41:27, csharrison wrote:
> On 2016/02/25 at 00:41:09, csharrison wrote:
> > On 2016/02/24 at 21:22:59, dcheng wrote:
> > >
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):
> > > 
> > >
https://codereview.chromium.org/1714063002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/loader/FrameLoader.cpp:1007: if
(!m_protectProvisionalLoader)
> > > On 2016/02/24 at 17:09:15, csharrison wrote:
> > > > On 2016/02/22 at 18:12:07, dcheng wrote:
> > > > > Would it be slightly more robust to put this check into
DocumentLoader?
> > > > > 
> > > > > Actually, I wonder if it makes sense to move this tracking into
DocumentLoader completely: then we don't need to make sure all the entry points
to stopLoading() and detachDocumentLoader() remember to check this bool: the
DocumentLoader itself can just make it a no-op and return immediately. WDYT?
> > > > 
> > > > I'm not sure. My first attempt is giving me crashes deep in
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld (service
worker related). I suspect there are edge cases where we want to detach the
provisional document loader. We just don't want to do it in |stopAllLoaders|.
> > > 
> > > Hmm, mind posting a stack on here? I'm kind of curious what it looks like.
> > 
> > Received signal 11 SEGV_MAPERR 000000000068
> > #0 0x7f489927c9de base::debug::StackTrace::StackTrace()
> > #1 0x7f489927c51f base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #2 0x7f488d1b0330 <unknown>
> > #3 0x7f488d1aa404 __GI___pthread_mutex_lock
> > #4 0x7f48993c28db base::internal::LockImpl::Lock()
> > #5 0x7f489925c653 base::Lock::Acquire()
> > #6 0x7f489925c353 base::AutoLock::AutoLock()
> > #7 0x7f48993fd284 base::ThreadCheckerImpl::EnsureThreadIdAssigned()
> > #8 0x7f48993fd309 base::ThreadCheckerImpl::CalledOnValidThread()
> > #9 0x7f48993bce4d base::SupportsUserData::GetUserData()
> > #10 0x7f489a1a7c6c
content::ServiceWorkerNetworkProvider::FromDocumentState()
> > #11 0x7f489bccd2a9 content::RenderFrameImpl::createServiceWorkerProvider()
> > #12 0x7f4893ef592b
blink::FrameLoaderClientImpl::createServiceWorkerProvider()
> > #13 0x7f48870031f0 blink::ServiceWorkerContainerClient::from()
> > #14 0x7f4886ffeb2c blink::ServiceWorkerContainer::ServiceWorkerContainer()
> > #15 0x7f4886ffcabb blink::ServiceWorkerContainer::create()
> > #16 0x7f4886ff5b24 blink::NavigatorServiceWorker::serviceWorker()
> > #17 0x7f4886ff5859 blink::NavigatorServiceWorker::from()
> > #18 0x7f4886ff570b blink::NavigatorServiceWorker::from()
> > #19 0x7f4893ef10c2
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> > #20 0x7f4888f09a81
blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
> > #21 0x7f4887f96026 blink::ScriptController::windowProxy()
> > #22 0x7f4887f95f80 blink::ScriptController::initializeMainWorld()
> > #23 0x7f4887f9737f blink::ScriptController::updateDocument()
> > #24 0x7f4888d67da7 blink::LocalDOMWindow::installNewDocument()
> > #25 0x7f4888ee9338 blink::DocumentLoader::createWriterFor()
> > #26 0x7f4888ee90af blink::DocumentLoader::ensureWriter()
> > #27 0x7f4888ee7b60 blink::DocumentLoader::commitData()
> > #28 0x7f4888ee97e0 blink::DocumentLoader::processData()
> > #29 0x7f4888ee967f blink::DocumentLoader::dataReceived()
> > #30 0x7f4888cd804e blink::RawResource::appendData()
> > #31 0x7f4888d016eb blink::ResourceLoader::didReceiveData()
> > #32 0x7f489a1cf188 content::WebURLLoaderImpl::Context::OnReceivedData()
> > #33 0x7f489a1cfb43
content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
> > #34 0x7f489a165dce content::ResourceDispatcher::OnReceivedData()
> > 
> > I'll upload this as another draft CL so you can take a look at the approach.
> 
> BTW this is using the new layout test.

https://codereview.chromium.org/1733093002 here is the draft CL where you can
see what I did. I wouldn't be surprised if other tests fail too.

[dcheng, 2016-02-25 18:40:54]

lgtm

[Charlie Harrison, 2016-02-25 21:09:03]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-02-25 21:14:04]

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

[commit-bot: I haz the power, 2016-02-25 22:14:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-25 22:14:31]

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_androi...)

[Charlie Harrison, 2016-02-26 15:14:17]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-02-26 15:14:53]

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

[commit-bot: I haz the power, 2016-02-26 16:41:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-26 16:43:20]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-02-26 16:43:22]

Patchset 4 (id:??) landed as
https://crrev.com/1d95c6320dce15e12545ac8856ab9f0e77aa12ed
Cr-Commit-Position: refs/heads/master@{#377900}