Bug 201625 – Nullptr crash in Page::sessionID() via WebKit::WebFrameLoaderClient::detachedFromParent2()

• Component: Page Loading
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

0   com.apple.WebCore             	0x00007fff4cba0154 WebCore::Page::sessionID() const + 4
1   com.apple.WebKit              	0x00007fff4d7cb8a2 WebKit::WebFrameLoaderClient::detachedFromParent2() + 102
2   com.apple.WebCore             	0x00007fff4b5c3f57 WebCore::CachedFrame::destroy() + 87
3   com.apple.WebCore             	0x00007fff4b5c400d WebCore::CachedFrame::destroy() + 269
4   com.apple.WebCore             	0x00007fff4c854a71 WebCore::PageCache::removeAllItemsForPage(WebCore::Page&) + 129
5   com.apple.WebCore             	0x00007fff4b4e384b WebCore::Page::~Page() + 507
6   com.apple.WebKit              	0x00007fff4dc8ec5d std::__1::unique_ptr<WebCore::Page, std::__1::default_delete<WebCore::Page> >::reset(WebCore::Page*) + 25
7   com.apple.WebKit              	0x00007fff4dc8ebd4 WebKit::DeferredPageDestructor::tryDestruction() + 110
8   com.apple.WebKit              	0x00007fff4d7d0f43 WebKit::WebPage::close() + 1369
9   com.apple.WebKit              	0x00007fff4d836872 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 114
10  com.apple.WebKit              	0x00007fff4dba7f32 WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 28
11  com.apple.WebKit              	0x00007fff4d81fc81 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 229
12  com.apple.WebKit              	0x00007fff4d8265cc WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, 

<rdar://problem/55160615>

[Ryosuke Niwa]

Created attachment 378431 [details]
Fixes the crash

[Ryosuke Niwa]

Created attachment 378432 [details]
Fixed GTK+ build

[Chris Dumez]

Comment on attachment 378432 [details]
Fixed GTK+ build

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:154
> +    m_frameSpecificStorageAccessIdentifier = FrameSpecificStorageAccessIdentifier { sessionID(), frameID().value(), pageID().value() };

What guarantees that frameID() & pageID() do not return WTF::null opt here?

[EWS Watchlist]

Comment on attachment 378432 [details]
Fixed GTK+ build

Attachment 378432 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13018713

New failing tests:
fast/workers/worker-cloneport.html

[EWS Watchlist]

Created attachment 378441 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ryosuke Niwa]

Comment on attachment 378432 [details]
Fixed GTK+ build

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

>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:154
>> +    m_frameSpecificStorageAccessIdentifier = FrameSpecificStorageAccessIdentifier { sessionID(), frameID().value(), pageID().value() };
> 
> What guarantees that frameID() & pageID() do not return WTF::null opt here?

setHasFrameSpecificStorageAccess is only called in DocumentStorageAccess::requestStorageAccess.
I don't think we can have no frame or no page while executing that function since it calls requestStorageAccess with frame ID, page ID, etc...

[Ryosuke Niwa]

Hm... looks like the API test failure is present on trunk:

https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/6266/steps/run-api-tests/logs/stdio

[Ryosuke Niwa]

I don't think fast/workers/worker-cloneport.html test failure on Windows is related to code change.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 378432 [details]
> Fixed GTK+ build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378432&action=review
> 
> >> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:154
> >> +    m_frameSpecificStorageAccessIdentifier = FrameSpecificStorageAccessIdentifier { sessionID(), frameID().value(), pageID().value() };
> > 
> > What guarantees that frameID() & pageID() do not return WTF::null opt here?
> 
> setHasFrameSpecificStorageAccess is only called in
> DocumentStorageAccess::requestStorageAccess.
> I don't think we can have no frame or no page while executing that function
> since it calls requestStorageAccess with frame ID, page ID, etc...

It is called from a lambda. We may have had a pageID / frameID when calling requestStorage was called but I am not sure it means we still have those once the request has been granted.

[Ryosuke Niwa]

Comment on attachment 378432 [details]
Fixed GTK+ build

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

>>>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:154
>>>> +    m_frameSpecificStorageAccessIdentifier = FrameSpecificStorageAccessIdentifier { sessionID(), frameID().value(), pageID().value() };
>>> 
>>> What guarantees that frameID() & pageID() do not return WTF::null opt here?
>> 
>> setHasFrameSpecificStorageAccess is only called in DocumentStorageAccess::requestStorageAccess.
>> I don't think we can have no frame or no page while executing that function since it calls requestStorageAccess with frame ID, page ID, etc...
> 
> It is called from a lambda. We may have had a pageID / frameID when calling requestStorage was called but I am not sure it means we still have those once the request has been granted.

Oh that's a good point. Do you have any suggestion as to what we wanna do?
I guess we'd have to store session ID, frame ID & pageID before we invoke the request...
It's gonna be ugly to merge into the branch but it's the safest approach as far I can tell
because not setting m_frameSpecificStorageAccessIdentifier here would also lead to leaks.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 378432 [details]
> Fixed GTK+ build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378432&action=review
> 
> >>>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:154
> >>>> +    m_frameSpecificStorageAccessIdentifier = FrameSpecificStorageAccessIdentifier { sessionID(), frameID().value(), pageID().value() };
> >>> 
> >>> What guarantees that frameID() & pageID() do not return WTF::null opt here?
> >> 
> >> setHasFrameSpecificStorageAccess is only called in DocumentStorageAccess::requestStorageAccess.
> >> I don't think we can have no frame or no page while executing that function since it calls requestStorageAccess with frame ID, page ID, etc...
> > 
> > It is called from a lambda. We may have had a pageID / frameID when calling requestStorage was called but I am not sure it means we still have those once the request has been granted.
> 
> Oh that's a good point. Do you have any suggestion as to what we wanna do?
> I guess we'd have to store session ID, frame ID & pageID before we invoke
> the request...
> It's gonna be ugly to merge into the branch but it's the safest approach as
> far I can tell
> because not setting m_frameSpecificStorageAccessIdentifier here would also
> lead to leaks.

My proposal would be to not call setHasFrameSpecificStorageAccess() from WebCore but instead to it from void WebPage::requestStorageAccess(), where we're still at the WebKit2 layer and where we can easily store the parameters used to make the storage request in a lambda. The code would look like:
void WebPage::requestStorageAccess(RegistrableDomain&& subFrameDomain, RegistrableDomain&& topFrameDomain, WebFrame& frame, CompletionHandler<void(WebCore::StorageAccessWasGranted, WebCore::StorageAccessPromptWasShown)>&& completionHandler)
{
    WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithAsyncReply(Messages::NetworkConnectionToWebProcess::RequestStorageAccess(sessionID(), WTFMove(subFrameDomain), WTFMove(topFrameDomain), frame.frameID(), m_identifier, m_webPageProxyIdentifier), [completionHandler = WTFMove(completionHandler), frame = makeRef(frame), sessionID = sessionID(), pageID = m_identifier, frameID = frame.frameID()](StorageAccessWasGranted wasGranted, WebCore::StorageAccessPromptWasShown promptWasShown) mutable {
        if (wasGranted == StorageAccessWasGranted::Yes)
            frame->frameLoaderClient()->setHasFrameSpecificStorageAccess(FrameSpecificStorageAccessIdentifier { sessionID, pageID, frameID });
        completionHandler(wasGranted, promptWasShown);
    });
}

[Alex Christensen]

Comment on attachment 378432 [details]
Fixed GTK+ build

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:139
>  PAL::SessionID WebFrameLoaderClient::sessionID() const

While we're at it, could we make this return an Optional<PAL::SessionID> and not fall back to defaultSessionID if there's no page?  Its callers should cancel whatever they were about to do if there's no session.  Maybe that would be better for a separate patch.

[Chris Dumez]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 378432 [details]
> Fixed GTK+ build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378432&action=review
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:139
> >  PAL::SessionID WebFrameLoaderClient::sessionID() const
> 
> While we're at it, could we make this return an Optional<PAL::SessionID> and
> not fall back to defaultSessionID if there's no page?  Its callers should
> cancel whatever they were about to do if there's no session.  Maybe that
> would be better for a separate patch.

Yes, we talked about that with Geoff yesterday but we should do this in a follow-up to facilitate merging to the branch.

[Chris Dumez]

Created attachment 378502 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378502 [details]
Patch

Clearing flags on attachment: 378502

Committed r249748: <https://trac.webkit.org/changeset/249748>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.