Bug 203017 – Resource Load Statistics (experimental): Block all third-party cookies on websites without prior user interaction

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should block all third-party cookies on websites without prior user interaction. This enhancement will start as off by default to allow for dedicated testing.

[John Wilander]

<rdar://problem/56262708>

[John Wilander]

Created attachment 381046 [details]
Patch

[Alex Christensen]

Comment on attachment 381046 [details]
Patch

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

Good for the most part.

> Source/WebCore/platform/network/NetworkStorageSession.cpp:144
> +    m_registrableDomainsWithUserInteractionAsFirstParty.clear();
> +    m_registrableDomainsWithUserInteractionAsFirstParty.add(domains.begin(), domains.end());

Would it not be more efficient to only add the newly-interacted-with domains instead of clearing and re-adding all of the interacted-with domains?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1761
> +    SQLiteStatement statement(m_database, "SELECT registrableDomain FROM ObservedDomains WHERE hadUserInteraction = 1"_s);
> +    if (statement.prepare() != SQLITE_OK)

Probably for a different patch:
We probably don't want to prepare statements every time we call these functions.  We could cache prepared statements

> Source/WebKit/NetworkProcess/NetworkSession.cpp:117
> +    m_adClickAttribution->setIsDebugModeEnabledFunction([this, weakThis = makeWeakPtr(this)] () {

This isn't a good pattern to construct an object then call a bunch of setters for callbacks.  I think it would be cleaner to have an AdClickAttributionClient interface that the NetworkSession implements, then pass it in as a constructor parameter.

> Source/WebKit/Shared/WebPreferences.yaml:1771
> +  humanReadableName: "Block 3p Cookies On Sites Without Interaction (ITP)"

I don't think "3p" is human readable.

> Tools/WebKitTestRunner/TestInvocation.cpp:1838
> +    WKPagePostMessageToInjectedBundle(TestController::singleton().mainWebView()->page(), messageName.get(), 0);

nullptr

[John Wilander]

Thanks, Alex!

(In reply to Alex Christensen from comment #3)
> Comment on attachment 381046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381046&action=review
> 
> Good for the most part.
> 
> > Source/WebCore/platform/network/NetworkStorageSession.cpp:144
> > +    m_registrableDomainsWithUserInteractionAsFirstParty.clear();
> > +    m_registrableDomainsWithUserInteractionAsFirstParty.add(domains.begin(), domains.end());
> 
> Would it not be more efficient to only add the newly-interacted-with domains
> instead of clearing and re-adding all of the interacted-with domains?

I assume you mean to move to a set rather than a vector. IIRC, this was optimized for lookup, not changes, since these lookups happen on every request but updates happen when ITP decides to classify or there is user interaction with a new first-party.

What would be the most performant data structure for lookups?

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1761
> > +    SQLiteStatement statement(m_database, "SELECT registrableDomain FROM ObservedDomains WHERE hadUserInteraction = 1"_s);
> > +    if (statement.prepare() != SQLITE_OK)
> 
> Probably for a different patch:
> We probably don't want to prepare statements every time we call these
> functions.  We could cache prepared statements

Got it. This was just copied from the other convenience functions.

> > Source/WebKit/NetworkProcess/NetworkSession.cpp:117
> > +    m_adClickAttribution->setIsDebugModeEnabledFunction([this, weakThis = makeWeakPtr(this)] () {
> 
> This isn't a good pattern to construct an object then call a bunch of
> setters for callbacks.  I think it would be cleaner to have an
> AdClickAttributionClient interface that the NetworkSession implements, then
> pass it in as a constructor parameter.

OK. I'll just drop this change for now then since it was a cleanup attempt and not part of what the patch is to achieve.

> > Source/WebKit/Shared/WebPreferences.yaml:1771
> > +  humanReadableName: "Block 3p Cookies On Sites Without Interaction (ITP)"
> 
> I don't think "3p" is human readable.

OK. This was just me trying to reduce the width of the menu item.

> > Tools/WebKitTestRunner/TestInvocation.cpp:1838
> > +    WKPagePostMessageToInjectedBundle(TestController::singleton().mainWebView()->page(), messageName.get(), 0);
> 
> nullptr

You want me to check for a nullptr? Or use nullptr instead of 0?

[Alex Christensen]

(In reply to John Wilander from comment #4)
> Thanks, Alex!
> 
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 381046 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=381046&action=review
> > 
> > Good for the most part.
> > 
> > > Source/WebCore/platform/network/NetworkStorageSession.cpp:144
> > > +    m_registrableDomainsWithUserInteractionAsFirstParty.clear();
> > > +    m_registrableDomainsWithUserInteractionAsFirstParty.add(domains.begin(), domains.end());
> > 
> > Would it not be more efficient to only add the newly-interacted-with domains
> > instead of clearing and re-adding all of the interacted-with domains?
> 
> I assume you mean to move to a set rather than a vector. IIRC, this was
> optimized for lookup, not changes, since these lookups happen on every
> request but updates happen when ITP decides to classify or there is user
> interaction with a new first-party.
> 
> What would be the most performant data structure for lookups?

What I meant was this: instead of clearing then adding the whole collection, could we just add the new entries?

Relatedly: if we have third party interaction then restart the browser, we won't have a record that that third party had interaction, right?

> You want me to check for a nullptr? Or use nullptr instead of 0?
nullptr instead of 0

[John Wilander]

(In reply to Alex Christensen from comment #5)
> (In reply to John Wilander from comment #4)
> > Thanks, Alex!
> > 
> > (In reply to Alex Christensen from comment #3)
> > > Comment on attachment 381046 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=381046&action=review
> > > 
> > > Good for the most part.
> > > 
> > > > Source/WebCore/platform/network/NetworkStorageSession.cpp:144
> > > > +    m_registrableDomainsWithUserInteractionAsFirstParty.clear();
> > > > +    m_registrableDomainsWithUserInteractionAsFirstParty.add(domains.begin(), domains.end());
> > > 
> > > Would it not be more efficient to only add the newly-interacted-with domains
> > > instead of clearing and re-adding all of the interacted-with domains?
> > 
> > I assume you mean to move to a set rather than a vector. IIRC, this was
> > optimized for lookup, not changes, since these lookups happen on every
> > request but updates happen when ITP decides to classify or there is user
> > interaction with a new first-party.
> > 
> > What would be the most performant data structure for lookups?
> 
> What I meant was this: instead of clearing then adding the whole collection,
> could we just add the new entries?

Oh, we had that functionality before but it requires bookkeeping of what has been transferred and not. Then later we decided that it's too fragile and we should just sync the whole thing when there's an update.

> Relatedly: if we have third party interaction then restart the browser, we
> won't have a record that that third party had interaction, right?

Third-parties can not get user interaction in the eyes of ITP, only first-parties can. But maybe you mean a third-party resource has previously received user interaction as first-party.

Everything ITP learns, including user interaction, is persisted and thus survives a browser relaunch.

> > You want me to check for a nullptr? Or use nullptr instead of 0?
> nullptr instead of 0

Got it.

[John Wilander]

Created attachment 381113 [details]
Patch

[John Wilander]

Comment on attachment 381113 [details]
Patch

Thanks, Alex!

[WebKit Commit Bot]

Comment on attachment 381113 [details]
Patch

Clearing flags on attachment: 381113

Committed r251213: <https://trac.webkit.org/changeset/251213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.