PCM: Send report to both click source and attribution destination website

• Component: New Bugs
• Status: RESOLVED FIXED

[Kate Cheney]

PCM: Send report to both click source and attribution destination website

[Kate Cheney]

Created attachment 423969 [details]
Patch

[Kate Cheney]

Created attachment 424041 [details]
Patch

[Kate Cheney]

Created attachment 424085 [details]
Patch

[John Wilander]

Comment on attachment 424085 [details]
Patch

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;

Ditto.

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

Ditto.

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

[John Wilander]

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.

[Kate Cheney]

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.

[John Wilander]

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

[Radar WebKit Bug Importer]

<rdar://problem/75849443>

[Kate Cheney]

Created attachment 424409 [details]
Patch

[Brent Fulgham]

Comment on attachment 424409 [details]
Patch

r=mews

[EWS]

Committed r275137: <https://commits.webkit.org/r275137>

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

[John Wilander]

Comment on attachment 424409 [details]
Patch

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":"127.0.0.1","source_id":3,"attributed_on_site":"localhost","trigger_data":12,"version":2}

Nice. It's working!