Bug 228984 – PCM: Support ephemeral measurement with non-persistent WebCore::PrivateClickMeasurement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We should support ephemeral measurement with non-persistent WebCore::PrivateClickMeasurement for direct response advertising.

[Radar WebKit Bug Importer]

<rdar://problem/81778213>

[John Wilander]

Created attachment 435325 [details]
Patch

[youenn fablet]

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.

[John Wilander]

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

[John Wilander]

Created attachment 435456 [details]
Patch

[Kate Cheney]

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.

[John Wilander]

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

[Chris Dumez]

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.

[John Wilander]

Created attachment 436248 [details]
Patch for landing

[John Wilander]

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

[EWS]

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