Make sure click attribution is processed in case of redirected kept alive loads
<rdar://problem/70896640>
Created attachment 419368 [details] Patch
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(() => {
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.
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.
(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.
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.
Committed r272422: <https://trac.webkit.org/changeset/272422> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419368 [details].