Bug 204389 – Resource Load Statistics: Allow multiple third-party cookie blocking settings

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

https://bugs.webkit.org/show_bug.cgi?id=203017 and https://bugs.webkit.org/show_bug.cgi?id=203195 introduced two different third-party cookie blocking settings. We should support and regression test both.

[Radar WebKit Bug Importer]

<rdar://problem/57344054>

[John Wilander]

Created attachment 383969 [details]
Patch

[John Wilander]

Created attachment 384071 [details]
Patch

[John Wilander]

Updated enum naming and default settings to make it more clear.

[Brent Fulgham]

Comment on attachment 384071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384071&action=review

> Source/WebCore/ChangeLog:11
> +        NetworkStorageSession::shouldBlockCookies().

I think it would be helpful to explain why we need three modes instead of two, otherwise it is difficult to reason about the rest of the patch.

> Source/WebCore/platform/network/NetworkStorageSession.cpp:125
> +    }

Add an "ASSERT_NOT_REACHED" here. We should never move past the switch (unless we someday add another blocking mode and forget to add a case!)

> Source/WebCore/platform/network/NetworkStorageSession.h:205
> +    ThirdPartyCookieBlockingMode m_thirdPartyCookieBlockingMode = ThirdPartyCookieBlockingMode::AllOnSitesWithoutUserInteraction;

Weird that this isn't using normal init syntax (and wasn't before):

ThirdPartyCookieBlockingMode m_thirdPartyCookieBlockingMode { ThirdPartyCookieBlockingMode::AllOnSitesWithoutUserInteraction };

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1829
> +    if (!isPrevalent && thirdPartyCookieBlockingMode() != ThirdPartyCookieBlockingMode::All)

Is this correct? What if the setting is 'AllOnSitesWithoutUserInteraction' and there was no user interaction? It seems like that should return "OnlyIfGranted'?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:217
> +    if (thirdPartyCookieBlockingMode() != ThirdPartyCookieBlockingMode::All && !resourceStatistic.isPrevalentResource)

In the Database path we return early here (with OnlyIfGranted) when we set blocking mode to all. What if blocking mode is "AllOnSitesWithoutUserInteraction" and the user did not interact?

One of the two code paths seem wrong; at the very least they are inconsistent and we are about to switch to the database case full time!

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1791
> +void WebsiteDataStore::setResourceLoadStatisticsShouldBlockThirdPartyCookiesForTesting(bool enabled, bool onlyOnSitesWithoutUserInteraction, CompletionHandler<void()>&& completionHandler)

Why don't we use the ThirdPartyCookieBlockingMode enum as the argument here?

[John Wilander]

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 384071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384071&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        NetworkStorageSession::shouldBlockCookies().
> 
> I think it would be helpful to explain why we need three modes instead of
> two, otherwise it is difficult to reason about the rest of the patch.

OK, I'll explain in the change log entry.

> > Source/WebCore/platform/network/NetworkStorageSession.cpp:125
> > +    }
> 
> Add an "ASSERT_NOT_REACHED" here. We should never move past the switch
> (unless we someday add another blocking mode and forget to add a case!)

We have build checks to make sure we never miss an enum. That's why we don't use a default clause in the end. And since this function requires a return value, we'd also get a build error for code path with no return. But I can add the assertion if you feel it helps.

> > Source/WebCore/platform/network/NetworkStorageSession.h:205
> > +    ThirdPartyCookieBlockingMode m_thirdPartyCookieBlockingMode = ThirdPartyCookieBlockingMode::AllOnSitesWithoutUserInteraction;
> 
> Weird that this isn't using normal init syntax (and wasn't before):

Agreed.

> ThirdPartyCookieBlockingMode m_thirdPartyCookieBlockingMode {
> ThirdPartyCookieBlockingMode::AllOnSitesWithoutUserInteraction };

Will fix.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1829
> > +    if (!isPrevalent && thirdPartyCookieBlockingMode() != ThirdPartyCookieBlockingMode::All)
> 
> Is this correct? What if the setting is 'AllOnSitesWithoutUserInteraction'
> and there was no user interaction? It seems like that should return
> "OnlyIfGranted'?

I'll have a look.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:217
> > +    if (thirdPartyCookieBlockingMode() != ThirdPartyCookieBlockingMode::All && !resourceStatistic.isPrevalentResource)
> 
> In the Database path we return early here (with OnlyIfGranted) when we set
> blocking mode to all. What if blocking mode is
> "AllOnSitesWithoutUserInteraction" and the user did not interact?
> 
> One of the two code paths seem wrong; at the very least they are
> inconsistent and we are about to switch to the database case full time!

Good catch. I'll have a look.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1791
> > +void WebsiteDataStore::setResourceLoadStatisticsShouldBlockThirdPartyCookiesForTesting(bool enabled, bool onlyOnSitesWithoutUserInteraction, CompletionHandler<void()>&& completionHandler)
> 
> Why don't we use the ThirdPartyCookieBlockingMode enum as the argument here?

The function is called by the C API layer and I didn't want to expose the enum in the C API code. We're currently not pulling anything from WebCore into WKWebsiteDataStoreRef.cpp. Should I?

Thanks!

[Brent Fulgham]

Comment on attachment 384071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384071&action=review

>>> Source/WebCore/platform/network/NetworkStorageSession.cpp:125
>>> +    }
>> 
>> Add an "ASSERT_NOT_REACHED" here. We should never move past the switch (unless we someday add another blocking mode and forget to add a case!)
> 
> We have build checks to make sure we never miss an enum. That's why we don't use a default clause in the end. And since this function requires a return value, we'd also get a build error for code path with no return. But I can add the assertion if you feel it helps.

Okay-- leave as is.

>>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1791
>>> +void WebsiteDataStore::setResourceLoadStatisticsShouldBlockThirdPartyCookiesForTesting(bool enabled, bool onlyOnSitesWithoutUserInteraction, CompletionHandler<void()>&& completionHandler)
>> 
>> Why don't we use the ThirdPartyCookieBlockingMode enum as the argument here?
> 
> The function is called by the C API layer and I didn't want to expose the enum in the C API code. We're currently not pulling anything from WebCore into WKWebsiteDataStoreRef.cpp. Should I?
> 
> Thanks!

Nah. That makes sense.

[John Wilander]

Created attachment 384210 [details]
Patch

[John Wilander]

The mac-debug-wk1 test failure is an unrelated assert crash.

[Brent Fulgham]

Comment on attachment 384210 [details]
Patch

Thank you for making the database and memory stores match. Everything else looks fine.

I don’t understand the Mac-debug-wk1 failures, but they do not seem related to your patch. Please keep an eye on the test bits after you land the patch and confirm they are unrelated.

[John Wilander]

(In reply to Brent Fulgham from comment #10)
> Comment on attachment 384210 [details]
> Patch
> 
> Thank you for making the database and memory stores match. Everything else
> looks fine.

Thank you for pointing it out. It was a good enhancement. That logic is not simple.

> I don’t understand the Mac-debug-wk1 failures, but they do not seem related
> to your patch. Please keep an eye on the test bits after you land the patch
> and confirm they are unrelated.

Will do.

[WebKit Commit Bot]

Comment on attachment 384210 [details]
Patch

Clearing flags on attachment: 384210

Committed r252840: <https://trac.webkit.org/changeset/252840>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.