RESOLVED FIXED 230778
PCM: Take app bundle ID into consideration when matching triggering events with pending clicks
https://bugs.webkit.org/show_bug.cgi?id=230778
Summary PCM: Take app bundle ID into consideration when matching triggering events wi...
John Wilander
Reported 2021-09-24 16:29:38 PDT
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().
Attachments
Patch (68.97 KB, patch)
2021-09-28 16:49 PDT, John Wilander
achristensen: review-
Patch (73.35 KB, patch)
2021-09-28 21:17 PDT, John Wilander
ews-feeder: commit-queue-
Patch (73.58 KB, patch)
2021-09-28 22:01 PDT, John Wilander
no flags
Patch (78.21 KB, patch)
2021-09-29 14:12 PDT, John Wilander
no flags
Patch (63.24 KB, patch)
2021-09-29 18:23 PDT, Alex Christensen
no flags
Patch (59.91 KB, patch)
2021-09-29 20:38 PDT, Alex Christensen
no flags
Patch (73.07 KB, patch)
2021-09-29 20:43 PDT, Alex Christensen
no flags
Radar WebKit Bug Importer
Comment 1 2021-09-24 16:30:04 PDT
John Wilander
Comment 2 2021-09-28 16:49:03 PDT
Alex Christensen
Comment 3 2021-09-28 17:02:45 PDT
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.
John Wilander
Comment 4 2021-09-28 17:07:25 PDT
(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!
John Wilander
Comment 5 2021-09-28 17:12:30 PDT
(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!
Alex Christensen
Comment 6 2021-09-28 19:31:50 PDT
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()
John Wilander
Comment 7 2021-09-28 21:17:15 PDT
John Wilander
Comment 8 2021-09-28 22:01:45 PDT
Alex Christensen
Comment 9 2021-09-29 09:50:27 PDT
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.
John Wilander
Comment 10 2021-09-29 14:12:42 PDT
John Wilander
Comment 11 2021-09-29 14:14:46 PDT
(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.
John Wilander
Comment 12 2021-09-29 15:22:08 PDT
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.
Chris Dumez
Comment 13 2021-09-29 15:24:00 PDT
(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.
Chris Dumez
Comment 14 2021-09-29 15:25:12 PDT
(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?
John Wilander
Comment 15 2021-09-29 15:27:53 PDT
(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?
Alex Christensen
Comment 16 2021-09-29 18:23:32 PDT
John Wilander
Comment 17 2021-09-29 19:34:32 PDT
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.
Alex Christensen
Comment 18 2021-09-29 19:54:46 PDT
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.
John Wilander
Comment 19 2021-09-29 20:25:57 PDT
(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.
Alex Christensen
Comment 20 2021-09-29 20:38:38 PDT
Alex Christensen
Comment 21 2021-09-29 20:43:24 PDT
EWS
Comment 22 2021-09-30 09:22:11 PDT
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].
Note You need to log in before you can comment on or make changes to this bug.