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>
Created attachment 257765 [details] Patch v1 - Speculative fix.
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?
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.
(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.
Created attachment 257766 [details] Patch v2
Comment on attachment 257766 [details] Patch v2 r=me
https://trac.webkit.org/changeset/187557
Created attachment 257770 [details] RELEASE_ASSERT followup
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.
https://trac.webkit.org/changeset/187558
(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