WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
203017
Resource Load Statistics (experimental): Block all third-party cookies on websites without prior user interaction
https://bugs.webkit.org/show_bug.cgi?id=203017
Summary
Resource Load Statistics (experimental): Block all third-party cookies on web...
John Wilander
Reported
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.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
John Wilander
Comment 1
2019-10-15 17:32:13 PDT
<
rdar://problem/56262708
>
John Wilander
Comment 2
2019-10-15 18:02:18 PDT
Created
attachment 381046
[details]
Patch
Alex Christensen
Comment 3
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
John Wilander
Comment 4
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?
Alex Christensen
Comment 5
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
John Wilander
Comment 6
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.
John Wilander
Comment 7
2019-10-16 14:51:48 PDT
Created
attachment 381113
[details]
Patch
John Wilander
Comment 8
2019-10-16 15:10:54 PDT
Comment on
attachment 381113
[details]
Patch Thanks, Alex!
WebKit Commit Bot
Comment 9
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
>
WebKit Commit Bot
Comment 10
2019-10-16 15:39:08 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug