Bug 223615 - PCM: Send report to both click source and attribution destination website
Summary: PCM: Send report to both click source and attribution destination website
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
Keywords: InRadar
Depends on:
Reported: 2021-03-22 17:20 PDT by Kate Cheney
Modified: 2021-04-09 22:11 PDT (History)
8 users (show)

See Also:

Patch (263.99 KB, patch)
2021-03-22 17:23 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (265.75 KB, patch)
2021-03-23 11:27 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (267.95 KB, patch)
2021-03-23 18:44 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (76.67 KB, patch)
2021-03-26 15:39 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-03-22 17:20:21 PDT
PCM: Send report to both click source and attribution destination website
Comment 1 Kate Cheney 2021-03-22 17:23:27 PDT
Created attachment 423969 [details]
Comment 2 Kate Cheney 2021-03-23 11:27:38 PDT
Created attachment 424041 [details]
Comment 3 Kate Cheney 2021-03-23 18:44:47 PDT
Created attachment 424085 [details]
Comment 4 John Wilander 2021-03-23 20:41:36 PDT
Comment on attachment 424085 [details]

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

The layout test failure indicates that we're firing double signing requests which would be wrong. I've made some comments on naming and clarity. Too tired to fully review functionality. It'll be easier once we have a little cleared naming and a couple of convenience functions. Marking r- for now, not because the patch is bad but I need to have another go at it and we need to investigate what's going on with the layout test failure.

> Source/WebCore/ChangeLog:17
> +        An attribution is valid if it has any valid reporting time.

Reporting time sounds like how much time it's spent reporting. Something like earliest time to report/send could work. Or something about a pending report.

> Source/WebKit/ChangeLog:12
> +        a new column earliestTimeToSendToAttributionDestinationURL. Refactor

You can call it just destination. Both have the prefix attribution in developer-facing names but in our code we know it's for attribution.

> Source/WebKit/ChangeLog:25
> +        destination site without messing up sorting of reports.

We should make a comment about this in the code (I haven't checked yet).

> Source/WebKit/ChangeLog:42
> +        A value of 0.0 indicates that the report has already been sent.

This should also be pointed out in a comment.

> Source/WebKit/ChangeLog:53
> +        partially-sent reports.

We should make sure we mark something reported even if we get an error back from the server. Otherwise a server can be set up to always report errors and thus allow for continuous re-triggering of attribution. I have not looked at the code (yet) to see if this is an issue. The easiest way to do this is to mark an attribution as hasPreviouslyBeenReported as soon as we bake its first attribution report.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:58
> +        && (m_earliestTimeToSend.sourceEarliestTimeToSend || m_earliestTimeToSend.attributionDestinationEarliestTimeToSend);

This can also be just destination. The attribution prefix is not needed.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:104
> +    auto attributionDestinationURLSecondsUntilSend = 24_h + Seconds(randomNumber() * (24_h).value());

Ditto on destination name. Would also be nice to break out a convenience function called randomlyBetweenTwentyFourAndFortyEightHours().

> Source/WebCore/loader/PrivateClickMeasurement.h:56
> +    enum class CurrentReportURLType : bool { Source, AttributionDestination };

I think this enum should be enum class AttributionReportEndpoint : bool { Source, Destination }.

> Source/WebCore/loader/PrivateClickMeasurement.h:253
> +        Optional<Seconds> attributionDestinationSeconds;


> Source/WebCore/loader/PrivateClickMeasurement.h:296
> +        Optional<WallTime> attributionDestinationEarliestTimeToSend;


> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:87
> +    "sourceID, attributionTriggerData, priority, timeOfAdClick, earliestTimeToSendToSourceURL, token, signature, keyID, earliestTimeToSendToAttributionDestinationURL) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"_s;

Ditto on destination name.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:492
> +    addMissingColumnsToTable(attributedTableName, columnsForTable(attributedTableName), attributedPrivateClickMeasurementExpectedSchema());

We should ask for some DB expert's review of these DB massage pieces.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3006
> +        auto attributionDestinationEarliestTimeToSendValue = statement->getColumnDouble(10);

Ditto on destination name.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3012
> +        Optional<WallTime> attributionDestinationEarliestTimeToSend;

Ditto on destination name.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3075
> +        auto attributionDestinationEarliestTimeToSend = attribution.earliestTimeToSend().attributionDestinationEarliestTimeToSend ? attribution.earliestTimeToSend().attributionDestinationEarliestTimeToSend.value().secondsSinceEpoch().value() : -1;

Ditto on destination name.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3164
> +    WebCore::PrivateClickMeasurement::AttributionSecondsUntilSendData secondsUntilSend { Seconds::infinity(), Seconds::infinity() };

I don't think you need the WebCore name space here. Not even sure you need the PCM name space.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3200
> +        if (!previouslyAttributed.value().earliestTimeToSend().hasPreviouslyBeenReported()) {

I'm not sure I follow the naming here. Can you spell it out? Maybe we can come up with something that reads better left to right. One step of getting this right could be to extract a convenience function.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3217
> +    if (secondsUntilSend.sourceSeconds == Seconds::infinity() || secondsUntilSend.attributionDestinationSeconds == Seconds::infinity())

Could we make this a convenience function that says what it's doing? Comparing with infinity is abstract.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3421
> +void ResourceLoadStatisticsDatabaseStore::markReportAsSentToAttributeOnURL()

I know you will do DB renaming later but this function name should be markReportAsSentToDestinationURL().

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:306
> +    std::unique_ptr<WebCore::SQLiteStatement> m_markReportAsSentToAttributeOnURLStatement;

Should be named destination.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:303
> +void PrivateClickMeasurementManager::fireConversionRequest(const PrivateClickMeasurement& attribution, PrivateClickMeasurement::CurrentReportURLType currentReportURLType)

Rename parameter according to what you go for in the enum.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:352
> +    

Not needed.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:363
> +void PrivateClickMeasurementManager::clearSentAttribution(PrivateClickMeasurement&& sentConversion, PrivateClickMeasurement::CurrentReportURLType currentReportURLType)

Ditto on parameter name.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:75
> +    void clearSentAttribution(PrivateClickMeasurement&&, WebCore::PrivateClickMeasurement::CurrentReportURLType);

WebCore namespace not needed.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:77
> +    void fireConversionRequest(const PrivateClickMeasurement&, WebCore::PrivateClickMeasurement::CurrentReportURLType);

WebCore namespace not needed.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:78
> +    void fireConversionRequestImpl(const PrivateClickMeasurement&, WebCore::PrivateClickMeasurement::CurrentReportURLType);

WebCore namespace not needed.
Comment 5 John Wilander 2021-03-23 20:43:13 PDT
The mac-wk2 bot now passed layout tests. That indicates that it's flaky. I believe it was LayoutTests/http/tests/privateClickMeasurement/store-private-click-measurement-with-source-nonce.html that failed once or twice.
Comment 6 Kate Cheney 2021-03-24 08:46:39 PDT
Ok, let me have another go with the renaming and flaky test addressed. I wonder if I should split out the sqlite changes because they have their own tests and can stand on their own. I can do the renaming in there as well from attributeOn -> attributionDestination. Then post a followup for strict PCM changes.
Comment 7 John Wilander 2021-03-24 08:50:59 PDT
(In reply to katherine_cheney from comment #6)
> Ok, let me have another go with the renaming and flaky test addressed. I
> wonder if I should split out the sqlite changes because they have their own
> tests and can stand on their own. I can do the renaming in there as well
> from attributeOn -> attributionDestination. Then post a followup for strict
> PCM changes.

If it’s not a hassle for you, reducing the complexity would help me do a good review of the logic.
Comment 8 Radar WebKit Bug Importer 2021-03-25 13:03:14 PDT
Comment 9 Kate Cheney 2021-03-26 15:39:22 PDT
Created attachment 424409 [details]
Comment 10 Brent Fulgham 2021-03-26 15:47:59 PDT
Comment on attachment 424409 [details]

Comment 11 EWS 2021-03-27 11:52:57 PDT
Committed r275137: <https://commits.webkit.org/r275137>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424409 [details].
Comment 12 John Wilander 2021-03-30 15:53:41 PDT
Comment on attachment 424409 [details]

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

I know this has already landed. Just here checking in. The naming is so much clearer now!

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:3517
> +    }

I believe ports not building with Clang need a macro here. Otherwise they get a build failure because the compiler thinks there's a default clause missing.

> LayoutTests/http/tests/privateClickMeasurement/send-attribution-conversion-request-expected.txt:31
> +{"source_engagement_type":"click","source_site":"","source_id":3,"attributed_on_site":"localhost","trigger_data":12,"version":2}

Nice. It's working!