Crash in WebCore::DocumentLoader::stopLoadingForPolicyChange

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

Crash in WebCore::DocumentLoader::stopLoadingForPolicyChange

There's a few different ways into this crash, but the tops of the stacks look like:

Thread 0 Crashed:
0   WebCore                       	0x000000019588607c WebCore::DocumentLoader::stopLoadingForPolicyChange() + 40 (DocumentLoader.cpp:769)
1   WebCore                       	0x00000001958877b4 std::__1::__function::__func<WebCore::DocumentLoader::willSendRequest(WebCore::ResourceRequest&, WebCore::ResourceResponse const&)::$_0, std::__1::allocator<WebCore::DocumentLoader::willSendRequest(WebCore::ResourceRequest&, WebCore::ResourceResponse const&)::$_0>, void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>&&, bool&&) + 40 (DocumentLoader.cpp:564)
2   WebCore                       	0x00000001958877b4 std::__1::__function::__func<WebCore::DocumentLoader::willSendRequest(WebCore::ResourceRequest&, WebCore::ResourceResponse const&)::$_0, std::__1::allocator<WebCore::DocumentLoader::willSendRequest(WebCore::ResourceRequest&, WebCore::ResourceResponse const&)::$_0>, void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>&&, bool&&) + 40 (DocumentLoader.cpp:564)
3   WebCore                       	0x000000019558e5a4 WebCore::PolicyCallback::cancel() + 164 (functional:1793)
4   WebCore                       	0x000000019558e06c WebCore::PolicyChecker::stopCheck() + 84 (PolicyChecker.cpp:161)
5   WebCore                       	0x00000001959a26e8 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*,
...

In stopLoadingForPolicyChange, frameLoader() is null.
This can only happen when m_frame is null.
That can only happen if the DocumentLoader has been detached from its Frame.

No known reproducibility.

We need to make absolutely sure that once the Frame is detached there are no outstanding policy callbacks.

In Radar - <rdar://problem/21412186>

[Brady Eidson]

Created attachment 257765 [details]
Patch v1 - Speculative fix.

[Chris Dumez]

Comment on attachment 257765 [details]
Patch v1 - Speculative fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=257765&action=review

Good safety net, and seems safe besides my comment

> Source/WebCore/loader/DocumentLoader.cpp:954
> +    cancelPolicyCheckIfNeeded();

Based on the comment and the null check below, I am slightly worried we introduce crashes due to dereferencing frameLoader() without check in this method. How about we null check frameLoader() in cancelPolicyCheckIfNeeded() and reset m_waitingForContentPolicy to false no matter what?

[Chris Dumez]

Comment on attachment 257765 [details]
Patch v1 - Speculative fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=257765&action=review

> Source/WebCore/loader/DocumentLoader.cpp:956
>      // Even though we ASSERT at the top of this method that we have an m_frame, we're seeing crashes where m_frame is null.

I was referring to this comment and the m_frame null check right after.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 257765 [details]
> Patch v1 - Speculative fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257765&action=review
> 
> Good safety net, and seems safe besides my comment
> 
> > Source/WebCore/loader/DocumentLoader.cpp:954
> > +    cancelPolicyCheckIfNeeded();
> 
> Based on the comment and the null check below, I am slightly worried we
> introduce crashes due to dereferencing frameLoader() without check in this
> method. How about we null check frameLoader() in cancelPolicyCheckIfNeeded()
> and reset m_waitingForContentPolicy to false no matter what?

I think that's reasonable.

I actually also hope to make the ASSERT(frameLoader()) be a RELEASE_ASSERT in a separate patch so that's the status quo for trunk.

We need to figure this out. :)

(In reply to comment #3)
> Comment on attachment 257765 [details]
> Patch v1 - Speculative fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257765&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:956
> >      // Even though we ASSERT at the top of this method that we have an m_frame, we're seeing crashes where m_frame is null.
> 
> I was referring to this comment and the m_frame null check right after.

Hell, you should've been referring to the remaining comment that has an if (m_frame) check. We'd already known this was happening.

[Brady Eidson]

Created attachment 257766 [details]
Patch v2

[Chris Dumez]

Comment on attachment 257766 [details]
Patch v2

r=me

[Brady Eidson]

https://trac.webkit.org/changeset/187557

[Brady Eidson]

Created attachment 257770 [details]
RELEASE_ASSERT followup

[Chris Dumez]

Comment on attachment 257770 [details]
RELEASE_ASSERT followup

View in context: https://bugs.webkit.org/attachment.cgi?id=257770&action=review

r=me

> Source/WebCore/loader/DocumentLoader.cpp:1479
>      m_waitingForContentPolicy = false;

nit: Now, I'd move this back inside the if scope.

[Brady Eidson]

https://trac.webkit.org/changeset/187558

[Brady Eidson]

(In reply to comment #9)
> Comment on attachment 257770 [details]
> RELEASE_ASSERT followup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257770&action=review
> 
> r=me
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1479
> >      m_waitingForContentPolicy = false;
> 
> nit: Now, I'd move this back inside the if scope.

Whoops.
https://trac.webkit.org/changeset/187559