Bug 183577

Summary:

Resource Load Statistics: Immediately forward cookie access at user interaction when there's an opener document

Product:

WebKit

Reporter:

John Wilander <wilander>

Component:

WebKit Misc.

Assignee:

John Wilander <wilander>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, japhet, kangil.han, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=183578
https://bugs.webkit.org/show_bug.cgi?id=183620

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch for landing	none

John Wilander

Reported 2018-03-12 13:41:47 PDT

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.

Attachments
Patch (26.54 KB, patch) 2018-03-12 14:19 PDT, John Wilander	no flags	Details Formatted Diff Diff
Patch (40.38 KB, patch) 2018-03-12 16:04 PDT, John Wilander	no flags	Details Formatted Diff Diff
Patch (40.43 KB, patch) 2018-03-12 16:24 PDT, John Wilander	no flags	Details Formatted Diff Diff
Patch for landing (40.80 KB, patch) 2018-03-12 18:03 PDT, John Wilander	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

John Wilander

Comment 1 2018-03-12 13:42:11 PDT

<rdar://problem/38266987>

John Wilander

Comment 2 2018-03-12 14:19:45 PDT

Created attachment 335639 [details] Patch

Brent Fulgham

Comment 3 2018-03-12 14:50:03 PDT

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

Brent Fulgham

Comment 4 2018-03-12 14:52:30 PDT

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

John Wilander

Comment 5 2018-03-12 16:03:59 PDT

:) I was in the Source/WebKit directory when I uploaded. Sorry about that. New patch coming up.

John Wilander

Comment 6 2018-03-12 16:04:40 PDT

Created attachment 335659 [details] Patch

John Wilander

Comment 7 2018-03-12 16:23:40 PDT

I'll put the opener check behind #if HAVE(CFNETWORK_STORAGE_PARTITIONING) to fix the GTK issue.

John Wilander

Comment 8 2018-03-12 16:24:43 PDT

Created attachment 335661 [details] Patch

Brent Fulgham

Comment 9 2018-03-12 16:45:35 PDT

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.

John Wilander

Comment 10 2018-03-12 18:03:40 PDT

Created attachment 335669 [details] Patch for landing

John Wilander

Comment 11 2018-03-12 18:04:49 PDT

(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!

WebKit Commit Bot

Comment 12 2018-03-12 19:06:01 PDT

Comment on attachment 335669 [details] Patch for landing Clearing flags on attachment: 335669 Committed r229569: <https://trac.webkit.org/changeset/229569>

WebKit Commit Bot

Comment 13 2018-03-12 19:06:03 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.