Bug 194510 - Store Ad Click Attribution requests in the network process
Summary: Store Ad Click Attribution requests in the network process
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-02-11 12:09 PST by John Wilander
Modified: 2019-04-05 15:56 PDT (History)
10 users (show)

See Also:


Attachments
Patch (53.64 KB, patch)
2019-02-11 12:35 PST, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.43 MB, application/zip)
2019-02-11 13:41 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.12 MB, application/zip)
2019-02-11 14:24 PST, Build Bot
no flags Details
Patch (62.17 KB, patch)
2019-02-12 17:35 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (62.49 KB, patch)
2019-02-13 10:02 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (62.65 KB, patch)
2019-02-13 12:19 PST, 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-02-11 12:09:49 PST
We should forward Ad Click Attribution requests from the NavigationAction to the network process.
Comment 1 John Wilander 2019-02-11 12:10:04 PST
<rdar://problem/47650118>
Comment 2 John Wilander 2019-02-11 12:35:11 PST
Created attachment 361703 [details]
Patch
Comment 3 Build Bot 2019-02-11 13:41:02 PST
Comment on attachment 361703 [details]
Patch

Attachment 361703 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11112916

New failing tests:
http/tests/adClickAttribution/store-ad-click-attribution.html
Comment 4 Build Bot 2019-02-11 13:41:04 PST
Created attachment 361708 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 5 Build Bot 2019-02-11 14:24:21 PST
Comment on attachment 361703 [details]
Patch

Attachment 361703 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11113016

New failing tests:
http/tests/adClickAttribution/store-ad-click-attribution.html
Comment 6 Build Bot 2019-02-11 14:24:23 PST
Created attachment 361713 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 7 Daniel Bates 2019-02-11 21:47:14 PST
Comment on attachment 361703 [details]
Patch

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

As I read through this patch this thought repeatedly hit me, "how can I write this code to shoot myself in the face." Please think about encapsulation. How can we avoid taking a shotgun to the face, foot, arm, torso. There is the potential for good here. Polish it up.

> Source/WebCore/loader/AdClickAttribution.h:133
> +    Source& source() { return m_source; };

Why return a lvalue ref?

> Source/WebCore/loader/AdClickAttribution.h:134
> +    Destination& destination() { return m_destination; };

Why return a lvalue ref?

> Source/WebCore/loader/NavigationAction.h:138
> +    Optional<AdClickAttribution> adClickAttribution() const { return m_adClickAttribution; };

Why change the return value?

> Source/WebKit/UIProcess/API/APINavigation.h:147
> +    Optional<WebCore::AdClickAttribution> adClickAttribution() const { return m_lastNavigationAction.adClickAttribution; }

return lvalue ref?
Comment 8 Daniel Bates 2019-02-11 21:58:59 PST
(In reply to Daniel Bates from comment #7)
> Comment on attachment 361703 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361703&action=review
> 
> As I read through this patch this thought repeatedly hit me, "how can I
> write this code to shoot myself in the face." Please think about
> encapsulation. How can we avoid taking a shotgun to the face, foot, arm,
> torso. There is the potential for good here. Polish it up.
> 

Thinking about this comment some more. It sounds negative. What I meant to say: this patch has potential and it would be better if it can be tightened up.
Comment 9 Daniel Bates 2019-02-11 22:36:12 PST
Comment on attachment 361703 [details]
Patch

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

> Source/WebCore/loader/AdClickAttribution.h:120
> +    AdClickAttribution() = default;

Why?

> Source/WebCore/loader/AdClickAttribution.h:130
> +    bool matchesDestination(const URL& url) const { return m_destination == Destination { topPrivatelyControlledDomain(url.host().toString()) }; };

Something feels weird about this convenience function. I think it's because you also exposed destination. It's confusing because why would a person use this function when they can just compare destination themselves since you exposed it. Yeah, I get that this helper does a bit more. Maybe making this a static, non-member function would help. Maybe tightening the encapsulation of source and destination. Design feels off.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:36
> +HashMap<String, AdClickAttribution>& NetworkAdClickAttribution::ensureAttributionsForAdClickSource(const AdClickAttribution::Source& source)

Use an alias for the return type so we don't have to repeat it everywhere.

No need for AdClick in name. It's redundant with the class name

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:38
> +    return m_adClickAttributionMap.ensure(source.registrableDomain, [] {

Code formatting looks weird. Grep the code to look for other examples for how we write this.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:69
> +    if (builder.isEmpty())

Move this check to the top (obviously can't be in terms of builder then -  do not find a way to write it in terms of builder), call the completion handler, and return.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:45
> +    HashMap<String, AdClickAttribution>& ensureAttributionsForAdClickSource(const AdClickAttribution::Source&);

Write using Source *NOT* String. Use type alias see below for more details.

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:47
> +    HashMap<String, HashMap<String, AdClickAttribution>> m_adClickAttributionMap;

Write this in terms of Source and Destination for type safety. Otherwise, what is the point of having Source and Destination classes? Use type alias to avoid repeativeness see my remake in .cpp

> LayoutTests/http/tests/adClickAttribution/store-ad-click-attribution-expected.txt:10
> +WebCore::AdClickAttribution 1

Stop doing this! This is a insult to me because I already told you this before in a previous bug (I will look it up when I have a moment). I am now taking this insult personally. I spend my time to provide review feedback and this is a slap in the face and reads that you do not care about such feedback. Stop printing text after TEST COMPLETE. Here's one solution to this problem: stop using js-test.js's description().
Comment 10 Daniel Bates 2019-02-11 22:44:48 PST
(In reply to Daniel Bates from comment #9)
> Stop doing this! This is a insult to me because I already told you this
> before in a previous bug (I will look it up when I have a moment). I am now
> taking this insult personally. I spend my time to provide review feedback
> and this is a slap in the face and reads that you do not care about such
> feedback. Stop printing text after TEST COMPLETE. Here's one solution to
> this problem: stop using js-test.js's description().

Actually, it's not just the text emitted after TEST COMPLETE it's also the missing PASS/FAIL messages and the extraneous text "Link", both isssues I also told you about in the same previous bug.
Comment 11 John Wilander 2019-02-12 11:57:14 PST
(In reply to Daniel Bates from comment #7)
> Comment on attachment 361703 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361703&action=review
> 
> As I read through this patch this thought repeatedly hit me, "how can I
> write this code to shoot myself in the face." Please think about
> encapsulation. How can we avoid taking a shotgun to the face, foot, arm,
> torso. There is the potential for good here. Polish it up.
> 
> > Source/WebCore/loader/AdClickAttribution.h:133
> > +    Source& source() { return m_source; };
> 
> Why return a lvalue ref?
> 
> > Source/WebCore/loader/AdClickAttribution.h:134
> > +    Destination& destination() { return m_destination; };
> 
> Why return a lvalue ref?
> 
> > Source/WebCore/loader/NavigationAction.h:138
> > +    Optional<AdClickAttribution> adClickAttribution() const { return m_adClickAttribution; };
> 
> Why change the return value?
> 
> > Source/WebKit/UIProcess/API/APINavigation.h:147
> > +    Optional<WebCore::AdClickAttribution> adClickAttribution() const { return m_lastNavigationAction.adClickAttribution; }
> 
> return lvalue ref?

These are structured this way because WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() takes a const NavigationAction& argument, calls navigationAction.adClickAttribution(), which returns m_lastNavigationAction.adClickAttribution. That chain has to be declared const and returning lvalue refs forces me to drop the const.

Is there some other way to do this?
Comment 12 Daniel Bates 2019-02-12 12:30:44 PST
(In reply to John Wilander from comment #11)
> (In reply to Daniel Bates from comment #7)
> > Comment on attachment 361703 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=361703&action=review
> > 
> > As I read through this patch this thought repeatedly hit me, "how can I
> > write this code to shoot myself in the face." Please think about
> > encapsulation. How can we avoid taking a shotgun to the face, foot, arm,
> > torso. There is the potential for good here. Polish it up.
> > 
> > > Source/WebCore/loader/AdClickAttribution.h:133
> > > +    Source& source() { return m_source; };
> > 
> > Why return a lvalue ref?
> > 
> > > Source/WebCore/loader/AdClickAttribution.h:134
> > > +    Destination& destination() { return m_destination; };
> > 
> > Why return a lvalue ref?
> > 
> > > Source/WebCore/loader/NavigationAction.h:138
> > > +    Optional<AdClickAttribution> adClickAttribution() const { return m_adClickAttribution; };
> > 
> > Why change the return value?
> > 
> > > Source/WebKit/UIProcess/API/APINavigation.h:147
> > > +    Optional<WebCore::AdClickAttribution> adClickAttribution() const { return m_lastNavigationAction.adClickAttribution; }
> > 
> > return lvalue ref?
> 
> These are structured this way because
> WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() takes a
> const NavigationAction& argument, calls
> navigationAction.adClickAttribution(), which returns
> m_lastNavigationAction.adClickAttribution. That chain has to be declared
> const and returning lvalue refs forces me to drop the const.
> 
> Is there some other way to do this?

All of the above should return const lvalue ref from a const function.
Comment 13 John Wilander 2019-02-12 12:33:17 PST
(In reply to Daniel Bates from comment #9)
> Comment on attachment 361703 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361703&action=review
> 
> > Source/WebCore/loader/AdClickAttribution.h:120
> > +    AdClickAttribution() = default;
> 
> Why?

The generated IPC code needs the default constructors added. Chris helped me figure this out.

> > Source/WebCore/loader/AdClickAttribution.h:130
> > +    bool matchesDestination(const URL& url) const { return m_destination == Destination { topPrivatelyControlledDomain(url.host().toString()) }; };
> 
> Something feels weird about this convenience function. I think it's because
> you also exposed destination. It's confusing because why would a person use
> this function when they can just compare destination themselves since you
> exposed it. Yeah, I get that this helper does a bit more. Maybe making this
> a static, non-member function would help. Maybe tightening the encapsulation
> of source and destination. Design feels off.

OK, I can do a regular comparison in WebKit::WebPageProxy instead, like so:
Destination { frame->url().host().toString() } == adClickAttribution->destination()

But it doesn't read as well as
adClickAttribution->matchesDestination(frame->url())
… in my veiw.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:36
> > +HashMap<String, AdClickAttribution>& NetworkAdClickAttribution::ensureAttributionsForAdClickSource(const AdClickAttribution::Source& source)
> 
> Use an alias for the return type so we don't have to repeat it everywhere.

Sure.

> No need for AdClick in name. It's redundant with the class name
> 
> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:38
> > +    return m_adClickAttributionMap.ensure(source.registrableDomain, [] {
> 
> Code formatting looks weird. Grep the code to look for other examples for
> how we write this.
> 
> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:69
> > +    if (builder.isEmpty())
> 
> Move this check to the top (obviously can't be in terms of builder then - 
> do not find a way to write it in terms of builder), call the completion
> handler, and return.

Good idea.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:45
> > +    HashMap<String, AdClickAttribution>& ensureAttributionsForAdClickSource(const AdClickAttribution::Source&);
> 
> Write using Source *NOT* String. Use type alias see below for more details.

OK, I'll introduce the necessary support for hashes.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:47
> > +    HashMap<String, HashMap<String, AdClickAttribution>> m_adClickAttributionMap;
> 
> Write this in terms of Source and Destination for type safety. Otherwise,
> what is the point of having Source and Destination classes? Use type alias
> to avoid repeativeness see my remake in .cpp

OK, I'll introduce the necessary support for hashes.

> > LayoutTests/http/tests/adClickAttribution/store-ad-click-attribution-expected.txt:10
> > +WebCore::AdClickAttribution 1
> 
> Stop doing this! This is a insult to me because I already told you this
> before in a previous bug (I will look it up when I have a moment). I am now
> taking this insult personally. I spend my time to provide review feedback
> and this is a slap in the face and reads that you do not care about such
> feedback. Stop printing text after TEST COMPLETE. Here's one solution to
> this problem: stop using js-test.js's description().

Going forward, you can assume I'm not trying to insult anyone with my patches. The benefit of the doubt is a much better approach in my view.

In this particular case, I'm using a dump function that appends to the test output. As discussed offline, I can avoid using js-test and output test results manually.
Comment 14 Daniel Bates 2019-02-12 13:07:38 PST
Look at Source/WebCore/page/csp/ContentSecurityPolicyHash.h in <https://bugs.webkit.org/attachment.cgi?id=273515&action=prettypatch> for an example of  defining a HashSet<> compatible object. In that patch I make ContentSecurityPolicyHash HashSet<> compatible.
Comment 15 Daniel Bates 2019-02-12 13:11:11 PST
(In reply to Daniel Bates from comment #14)
> Look at Source/WebCore/page/csp/ContentSecurityPolicyHash.h in
> <https://bugs.webkit.org/attachment.cgi?id=273515&action=prettypatch> for an
> example of  defining a HashSet<> compatible object. In that patch I make
> ContentSecurityPolicyHash HashSet<> compatible.

Maybe looking at StringHash in HashTraits.h may help as well. Grepping the code for more examples.
Comment 16 Daniel Bates 2019-02-12 13:13:06 PST
(In reply to John Wilander from comment #13)
> (In reply to Daniel Bates from comment #9)
> > Comment on attachment 361703 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=361703&action=review
> > 
> > > Source/WebCore/loader/AdClickAttribution.h:120
> > > +    AdClickAttribution() = default;
> > 
> > Why?
> 
> The generated IPC code needs the default constructors added. Chris helped me
> figure this out.
> 
> > > Source/WebCore/loader/AdClickAttribution.h:130
> > > +    bool matchesDestination(const URL& url) const { return m_destination == Destination { topPrivatelyControlledDomain(url.host().toString()) }; };
> > 
> > Something feels weird about this convenience function. I think it's because
> > you also exposed destination. It's confusing because why would a person use
> > this function when they can just compare destination themselves since you
> > exposed it. Yeah, I get that this helper does a bit more. Maybe making this
> > a static, non-member function would help. Maybe tightening the encapsulation
> > of source and destination. Design feels off.
> 
> OK, I can do a regular comparison in WebKit::WebPageProxy instead, like so:
> Destination { frame->url().host().toString() } ==
> adClickAttribution->destination()
> 
> But it doesn't read as well as
> adClickAttribution->matchesDestination(frame->url())
> … in my veiw.
> 

What about putting this matching function on Destination itself?
Comment 17 John Wilander 2019-02-12 17:35:29 PST
Created attachment 361876 [details]
Patch
Comment 18 John Wilander 2019-02-12 17:38:05 PST
I've hopefully addressed all issues including the build errors on non-Apple ports (all missing header includes).

Dan, the one thing I couldn't address was the alleged weird formatting of:

return m_adClickAttributionMap.ensure(source.registrableDomain, [] {

… in WebKit::NetworkAdClickAttribution. I looked at a handful of other ensure() calls and they seem formatted this way and the style checker doesn't complain.
Comment 19 John Wilander 2019-02-13 10:02:27 PST
Created attachment 361922 [details]
Patch
Comment 20 John Wilander 2019-02-13 10:03:06 PST
Windows build fix. The rest is good to go. R+? :)
Comment 21 Alex Christensen 2019-02-13 10:30:13 PST
Comment on attachment 361922 [details]
Patch

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

r=me, lots of nits

> Source/WebCore/loader/AdClickAttribution.cpp:96
> +    builder.appendLiteral("Source: ");

We might find it useful in the future to have this in JSON format.  We can always change it if needed, but just a thought.

> Source/WebCore/loader/AdClickAttribution.h:100
>          String registrableDomain;
> +        bool isDeleted { false };

String has a constructor that takes HashTableDeletedValueType, so we don't need a separate bool for this.

> Source/WebCore/loader/AdClickAttribution.h:167
>          String registrableDomain;
> +        bool isDeleted { false };

ditto

> Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:67
> +            builder.appendLiteral("WebCore::AdClickAttribution ");

NetworkAdClickAttribution?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2301
> +    completionHandler(emptyString());

I'm not sure if it's important to use emptyString here or if a null string would suffice.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4106
> +        if (auto adClickAttribution = navigation->adClickAttribution()) {

auto&
No need to copy the Optional.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2731
> +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("dumpAdClickAttribution"));

This is old style WKRetainPtr use.  Try this instead:
auto messageName = adoptWK(WKStringCreateWithUTF8CString(...));

> Tools/WebKitTestRunner/TestInvocation.h:148
> +    String m_savedAdClickAttribution;

This is never set.  We can remove it.
Comment 22 John Wilander 2019-02-13 12:17:26 PST
(In reply to Alex Christensen from comment #21)
> Comment on attachment 361922 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361922&action=review
> 
> r=me, lots of nits
> 
> > Source/WebCore/loader/AdClickAttribution.cpp:96
> > +    builder.appendLiteral("Source: ");
> 
> We might find it useful in the future to have this in JSON format.  We can
> always change it if needed, but just a thought.

I see. Yeah, I'll fix that when/if needed.

> > Source/WebCore/loader/AdClickAttribution.h:100
> >          String registrableDomain;
> > +        bool isDeleted { false };
> 
> String has a constructor that takes HashTableDeletedValueType, so we don't
> need a separate bool for this.

Will fix.

> > Source/WebCore/loader/AdClickAttribution.h:167
> >          String registrableDomain;
> > +        bool isDeleted { false };
> 
> ditto

Will fix.

> > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:67
> > +            builder.appendLiteral("WebCore::AdClickAttribution ");
> 
> NetworkAdClickAttribution?

No, actually WebCore::AdClickAttribution. It'll output one of these per WebCore::AdClickAttribution object in the map and enumerate them.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2301
> > +    completionHandler(emptyString());
> 
> I'm not sure if it's important to use emptyString here or if a null string
> would suffice.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:4106
> > +        if (auto adClickAttribution = navigation->adClickAttribution()) {
> 
> auto&
> No need to copy the Optional.

Will fix.

> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2731
> > +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("dumpAdClickAttribution"));
> 
> This is old style WKRetainPtr use.  Try this instead:
> auto messageName = adoptWK(WKStringCreateWithUTF8CString(...));

Will fix.

> > Tools/WebKitTestRunner/TestInvocation.h:148
> > +    String m_savedAdClickAttribution;
> 
> This is never set.  We can remove it.

Will fix.


Thanks Alex and Dan for all of the review comments!
Comment 23 John Wilander 2019-02-13 12:19:19 PST
Created attachment 361931 [details]
Patch
Comment 24 John Wilander 2019-02-13 12:19:48 PST
Comment on attachment 361931 [details]
Patch

Just making sure the Windows bot is happy before landing.
Comment 25 John Wilander 2019-02-13 12:21:03 PST
Actually, the previous EWS run already successfully built on Windows. Yay.
Comment 26 WebKit Commit Bot 2019-02-13 12:47:06 PST
Comment on attachment 361931 [details]
Patch

Clearing flags on attachment: 361931

Committed r241451: <https://trac.webkit.org/changeset/241451>
Comment 27 WebKit Commit Bot 2019-02-13 12:47:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Alex Christensen 2019-02-18 19:05:30 PST
Given https://bugs.webkit.org/show_bug.cgi?id=194774 and the fact that it was still asserting after that, and the test started failing when I disabled safe browsing, this is fundamentally flawed somehow.  I reverted the WebPageProxy part of this patch in https://trac.webkit.org/changeset/241754/webkit
Comment 29 Alex Christensen 2019-02-18 19:44:50 PST
http://trac.webkit.org/r241755
Comment 30 John Wilander 2019-04-05 15:56:24 PDT
This was resolved again through patches tracked in the related bugs ("See Also").