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.
<rdar://problem/56262708>
Created attachment 381046 [details] Patch
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
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?
(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
(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.
Created attachment 381113 [details] Patch
Comment on attachment 381113 [details] Patch Thanks, Alex!
Comment on attachment 381113 [details] Patch Clearing flags on attachment: 381113 Committed r251213: <https://trac.webkit.org/changeset/251213>
All reviewed patches have been landed. Closing bug.