Bug (Ram) – PCM: Accept ad click data when the link opens a new window

• Component: WebKit2
• Status: RESOLVED FIXED

[Ramkumar]

Hello Team, 

I'm testing [Private click measurement](https://webkit.org/blog/8943/privacy-preserving-ad-click-attribution-for-the-web/) feature. I have enabled experiment features for `adclick attribution` &  `adclick attribution debug mode` in Latest Safari Tech Preview Browser version: `Release 109 (Safari 14.0, WebKit 15610.1.17.2)`. 

When trying to repro(in localhost) the ad click attribution behavior per the proposal (with redirect and waiting on step 3) -- I load the href link in the same window, I get everything as expected -- Storing ad click, redirect and attribution call after 60 seconds. [console logs](https://user-images.githubusercontent.com/2718218/87091235-2f68dc80-c1ee-11ea-8750-ae9abe997353.png) 

when I load the ad click on a new window(`target="_blank"`) as the typical behavior in from publisher page, I don't see the repro for Step 1( and subsequently for step 2 and 3). Expected behavior when I load the adlink on new window is to see "Storing Ad Click" in the console logs(debug mode).

This is was posted earlier in GH page and was asked to report here.

[Radar WebKit Bug Importer]

<rdar://problem/65358005>

[Kartik]

Hello Team,

Do you have any updates on this bug?

Thanks,
Kartik

[John Wilander]

Hi, and thanks for filing this bug! No updates at this point other than that this should be fixed.

[John Wilander]

*** Bug 218127 has been marked as a duplicate of this bug. ***

[John Wilander]

Created attachment 412507 [details]
Patch

[Chris Dumez]

Comment on attachment 412507 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.h:2889
> +    Optional<WebCore::AdClickAttribution> m_newPageNavigationAdClickAttribution;

It would be good to find a way to address this without adding a new data member to WebPageProxy. The adClickAttribution is associated with a navigation, not a page.

[Chris Dumez]

Comment on attachment 412507 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        where it can be picked up in WebPageProxy::didCommitLoadForFrame().

The change log is unclear about why WebPageProxy::createNewPage() somehow drops the adClickAttribution which is present in the NavigationAction.

[John Wilander]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 412507 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412507&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:2889
> > +    Optional<WebCore::AdClickAttribution> m_newPageNavigationAdClickAttribution;
> 
> It would be good to find a way to address this without adding a new data
> member to WebPageProxy. The adClickAttribution is associated with a
> navigation, not a page.

I've spent a lot of time trying to figure that out. The NavigationAction object is not forwarded to the new page and the handling is done in the client which differs per application using WebKit as far as I can tell, i.e. TestRunner has one handling and Safari has another. That looked like a risk of passing tests with regressions in Safari or other users of WebKit. If you have guidance on how to get the NavigationAction object through, I'm all ears. I've discussed this at length with Alex and Jiewen.

[John Wilander]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 412507 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412507&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        where it can be picked up in WebPageProxy::didCommitLoadForFrame().
> 
> The change log is unclear about why WebPageProxy::createNewPage() somehow
> drops the adClickAttribution which is present in the NavigationAction.

As far as I can tell, it drops NavigationAction. When we execute the completion handler in WebPageProxy::createNewPage, there is just a WebPageProxy.

[Kate Cheney]

Comment on attachment 412507 [details]
Patch

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

> Source/WebKit/ChangeLog:18
> +            Noe also checks for pending ad click attribution data in its new

Now.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4657
> +            adClickAttribution = m_newPageNavigationAdClickAttribution;

This case doesn't rely on the navigation object, should it still be inside the if (navigation) check?

> Source/WebKit/UIProcess/WebPageProxy.cpp:4664
> +        m_newPageNavigationAdClickAttribution.reset();

Why call reset() only inside this if-statement?

[John Wilander]

(In reply to katherine_cheney from comment #10)
> Comment on attachment 412507 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412507&action=review
> 
> > Source/WebKit/ChangeLog:18
> > +            Noe also checks for pending ad click attribution data in its new
> 
> Now.

Will fix.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:4657
> > +            adClickAttribution = m_newPageNavigationAdClickAttribution;
> 
> This case doesn't rely on the navigation object, should it still be inside
> the if (navigation) check?

No, I should probably move it to else if (navigation->adClickAttribution()). Thx.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:4664
> > +        m_newPageNavigationAdClickAttribution.reset();
> 
> Why call reset() only inside this if-statement?

True. Will move it outside.

Thanks! Awaiting Chris's further comments before I update the patch.

[Sam Weinig]

Comment on attachment 412507 [details]
Patch

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

> LayoutTests/http/tests/adClickAttribution/attribution-conversion-through-image-redirect-in-new-window.html:52
> +            setTimeout(tearDownAndFinish, 4000);

Using a timeout like this is likely to make the test flakey. What exactly is it that you think will take this long? Can you find specific event to wait for?

[John Wilander]

(In reply to Sam Weinig from comment #12)
> Comment on attachment 412507 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412507&action=review
> 
> > LayoutTests/http/tests/adClickAttribution/attribution-conversion-through-image-redirect-in-new-window.html:52
> > +            setTimeout(tearDownAndFinish, 4000);
> 
> Using a timeout like this is likely to make the test flakey. What exactly is
> it that you think will take this long? Can you find specific event to wait
> for?

Hi Sam! Good catch. That timeout is just a way to make the test fail after 4 seconds rather than actually time out. I added it when I was working on the fix because the test was (obviously) failing. I'll remove it.

[John Wilander]

Created attachment 412566 [details]
Patch

[John Wilander]

Created attachment 412569 [details]
Patch

[Brent Fulgham]

Comment on attachment 412569 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:4659
> +            adClickAttribution = navigation->adClickAttribution();

I'm not sure I understand this bit. It seems like the ad click attribution in m_newPageNavigationAdClickAttribution was present at the time the page load started. But now that the load is committed, we can check the navigation and see if it has attribution data.

Is the attribution data in the member variable "fresher" than the attribution generated as part of the navigation? Or are you concerned that the client might manipulate or clear the attribution state known at the start of the navigation, and that should always be the governing attribution?

This seems to say:
1. If we had attribution at the start of the navigation, use that.
2. ... but let the client possibly add attribution if it didn't exist at the start of the navigation

If that's not the goal here, we could make it a debug variable, where we have the m_newPageNavigationAdClickAttribution present in debug builds, and ASSERT if the client clears it for some reason. That might help Chris feel better about having to add the new member.

[John Wilander]

(In reply to Brent Fulgham from comment #16)
> Comment on attachment 412569 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412569&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:4659
> > +            adClickAttribution = navigation->adClickAttribution();
> 
> I'm not sure I understand this bit. It seems like the ad click attribution
> in m_newPageNavigationAdClickAttribution was present at the time the page
> load started. But now that the load is committed, we can check the
> navigation and see if it has attribution data.
> 
> Is the attribution data in the member variable "fresher" than the
> attribution generated as part of the navigation? Or are you concerned that
> the client might manipulate or clear the attribution state known at the
> start of the navigation, and that should always be the governing attribution?
> 
> This seems to say:
> 1. If we had attribution at the start of the navigation, use that.
> 2. ... but let the client possibly add attribution if it didn't exist at the
> start of the navigation
> 
> If that's not the goal here, we could make it a debug variable, where we
> have the m_newPageNavigationAdClickAttribution present in debug builds, and
> ASSERT if the client clears it for some reason. That might help Chris feel
> better about having to add the new member.

What you’re seeing is the gist of the bug fix.

In the case of a navigation in the same window, the NavigationAction object will have the ad click attribution data and when the page load is committed we send that data to the network process. That’s the existing code in trunk.

In the case of a navigation that opens a new window, we hand over to the client using WebKit which is TestRunner when we run layout tests and the browser or app itself in real usage. That’s when the NavigationAction is not carried over to the new WebPageProxy. To make sure the client can’t regress that, I now copy and ad click data to the completion handler which will execute with access to the new WebPageProxy and save it on the proxy there.

Later when we have a committed load, I check “do I have a pending ad click for a new window navigation? If not, do I have a pending ad click for a same-window navigation?”

[Brent Fulgham]

Comment on attachment 412569 [details]
Patch

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

r=me

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:4659
>>> +            adClickAttribution = navigation->adClickAttribution();
>> 
>> I'm not sure I understand this bit. It seems like the ad click attribution in m_newPageNavigationAdClickAttribution was present at the time the page load started. But now that the load is committed, we can check the navigation and see if it has attribution data.
>> 
>> Is the attribution data in the member variable "fresher" than the attribution generated as part of the navigation? Or are you concerned that the client might manipulate or clear the attribution state known at the start of the navigation, and that should always be the governing attribution?
>> 
>> This seems to say:
>> 1. If we had attribution at the start of the navigation, use that.
>> 2. ... but let the client possibly add attribution if it didn't exist at the start of the navigation
>> 
>> If that's not the goal here, we could make it a debug variable, where we have the m_newPageNavigationAdClickAttribution present in debug builds, and ASSERT if the client clears it for some reason. That might help Chris feel better about having to add the new member.
> 
> What you’re seeing is the gist of the bug fix.
> 
> In the case of a navigation in the same window, the NavigationAction object will have the ad click attribution data and when the page load is committed we send that data to the network process. That’s the existing code in trunk.
> 
> In the case of a navigation that opens a new window, we hand over to the client using WebKit which is TestRunner when we run layout tests and the browser or app itself in real usage. That’s when the NavigationAction is not carried over to the new WebPageProxy. To make sure the client can’t regress that, I now copy and ad click data to the completion handler which will execute with access to the new WebPageProxy and save it on the proxy there.
> 
> Later when we have a committed load, I check “do I have a pending ad click for a new window navigation? If not, do I have a pending ad click for a same-window navigation?”

I see. Thanks for clarifying!

[EWS]

Committed r269129: <https://trac.webkit.org/changeset/269129>

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