Summary: | Resource Load Statistics: Downgrade document.referrer to the referrer's eTLD+1 if the page was navigated to with a prevalent resource referrer containing link decoration | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||
Component: | WebKit Misc. | Assignee: | John Wilander <wilander> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, ehsan, senglehardt, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
John Wilander
2019-05-24 12:48:28 PDT
Created attachment 370601 [details]
Patch
Created attachment 370605 [details]
Patch
WPE build fix. Comment on attachment 370605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370605&action=review r=me with a few comments. > Source/WebCore/page/CrossSiteNavigationDataTransfer.h:34 > + enum Flag { enum class Flag : uint8_t > Source/WebKit/UIProcess/WebProcessPool.cpp:2528 > + m_networkProcess->didCommitCrossSiteLoadWithDataTransfer(sessionID, fromDomain, toDomain, navigationDataTransfer, pageID, [] { }); I don't understand why this completion handler is necessary. Could we remove it and have this message just be a notification? That would make this code a lot simpler. > LayoutTests/http/tests/resourceLoadStatistics/downgraded-referrer-for-navigation-with-link-query-from-prevalent-resource.html:14 > + if (document.referrer === prevalentResourceOrigin + "/") If this test is flaky, we should spin until this is true. I hope it's not flaky, but I think it's currently based on a race condition. Created attachment 370658 [details]
Patch for landing
Thank you so much for pitching in on a Saturday, Alex! (In reply to Alex Christensen from comment #5) > Comment on attachment 370605 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370605&action=review > > r=me with a few comments. > > > Source/WebCore/page/CrossSiteNavigationDataTransfer.h:34 > > + enum Flag { > > enum class Flag : uint8_t Fixed. > > Source/WebKit/UIProcess/WebProcessPool.cpp:2528 > > + m_networkProcess->didCommitCrossSiteLoadWithDataTransfer(sessionID, fromDomain, toDomain, navigationDataTransfer, pageID, [] { }); > > I don't understand why this completion handler is necessary. Could we > remove it and have this message just be a notification? That would make > this code a lot simpler. I put there in anticipation of building test infrastructure but the only one needed is for WebResourceLoadStatisticsStore::logCrossSiteLoadWithLinkDecoration() which is called from NetworkProcess::setCrossSiteLoadWithLinkDecorationForTesting(). I removed the others. > > LayoutTests/http/tests/resourceLoadStatistics/downgraded-referrer-for-navigation-with-link-query-from-prevalent-resource.html:14 > > + if (document.referrer === prevalentResourceOrigin + "/") > > If this test is flaky, we should spin until this is true. I hope it's not > flaky, but I think it's currently based on a race condition. Good call. Fixed. Comment on attachment 370658 [details] Patch for landing Clearing flags on attachment: 370658 Committed r245784: <https://trac.webkit.org/changeset/245784> All reviewed patches have been landed. Closing bug. Do I understand correctly that this change only changes the script visible value of document.referrer and not the network visible value (e.g. the value that gets used here: https://html.spec.whatwg.org/#navigating-across-documents:the-document's-referrer)? (In reply to Ehsan Akhgari [:ehsan] from comment #10) > Do I understand correctly that this change only changes the script visible > value of document.referrer and not the network visible value (e.g. the value > that gets used here: > https://html.spec.whatwg.org/#navigating-across-documents:the-document's- > referrer)? Yes, this only affects the JavaScript API, not the protocol part. |