Bug 228984

Summary: PCM: Support ephemeral measurement with non-persistent WebCore::PrivateClickMeasurement
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, ews-watchlist, japhet, katherine_cheney, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description John Wilander 2021-08-10 23:01:45 PDT
We should support ephemeral measurement with non-persistent WebCore::PrivateClickMeasurement for direct response advertising.
Comment 1 Radar WebKit Bug Importer 2021-08-10 23:02:05 PDT
<rdar://problem/81778213>
Comment 2 John Wilander 2021-08-10 23:17:19 PDT
Created attachment 435325 [details]
Patch
Comment 3 youenn fablet 2021-08-12 04:52:04 PDT
Comment on attachment 435325 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1521
> +void WebResourceLoadStatisticsStore::attributePrivateClickMeasurement(const PrivateClickMeasurement::SourceSite& sourceSite, const PrivateClickMeasurement::AttributionDestinationSite& destinationSite, PrivateClickMeasurement::AttributionTriggerData&& attributionTriggerData, std::optional<PrivateClickMeasurement> ephemeralMeasurement, CompletionHandler<void(std::optional<WebCore::PrivateClickMeasurement::AttributionSecondsUntilSendData>)>&& completionHandler)

Should probably be optional<>&& since that could otherwise trigger ref/unref

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1530
> +    postTask([this, sourceSite, destinationSite, attributionTriggerData = WTFMove(attributionTriggerData), ephemeralMeasurement, completionHandler = WTFMove(completionHandler)]() mutable {

postTask probably means going to a background thread.
In that case we should isolate copy ephemeralMeasurement which has some strings.
Comment 4 John Wilander 2021-08-12 10:05:50 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 435325 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435325&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1521
> > +void WebResourceLoadStatisticsStore::attributePrivateClickMeasurement(const PrivateClickMeasurement::SourceSite& sourceSite, const PrivateClickMeasurement::AttributionDestinationSite& destinationSite, PrivateClickMeasurement::AttributionTriggerData&& attributionTriggerData, std::optional<PrivateClickMeasurement> ephemeralMeasurement, CompletionHandler<void(std::optional<WebCore::PrivateClickMeasurement::AttributionSecondsUntilSendData>)>&& completionHandler)
> 
> Should probably be optional<>&& since that could otherwise trigger ref/unref
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1530
> > +    postTask([this, sourceSite, destinationSite, attributionTriggerData = WTFMove(attributionTriggerData), ephemeralMeasurement, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> postTask probably means going to a background thread.
> In that case we should isolate copy ephemeralMeasurement which has some
> strings.

Thanks, Youenn! Will fix in the patch that also has the test.
Comment 5 John Wilander 2021-08-12 16:27:19 PDT
Created attachment 435456 [details]
Patch
Comment 6 Kate Cheney 2021-08-18 13:27:35 PDT
Comment on attachment 435456 [details]
Patch

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

Needs to be rebased to fix some data races but otherwise LGTM.

> Source/WebCore/loader/PrivateClickMeasurement.h:50
> +enum class PrivateClickMeasurementAttributionPersistent : bool { Yes, No };

I think we usually use { No, Yes }.

> Source/WebCore/loader/PrivateClickMeasurement.h:392
> +    PrivateClickMeasurementAttributionPersistent m_isPersistent;

You may need to set the default value to be ::Yes, I think enums default to 0 which will be ::No if you change the ordering above.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:248
> +        m_ephemeralMeasurement = measurement;

m_ephemeralMeasurement = WTFMove(measurement) I think.
Comment 7 John Wilander 2021-08-23 14:05:51 PDT
(In reply to Kate Cheney from comment #6)
> Comment on attachment 435456 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435456&action=review
> 
> Needs to be rebased to fix some data races but otherwise LGTM.

I saw that. Will fix.

> > Source/WebCore/loader/PrivateClickMeasurement.h:50
> > +enum class PrivateClickMeasurementAttributionPersistent : bool { Yes, No };
> 
> I think we usually use { No, Yes }.

Will fix.

> > Source/WebCore/loader/PrivateClickMeasurement.h:392
> > +    PrivateClickMeasurementAttributionPersistent m_isPersistent;
> 
> You may need to set the default value to be ::Yes, I think enums default to
> 0 which will be ::No if you change the ordering above.
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:248
> > +        m_ephemeralMeasurement = measurement;
> 
> m_ephemeralMeasurement = WTFMove(measurement) I think.

Good catch. Will fix.

Thanks for the review, Kate!
Comment 8 Chris Dumez 2021-08-23 14:09:30 PDT
Comment on attachment 435456 [details]
Patch

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

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:300
> +        resourceLoadStatistics->attributePrivateClickMeasurement(sourceSite, destinationSite, WTFMove(attributionTriggerData), WTFMove(m_ephemeralMeasurement), [this, weakThis = makeWeakPtr(*this)] (auto attributionSecondsUntilSendData) {

Please use std::exchange() instead of WTFMove() for m_ephemeralMeasurement for avoid use-after-move bugs.
Comment 9 John Wilander 2021-08-23 16:45:57 PDT
Created attachment 436248 [details]
Patch for landing
Comment 10 John Wilander 2021-08-23 16:49:33 PDT
(In reply to Chris Dumez from comment #8)
> Comment on attachment 435456 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435456&action=review
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:300
> > +        resourceLoadStatistics->attributePrivateClickMeasurement(sourceSite, destinationSite, WTFMove(attributionTriggerData), WTFMove(m_ephemeralMeasurement), [this, weakThis = makeWeakPtr(*this)] (auto attributionSecondsUntilSendData) {
> 
> Please use std::exchange() instead of WTFMove() for m_ephemeralMeasurement
> for avoid use-after-move bugs.

I made this change. Thanks, Chris!
Comment 11 EWS 2021-08-23 17:33:40 PDT
Committed r281480 (240858@main): <https://commits.webkit.org/240858@main>

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