Bug 214176 (Ram)

<rdar://problem/65358005>

Hello Team, Do you have any updates on this bug? Thanks, Kartik

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

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

Created attachment 412507 [details] Patch

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.

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.

(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.

(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.

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?

(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.

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?

(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.

Created attachment 412566 [details] Patch

Created attachment 412569 [details] Patch

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.

(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?”

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!

Committed r269129: <https://trac.webkit.org/changeset/269129> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412569 [details].