Bug 230778 – PCM: Take app bundle ID into consideration when matching triggering events with pending clicks

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

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().

[Radar WebKit Bug Importer]

<rdar://problem/83516528>

[John Wilander]

Created attachment 439544 [details]
Patch

[Alex Christensen]

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]

(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]

(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]

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]

Created attachment 439563 [details]
Patch

[John Wilander]

Created attachment 439564 [details]
Patch

[Alex Christensen]

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]

Created attachment 439655 [details]
Patch

[John Wilander]

(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]

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]

(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]

(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]

(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]

Created attachment 439688 [details]
Patch

[John Wilander]

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 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]

(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]

Created attachment 439695 [details]
Patch

[Alex Christensen]

Created attachment 439696 [details]
Patch

[EWS]

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].