Bug 197183 - Ad Click Attribution redirects to well-known location should not trigger a conversion if they are blocked by content blockers
Summary: Ad Click Attribution redirects to well-known location should not trigger a co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-22 16:29 PDT by John Wilander
Modified: 2019-04-23 19:31 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.22 KB, patch)
2019-04-22 16:45 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-04-22 16:29:42 PDT
We should make sure Ad Click Attribution redirects to well-known location don't trigger a conversion if they are blocked by content blockers.
Comment 1 John Wilander 2019-04-22 16:29:58 PDT
<rdar://problem/47763188>
Comment 2 John Wilander 2019-04-22 16:45:18 PDT
Created attachment 367996 [details]
Patch
Comment 3 youenn fablet 2019-04-22 22:16:46 PDT
Comment on attachment 367996 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        a blocked request comes back empty.

It makes sense to be consistent there with content blockers but more generally with the whole loading code.
Web Process might cancel the load due to injected bundles, CSP, the context going away...
The easiest approach might be to do the handleAdClickAttributionConversion call in NetworkResourceLoader::continueWillSendRequest if the request is not null.
That means keeping an Optional<AdClickAttribution::Conversion> as a member field of NetworkResourceLoader and populate it in NetworkResourceLoader::willSendRedirectedRequest.
Comment 4 Alex Christensen 2019-04-23 08:03:10 PDT
Comment on attachment 367996 [details]
Patch

I think this is consistent with the rest of the loading code.
Comment 5 John Wilander 2019-04-23 08:57:42 PDT
Comment on attachment 367996 [details]
Patch

Thanks, both of you! Let’s revisit this when we move content blockers to the network process.
Comment 6 WebKit Commit Bot 2019-04-23 09:14:13 PDT
Comment on attachment 367996 [details]
Patch

Clearing flags on attachment: 367996

Committed r244544: <https://trac.webkit.org/changeset/244544>
Comment 7 WebKit Commit Bot 2019-04-23 09:14:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 youenn fablet 2019-04-23 19:31:50 PDT
> I think this is consistent with the rest of the loading code.

Not really. Even though NetworkProcess is more and more independent of WebProcess, this is still not the case. Given network load checker can be null in some cases, given injected bundles, the most consistent place to do the whole conversion is in NetworkResourceLoader::continueWillSendRequest. There may not even be the need to add a  NetworkResourceLoader field since originalRequest is kept.