Summary: | Nullptr crash in Page::sessionID() via WebKit::WebFrameLoaderClient::detachedFromParent2() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | Page Loading | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, beidson, bfulgham, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, ggaren, japhet, kangil.han, wilander | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2019-09-09 18:56:00 PDT
Created attachment 378431 [details]
Fixes the crash
Created attachment 378432 [details]
Fixed GTK+ build
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? 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 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
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... 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 I don't think fast/workers/worker-cloneport.html test failure on Windows is related to code change. (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. 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. (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); }); } 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. (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. Created attachment 378502 [details]
Patch
Comment on attachment 378502 [details] Patch Clearing flags on attachment: 378502 Committed r249748: <https://trac.webkit.org/changeset/249748> All reviewed patches have been landed. Closing bug. |