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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

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.

[John Wilander]

<rdar://problem/38266987>

[John Wilander]

Created attachment 335639 [details]
Patch

[Brent Fulgham]

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

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

[John Wilander]

Created attachment 335659 [details]
Patch

[John Wilander]

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

[John Wilander]

Created attachment 335661 [details]
Patch

[Brent Fulgham]

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]

Created attachment 335669 [details]
Patch for landing

[John Wilander]

(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 on attachment 335669 [details]
Patch for landing

Clearing flags on attachment: 335669

Committed r229569: <https://trac.webkit.org/changeset/229569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.