Bug 230576 – PCM should include the bundle ID of the app from which it originated

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

PCM should include the bundle ID of the app from which it originated

[Alex Christensen]

Created attachment 438861 [details]
Patch

[Alex Christensen]

<rdar://problem/83065221>

[Alex Christensen]

Created attachment 438866 [details]
Patch

[Alex Christensen]

Created attachment 438904 [details]
Patch

[Kate Cheney]

(In reply to Alex Christensen from comment #4)
> Created attachment 438904 [details]
> Patch

WebKitTestRunner has different bundleIDs for iOS and macOS, that is causing the test failures.

[Kate Cheney]

Comment on attachment 438904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438904&action=review

> Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:153
> +    constexpr auto mobileSafariBundleID = "com.apple.mobilesafari";

I wonder if we should use com.apple.Safari for macOS.

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:370
> +            attributionToStringForTesting(buildPrivateClickMeasurementFromDatabase(*unattributedScopedStatement.get(), PrivateClickMeasurementAttributionType::Unattributed)));

Good change.

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:406
> +        if (attributionTriggerData != std::numeric_limits<uint32_t>::max()) {

I don't think this check is needed. If attributionTriggerData in the sqlite result is -1, buildPrivateClickMeasurementFromDatabase will not set the attribution value, so triggerData will be nullopt.

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:686
> +        RELEASE_LOG_ERROR(PrivateClickMeasurement, "%p - Database::getColumnsFromTableInfoStatement Unable to prepare statement to fetch schema for table, error message: %" PRIVATE_LOG_STRING, this, m_database.lastErrorMsg());

Nit: Database::columnsForTable instead

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:713
> +    statement->reset();

No need for this reset since this statement will go out of scope before you use it again.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:343
> +            [webView _addEventAttributionWithSourceID:42 destinationURL:exampleURL() sourceDescription:@"test source description" purchaser:@"test purchaser" reportEndpoint:server.request().URL optionalNonce:nil applicationBundleID:@"test.bundle.id"];

What is the point of passing the bundleID here if we never check it for the test?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PrivateClickMeasurement.mm:201
> +        "Application bundle identifier: com.apple.mobilesafari\n";

Seems like this should be com.apple.Safari for macOS versions of this test.

[Alex Christensen]

(In reply to Kate Cheney from comment #6)
> Comment on attachment 438904 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438904&action=review
> 
> > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:153
> > +    constexpr auto mobileSafariBundleID = "com.apple.mobilesafari";
> 
> I wonder if we should use com.apple.Safari for macOS.
I don't think so.  com.apple.mobilesafari is the only application that has ever had this enabled.  It's never been used on Mac.  This is only for legacy data migration purposes.  Maybe I'll rename it to make it more clear that is what is going on.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:343
> > +            [webView _addEventAttributionWithSourceID:42 destinationURL:exampleURL() sourceDescription:@"test source description" purchaser:@"test purchaser" reportEndpoint:server.request().URL optionalNonce:nil applicationBundleID:@"test.bundle.id"];
> 
> What is the point of passing the bundleID here if we never check it for the
> test?
I'm making it so that there is no way to make a PrivateClickMeasurement without a bundle ID except for pulling them out of legacy databases from mobile safari.

[John Wilander]

(In reply to Alex Christensen from comment #7)
> (In reply to Kate Cheney from comment #6)
> > Comment on attachment 438904 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=438904&action=review
> > 
> > > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:153
> > > +    constexpr auto mobileSafariBundleID = "com.apple.mobilesafari";
> > 
> > I wonder if we should use com.apple.Safari for macOS.
> I don't think so.  com.apple.mobilesafari is the only application that has
> ever had this enabled.  It's never been used on Mac.  This is only for
> legacy data migration purposes.  Maybe I'll rename it to make it more clear
> that is what is going on.

MobileSafari is the only application that has PCM *app-to-web* enabled but regular web-to-web is very much enabled in Safari on macOS.

> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:343
> > > +            [webView _addEventAttributionWithSourceID:42 destinationURL:exampleURL() sourceDescription:@"test source description" purchaser:@"test purchaser" reportEndpoint:server.request().URL optionalNonce:nil applicationBundleID:@"test.bundle.id"];
> > 
> > What is the point of passing the bundleID here if we never check it for the
> > test?
> I'm making it so that there is no way to make a PrivateClickMeasurement
> without a bundle ID except for pulling them out of legacy databases from
> mobile safari.

[John Wilander]

Comment on attachment 438904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438904&action=review

I assume you will add tests with multiple bundle IDs to make sure the matching logic does the right thing once you add it.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:713
>> +    statement->reset();
> 
> No need for this reset since this statement will go out of scope before you use it again.

Can we add an ASSERT that tells everyone that we are relying on that side effect? I'm thinking of when another engineer comes along in two years and makes changes.

[Alex Christensen]

Created attachment 438965 [details]
Patch

[Kate Cheney]

Comment on attachment 438965 [details]
Patch

r=me

[Alex Christensen]

Created attachment 438971 [details]
Patch

[EWS]

Committed r282884 (242013@main): <https://commits.webkit.org/242013@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438971 [details].