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.
<rdar://problem/57344054>
Created attachment 383969 [details] Patch
Created attachment 384071 [details] Patch
Updated enum naming and default settings to make it more clear.
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?
(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!
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.
Created attachment 384210 [details] Patch
The mac-debug-wk1 test failure is an unrelated assert crash.
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.
(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.
Comment on attachment 384210 [details] Patch Clearing flags on attachment: 384210 Committed r252840: <https://trac.webkit.org/changeset/252840>
All reviewed patches have been landed. Closing bug.