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