Bug 221453 – Make sure click attribution is processed in case of redirected kept alive loads

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Make sure click attribution is processed in case of redirected kept alive loads

[youenn fablet]

<rdar://problem/70896640>

[youenn fablet]

Created attachment 419368 [details]
Patch

[Kate Cheney]

Comment on attachment 419368 [details]
Patch

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

> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive-expected.txt:1
> +CONSOLE MESSAGE: [Private Click Measurement] Conversion was not accepted because the URL path contained unrecognized parts.

If you changed "whatever" to a number between 0 and 15, you could get rid of this console message and successfully convert the report.

> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive-expected.txt:2
> +CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin.

It seems like maybe you did not intend for this message to be there because you added header("Access-Control-Allow-Origin: *"); in redirectToConversion.php?

> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html:43
> +            triggerFetch("whatever").catch(() => {

e.g. triggerFetch("15").catch(() => {

[John Wilander]

Comment on attachment 419368 [details]
Patch

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

Thanks for fixing this!

> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive-expected.txt:2
> +CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin.

Could we add a header to the php file to silence this output?

> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive-expected.txt:3
> +CONSOLE MESSAGE: Fetch API cannot load https://127.0.0.1:8443/.well-known/private-click-measurement/trigger-attribution/whatever due to access control checks.

Ditto.

> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html:36
> +            triggerFetch(12);

Do we risk being flaky because of this time dependence?

> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html:42
> +            // Do an invalid private click attribution fetch to ensure the previous correct click attribution fetch will be finished.

Clever.

[Kate Cheney]

Comment on attachment 419368 [details]
Patch

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

>> LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html:43
>> +            triggerFetch("whatever").catch(() => {
> 
> e.g. triggerFetch("15").catch(() => {

Ah, I see this was intentional. Disregard my comments.

[youenn fablet]

(In reply to katherine_cheney from comment #3)
> Comment on attachment 419368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419368&action=review
> 
> > LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive-expected.txt:1
> > +CONSOLE MESSAGE: [Private Click Measurement] Conversion was not accepted because the URL path contained unrecognized parts.
> 
> If you changed "whatever" to a number between 0 and 15, you could get rid of
> this console message and successfully convert the report.

I would prefer not to. There should be only one report, the second fetch is just a way to not be flaky (hopefully).

> > LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive-expected.txt:2
> > +CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin.
> 
> Could we add a header to the php file to silence this output?

There is no php file, it is receiving a 404 straight from Apache and without any CORS header.
We could try to change the redirect to something else that would point to a real php script but this does not seem worth it.

> > LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive-expected.txt:3
> > +CONSOLE MESSAGE: Fetch API cannot load https://127.0.0.1:8443/.well-known/private-click-measurement/trigger-attribution/whatever due to access control checks.
> 
> Ditto.
> 
> > LayoutTests/http/tests/privateClickMeasurement/attribution-conversion-through-fetch-keepalive.html:36
> > +            triggerFetch(12);
> 
> Do we risk being flaky because of this time dependence?

Hopefully no, this fetch will happen first so should complete before the second one.
But this is not bullet proof given we are talking of network requests that could be in theory done in parallel.

[youenn fablet]

Comment on attachment 419368 [details]
Patch

CQ+ it now since it seems good to have it in the build sooner rather than later. Let me know if you would still like some removal of console lines.

[EWS]

Committed r272422: <https://trac.webkit.org/changeset/272422>

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