We should shortcut forwarding of user interaction when there's an opener document instead of capturing the interaction as "statistics." This will fix compatibility issues where popups talk to a cross-origin opener document.
<rdar://problem/38266987>
Created attachment 335639 [details] Patch
/Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:776:73: error: no member named 'hasStorageAccess' in 'WebCore::NetworkStorageSession' bool hasStorageAccess = (frameID && pageID) ? networkStorageSession.hasStorageAccess(url.string(), partition.string(), frameID.value(), pageID.value()) : false; ~~~~~~~~~~~~~~~~~~~~~ ^ 1 error generated.
Comment on attachment 335639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335639&action=review Looks like the patch is missing NetworkStorageSession changes! > Source/WebKit/NetworkProcess/NetworkProcess.cpp:371 > + parentProcessConnection()->send(Messages::NetworkProcessProxy::StorageAccessRequestResult(networkStorageSession->hasStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID), contextId), 0); It looks like you are missing the change to NetworkStorageSession that renames 'hasStorageAccessForFrame' -> 'hasStorageAccess'.
:) I was in the Source/WebKit directory when I uploaded. Sorry about that. New patch coming up.
Created attachment 335659 [details] Patch
I'll put the opener check behind #if HAVE(CFNETWORK_STORAGE_PARTITIONING) to fix the GTK issue.
Created attachment 335661 [details] Patch
Comment on attachment 335661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335661&action=review This looks good, but I'm worried you have a mistake in iterator handling in NetworkStorageSession::grantStorageAccess. Can you please fix that before landing? > Source/WebCore/ChangeLog:12 > + in the popup. I would mention that there are manual tests for this, just to make it clear that this is not untested code. :-) > Source/WebCore/loader/ResourceLoadObserver.cpp:324 > + if (std::optional<uint64_t> openerPageID = openerFrame->loader().client().pageID()) { auto openerPageID = ... > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:319 > + auto it1 = m_pagesGrantedStorageAccess.find(pageID); auto pagesGrantedIterator = ... > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:325 > + auto it2 = it1->value.find(firstPartyDomain); What if it2 is it1->value.end()? Crash! Should you ASSERT here (if it's unthinkable that will happen)? Or, just check we aren't past the end of the container before trying to set the value. > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:337 > + auto it2 = it1->value.find(frameID.value()); What if it2 is it1->value.end()? This will crash! Ditto recommendations above.
Created attachment 335669 [details] Patch for landing
(In reply to Brent Fulgham from comment #9) > Comment on attachment 335661 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335661&action=review > > This looks good, but I'm worried you have a mistake in iterator handling in > NetworkStorageSession::grantStorageAccess. Can you please fix that before > landing? > > > Source/WebCore/ChangeLog:12 > > + in the popup. > > I would mention that there are manual tests for this, just to make it clear > that this is not untested code. :-) Fixed. > > Source/WebCore/loader/ResourceLoadObserver.cpp:324 > > + if (std::optional<uint64_t> openerPageID = openerFrame->loader().client().pageID()) { > > auto openerPageID = ... Fixed. > > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:319 > > + auto it1 = m_pagesGrantedStorageAccess.find(pageID); > > auto pagesGrantedIterator = ... Fixed. > > > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:325 > > + auto it2 = it1->value.find(firstPartyDomain); > > What if it2 is it1->value.end()? Crash! > > Should you ASSERT here (if it's unthinkable that will happen)? Or, just > check we aren't past the end of the container before trying to set the value. Fixed. > > > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:337 > > + auto it2 = it1->value.find(frameID.value()); > > What if it2 is it1->value.end()? This will crash! > > Ditto recommendations above. Fixed. Thanks, Brent!
Comment on attachment 335669 [details] Patch for landing Clearing flags on attachment: 335669 Committed r229569: <https://trac.webkit.org/changeset/229569>
All reviewed patches have been landed. Closing bug.