Bug 203017 - Resource Load Statistics (experimental): Block all third-party cookies on websites without prior user interaction
Summary: Resource Load Statistics (experimental): Block all third-party cookies on web...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-15 17:31 PDT by John Wilander
Modified: 2020-01-18 18:12 PST (History)
5 users (show)

See Also:


Attachments
Patch (71.41 KB, patch)
2019-10-15 18:02 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (66.23 KB, patch)
2019-10-16 14:51 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-10-15 17:31:52 PDT
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.
Comment 1 John Wilander 2019-10-15 17:32:13 PDT
<rdar://problem/56262708>
Comment 2 John Wilander 2019-10-15 18:02:18 PDT
Created attachment 381046 [details]
Patch
Comment 3 Alex Christensen 2019-10-16 12:57:01 PDT
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
Comment 4 John Wilander 2019-10-16 13:06:57 PDT
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?
Comment 5 Alex Christensen 2019-10-16 13:36:53 PDT
(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
Comment 6 John Wilander 2019-10-16 13:40:57 PDT
(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.
Comment 7 John Wilander 2019-10-16 14:51:48 PDT
Created attachment 381113 [details]
Patch
Comment 8 John Wilander 2019-10-16 15:10:54 PDT
Comment on attachment 381113 [details]
Patch

Thanks, Alex!
Comment 9 WebKit Commit Bot 2019-10-16 15:39:05 PDT
Comment on attachment 381113 [details]
Patch

Clearing flags on attachment: 381113

Committed r251213: <https://trac.webkit.org/changeset/251213>
Comment 10 WebKit Commit Bot 2019-10-16 15:39:08 PDT
All reviewed patches have been landed.  Closing bug.