[webkit-reviews] review granted: [Bug 183577] Resource Load Statistics: Immediately forward cookie access at user interaction when there's an opener document : [Attachment 335661] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 183577: Resource Load Statistics: Immediately forward cookie access at user
interaction when there's an opener document
https://bugs.webkit.org/show_bug.cgi?id=183577

Attachment 335661: Patch

https://bugs.webkit.org/attachment.cgi?id=335661&action=review




--- Comment #9 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 335661
  --> https://bugs.webkit.org/attachment.cgi?id=335661
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.