https://commits.webkit.org/r282884 added app bundle IDs to unattributed and attributed clicks in PCM. We need to take those IDs into consideration in PCM::Database::attributePrivateClickMeasurement().
<rdar://problem/83516528>
Created attachment 439544 [details] Patch
Comment on attachment 439544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439544&action=review > Source/WebCore/loader/PrivateClickMeasurement.cpp:152 > + RELEASE_ASSERT(WebCore::applicationBundleIdentifier() == "com.apple.WebKit.WebKitTestRunner"_s); iOS has a different bundle ID, "org.webkit.WebKitTestRunnerApp" > Source/WebCore/loader/PrivateClickMeasurement.h:424 > + std::optional<String> m_sourceApplicationBundleIDForTesting; I don't think this needs to be stored separately on the PCM. setSourceApplicationBundleIDForTesting could just set m_sourceApplicationBundleID. > Source/WebKit/NetworkProcess/NetworkSession.h:217 > + std::optional<String> m_privateClickMeasurementAppBundleIDForTesting; This should probably be stored on the PrivateClickMeasurementManager along with all the other testing state.
(In reply to Alex Christensen from comment #3) > Comment on attachment 439544 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=439544&action=review > > > Source/WebCore/loader/PrivateClickMeasurement.cpp:152 > > + RELEASE_ASSERT(WebCore::applicationBundleIdentifier() == "com.apple.WebKit.WebKitTestRunner"_s); > > iOS has a different bundle ID, "org.webkit.WebKitTestRunnerApp" Will fix. > > Source/WebCore/loader/PrivateClickMeasurement.h:424 > > + std::optional<String> m_sourceApplicationBundleIDForTesting; > > I don't think this needs to be stored separately on the PCM. > setSourceApplicationBundleIDForTesting could just set > m_sourceApplicationBundleID. I remember thinking about that and opted for what's in the patch but I can't recall what it was. Will change and hopefully that just works. > > Source/WebKit/NetworkProcess/NetworkSession.h:217 > > + std::optional<String> m_privateClickMeasurementAppBundleIDForTesting; > > This should probably be stored on the PrivateClickMeasurementManager along > with all the other testing state. I discussed with Kate and she said PrivateClickMeasurementManager will be part of the daemon and not part of the app that's using WebKit which would break this functionality. That's why I moved it back to NetworkSession. Are you saying PrivateClickMeasurementManager is guaranteed to always be part of the app? Thanks for the review!
(In reply to John Wilander from comment #4) > (In reply to Alex Christensen from comment #3) > > Comment on attachment 439544 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=439544&action=review > > > > > Source/WebCore/loader/PrivateClickMeasurement.cpp:152 > > > + RELEASE_ASSERT(WebCore::applicationBundleIdentifier() == "com.apple.WebKit.WebKitTestRunner"_s); > > > > iOS has a different bundle ID, "org.webkit.WebKitTestRunnerApp" > > Will fix. > > > > Source/WebCore/loader/PrivateClickMeasurement.h:424 > > > + std::optional<String> m_sourceApplicationBundleIDForTesting; > > > > I don't think this needs to be stored separately on the PCM. > > setSourceApplicationBundleIDForTesting could just set > > m_sourceApplicationBundleID. > > I remember thinking about that and opted for what's in the patch but I can't > recall what it was. Will change and hopefully that just works. Aaand now I remember why. It's to be able to reset it back to its original app bundle ID. Otherwise I'd have to have a variable to store the old one instead. But I guess I could just set it to WebCore::applicationBundleIdentifier(). > > > Source/WebKit/NetworkProcess/NetworkSession.h:217 > > > + std::optional<String> m_privateClickMeasurementAppBundleIDForTesting; > > > > This should probably be stored on the PrivateClickMeasurementManager along > > with all the other testing state. > > I discussed with Kate and she said PrivateClickMeasurementManager will be > part of the daemon and not part of the app that's using WebKit which would > break this functionality. That's why I moved it back to NetworkSession. Are > you saying PrivateClickMeasurementManager is guaranteed to always be part of > the app? > > Thanks for the review!
PrivateClickMeasurementManager is in the daemon, but the ability to set an override bundle ID for testing is one of many test-only functions that will only be available when running tests. I think it will be fine. Yeah, reset it to WebCore::applicationBundleIdentifier()
Created attachment 439563 [details] Patch
Created attachment 439564 [details] Patch
Comment on attachment 439564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439564&action=review Seems good. API test needs fixing. > Source/WebCore/loader/PrivateClickMeasurement.cpp:152 > + RELEASE_ASSERT(WebCore::applicationBundleIdentifier() == "com.apple.WebKit.WebKitTestRunner"_s || WebCore::applicationBundleIdentifier() == "org.webkit.WebKitTestRunnerApp"_s); nit: we could only check the first one if PLATFORM(MAC) and the second one elif PLATFORM(COCOA) Also, I think we should move this check to NetworkSession, which is the place that won't move to the daemon. > Source/WebKit/NetworkProcess/NetworkSession.cpp:328 > + privateClickMeasurement().handleAttribution(WTFMove(attributionTriggerData), requestURL, RegistrableDomain(redirectRequest.url()), redirectRequest.firstPartyForCookies(), WebCore::applicationBundleIdentifier()); nit: we could reduce some redundancy by doing something like this: #if PLATFORM(COCOA) auto bundleID = WebCore::applicationBundleIdentifier(); #else auto bundleID = emptyString(); #endif > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementManager.cpp:265 > + RELEASE_ASSERT(WebCore::applicationBundleIdentifier() == "com.apple.WebKit.WebKitTestRunner"_s || WebCore::applicationBundleIdentifier() == "org.webkit.WebKitTestRunnerApp"_s); This is currently redundant. One check in NetworkSession ought to do.
Created attachment 439655 [details] Patch
(In reply to Alex Christensen from comment #9) > Comment on attachment 439564 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=439564&action=review > > Seems good. API test needs fixing. > > > Source/WebCore/loader/PrivateClickMeasurement.cpp:152 > > + RELEASE_ASSERT(WebCore::applicationBundleIdentifier() == "com.apple.WebKit.WebKitTestRunner"_s || WebCore::applicationBundleIdentifier() == "org.webkit.WebKitTestRunnerApp"_s); > > nit: we could only check the first one if PLATFORM(MAC) and the second one > elif PLATFORM(COCOA) > Also, I think we should move this check to NetworkSession, which is the > place that won't move to the daemon. > > > Source/WebKit/NetworkProcess/NetworkSession.cpp:328 > > + privateClickMeasurement().handleAttribution(WTFMove(attributionTriggerData), requestURL, RegistrableDomain(redirectRequest.url()), redirectRequest.firstPartyForCookies(), WebCore::applicationBundleIdentifier()); > > nit: we could reduce some redundancy by doing something like this: > > #if PLATFORM(COCOA) > auto bundleID = WebCore::applicationBundleIdentifier(); > #else > auto bundleID = emptyString(); > #endif > > > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementManager.cpp:265 > > + RELEASE_ASSERT(WebCore::applicationBundleIdentifier() == "com.apple.WebKit.WebKitTestRunner"_s || WebCore::applicationBundleIdentifier() == "org.webkit.WebKitTestRunnerApp"_s); > > This is currently redundant. One check in NetworkSession ought to do. I changed the release asserts to the Mac vs Cocoa thing you wanted. However, I kept the check in WebCore::PrivateClickMeasurement since the two places protect against two different kinds of abuse. One is about making a *click* count for another application. The other is for a *conversion* to count for another application. You could of course have cases where both are done at the same time but they matter isolated too.
The WPE build failure looks unrelated and was caused by https://commits.webkit.org/r283029. Sihui is fixing it in https://bugs.webkit.org/show_bug.cgi?id=230989. The Win layout test failures are unrelated.
(In reply to John Wilander from comment #12) > The WPE build failure looks unrelated and was caused by > https://commits.webkit.org/r283029. Sihui is fixing it in > https://bugs.webkit.org/show_bug.cgi?id=230989. > > The Win layout test failures are unrelated. Looks related to me: Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified-sources/UnifiedSource-72468c22-2.cpp.o:UnifiedSource-72468c22-2.cpp:function WebKit::NetworkProcess::setPrivateClickMeasurementAppBundleIDForTesting(PAL::SessionID, WTF::String&&, WTF::CompletionHandler<void ()>&&): error: undefined reference to 'WebKit::NetworkSession::setPrivateClickMeasurementAppBundleIDForTesting(WTF::String&&)' collect2: error: ld returned 1 exit status The other one is just a warning.
(In reply to Chris Dumez from comment #13) > (In reply to John Wilander from comment #12) > > The WPE build failure looks unrelated and was caused by > > https://commits.webkit.org/r283029. Sihui is fixing it in > > https://bugs.webkit.org/show_bug.cgi?id=230989. > > > > The Win layout test failures are unrelated. > > Looks related to me: > Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified- > sources/UnifiedSource-72468c22-2.cpp.o:UnifiedSource-72468c22-2.cpp:function > WebKit::NetworkProcess::setPrivateClickMeasurementAppBundleIDForTesting(PAL:: > SessionID, WTF::String&&, WTF::CompletionHandler<void ()>&&): error: > undefined reference to > 'WebKit::NetworkSession::setPrivateClickMeasurementAppBundleIDForTesting(WTF: > :String&&)' > collect2: error: ld returned 1 exit status > > The other one is just a warning. That said, given that this is the only bot, maybe it is just an incremental build issue?
(In reply to Chris Dumez from comment #14) > (In reply to Chris Dumez from comment #13) > > (In reply to John Wilander from comment #12) > > > The WPE build failure looks unrelated and was caused by > > > https://commits.webkit.org/r283029. Sihui is fixing it in > > > https://bugs.webkit.org/show_bug.cgi?id=230989. > > > > > > The Win layout test failures are unrelated. > > > > Looks related to me: > > Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified- > > sources/UnifiedSource-72468c22-2.cpp.o:UnifiedSource-72468c22-2.cpp:function > > WebKit::NetworkProcess::setPrivateClickMeasurementAppBundleIDForTesting(PAL:: > > SessionID, WTF::String&&, WTF::CompletionHandler<void ()>&&): error: > > undefined reference to > > 'WebKit::NetworkSession::setPrivateClickMeasurementAppBundleIDForTesting(WTF: > > :String&&)' > > collect2: error: ld returned 1 exit status > > > > The other one is just a warning. Oh. > That said, given that this is the only bot, maybe it is just an incremental > build issue? I did look at that part too and couldn't figure out why it was complaining. Can such a thing happen due to incremental build?
Created attachment 439688 [details] Patch
Comment on attachment 439688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439688&action=review > Source/WebCore/loader/PrivateClickMeasurement.cpp:149 > + m_sourceApplicationBundleID = appBundleIDForTesting; It needs to check that it's running a test here too. Otherwise it's possible to store a click for arbitrary bundle IDs.
Comment on attachment 439688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439688&action=review >> Source/WebCore/loader/PrivateClickMeasurement.cpp:149 >> + m_sourceApplicationBundleID = appBundleIDForTesting; > > It needs to check that it's running a test here too. Otherwise it's possible to store a click for arbitrary bundle IDs. This is only called if the manager's m_privateClickMeasurementAppBundleIDForTesting has been set, which can only be set through NetworkSession::setPrivateClickMeasurementAppBundleIDForTesting.
(In reply to Alex Christensen from comment #18) > Comment on attachment 439688 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=439688&action=review > > >> Source/WebCore/loader/PrivateClickMeasurement.cpp:149 > >> + m_sourceApplicationBundleID = appBundleIDForTesting; > > > > It needs to check that it's running a test here too. Otherwise it's possible to store a click for arbitrary bundle IDs. > > This is only called if the manager's > m_privateClickMeasurementAppBundleIDForTesting has been set, which can only > be set through > NetworkSession::setPrivateClickMeasurementAppBundleIDForTesting. In our current benign, mistake-free code, yes. I want to make sure no code, good or bad, can call that setter unless it's a test client.
Created attachment 439695 [details] Patch
Created attachment 439696 [details] Patch
Committed r283316 (242341@main): <https://commits.webkit.org/242341@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439696 [details].