Fix fetch/api/policies/referrer-origin-worker.html by adding referrer-origin-worker.html.headers, which got lost in the automated import process.
Created attachment 388287 [details] Patch
Comment on attachment 388287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388287&action=review > LayoutTests/imported/w3c/ChangeLog:10 > + automated import process. Do you know how it got lost?
(In reply to youenn fablet from comment #2) > Comment on attachment 388287 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388287&action=review > > > LayoutTests/imported/w3c/ChangeLog:10 > > + automated import process. > > Do you know how it got lost? I am not sure it got lost. It seems you imported it in 2016: https://bugs.webkit.org/show_bug.cgi?id=156914 I don't know if the import process can handle it (or could handle it in 2016) or you removed it locally. Furthermore I do understand why the lack of headers is no problem for referrer-unsafe-url-worker.html, but I don't understand why it is not a problem for referrer-no-referrer-worker.html. Anyway, since the latter passes, I do not feel like investigating. I do want to have a quick look though to see if there are more cases like fetch/api/policies/referrer-origin-worker.html.
Comment on attachment 388287 [details] Patch Clearing flags on attachment: 388287 Committed r254850: <https://trac.webkit.org/changeset/254850>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58757006>
(In reply to Rob Buis from comment #3) > (In reply to youenn fablet from comment #2) > > Comment on attachment 388287 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=388287&action=review > > > > > LayoutTests/imported/w3c/ChangeLog:10 > > > + automated import process. > > > > Do you know how it got lost? > > I am not sure it got lost. It seems you imported it in 2016: > https://bugs.webkit.org/show_bug.cgi?id=156914 > > I don't know if the import process can handle it (or could handle it in > 2016) or you removed it locally. > > I don't see that header (referrer-origin-worker.html.headers) previously imported there, neither on previous git log history to this commit. I also don't see it on WPT upstream: https://github.com/web-platform-tests/wpt/tree/master/fetch/api/policies Can you please double check where you got this header from? This test is actually failing for both Safari and WebKitGTK on wpt.fyi https://wpt.fyi/results/fetch/api/policies/referrer-origin-worker.html?label=experimental&label=master&product=chrome&product=firefox&product=safari&product=webkitgtk&aligned Perhaps its needed to upstream this header file to WPT?
(In reply to Carlos Alberto Lopez Perez from comment #7) > > I don't see that header (referrer-origin-worker.html.headers) previously > imported there, neither on previous git log history to this commit. > > I also don't see it on WPT upstream: > https://github.com/web-platform-tests/wpt/tree/master/fetch/api/policies > > Can you please double check where you got this header from? > > > This test is actually failing for both Safari and WebKitGTK on wpt.fyi > https://wpt.fyi/results/fetch/api/policies/referrer-origin-worker. > html?label=experimental&label=master&product=chrome&product=firefox&product=s > afari&product=webkitgtk&aligned > > Perhaps its needed to upstream this header file to WPT? Hmm, maybe this is the wrong way to fix this. I guess the expectation is that the Worker will fetch referrer-no-referrer.js.headers to have the Referrer-Policy set, which is not happening and is the real bug. If Youenn or anybody else can confirm that then this can be reverted. If so my apologies for the misleading statements about the importing process!
(In reply to Rob Buis from comment #8) > (In reply to Carlos Alberto Lopez Perez from comment #7) > > > > I don't see that header (referrer-origin-worker.html.headers) previously > > imported there, neither on previous git log history to this commit. > > > > I also don't see it on WPT upstream: > > https://github.com/web-platform-tests/wpt/tree/master/fetch/api/policies > > > > Can you please double check where you got this header from? > > > > > > This test is actually failing for both Safari and WebKitGTK on wpt.fyi > > https://wpt.fyi/results/fetch/api/policies/referrer-origin-worker. > > html?label=experimental&label=master&product=chrome&product=firefox&product=s > > afari&product=webkitgtk&aligned > > > > Perhaps its needed to upstream this header file to WPT? > > Hmm, maybe this is the wrong way to fix this. I guess the expectation is > that the Worker will fetch referrer-no-referrer.js.headers to have the > Referrer-Policy set, which is not happening and is the real bug. If Youenn > or anybody else can confirm that then this can be reverted. If so my > apologies for the misleading statements about the importing process! I don't know about the specific of this test and if r254850 is the right fix for it or not. But I know that we shouldn't do changes on imported WPT tests without exporting those changes to WPT itself. So if you think r254850 is the right fix, then please open a pull-request in https://github.com/web-platform-tests/wpt to upstream this header to WPT. And if you think this is not the right fix, then please revert this change. Thanks
Reopening to attach new patch.
Created attachment 388417 [details] Patch
(In reply to Carlos Alberto Lopez Perez from comment #9) > So if you think r254850 is the right fix, then please open a pull-request in > https://github.com/web-platform-tests/wpt to upstream this header to WPT. Yep, I think I know how to fix it correctly, but this was the wrong way. I tried a webkit-patch rollout but it seemed to get stuck in an endless loop, so I attached the revert by hand. Are you able to review it?
Comment on attachment 388417 [details] Patch Clearing flags on attachment: 388417 Committed r254920: <https://trac.webkit.org/changeset/254920>
I'm confused by the state of this bug. Looks like the fix was reverted and the test expectations are FAIL again, so why is it in RESOLVED/FIXED state?
Still needs to be fixed.
(In reply to Alexey Proskuryakov from comment #15) > I'm confused by the state of this bug. Looks like the fix was reverted and > the test expectations are FAIL again, so why is it in RESOLVED/FIXED state? Right, the automatic cq+ behavior was to close this bug, I re-opened it since the test is not fixed yet indeed. I believe I have a patch for this which I can upload today.
Created attachment 388522 [details] Patch
Created attachment 388552 [details] Patch
Created attachment 388567 [details] Patch
Comment on attachment 388567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388567&action=review > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:58 > + , m_referrerPolicy(referrerPolicy) Can we directly pass a ReferrerPolicy there instead of a String? Also, can we move this to WorkerGlobalScope to increase code reuse? Maybe > Source/WebCore/workers/Worker.cpp:214 > + m_contextProxy.startWorkerGlobalScope(m_scriptLoader->url(), m_name, context->userAgent(m_scriptLoader->url()), isOnline, m_scriptLoader->script(), contentSecurityPolicyResponseHeaders, m_shouldBypassMainWorldContentSecurityPolicy, m_workerCreationTime, m_scriptLoader->referrerPolicy(), m_runtimeFlags); Can we make ScriptLoader return a ReferrerPolicy? > Source/WebCore/workers/WorkerThread.cpp:107 > + , m_referrerPolicy(referrerPolicy) All other strings are isolated, so we should probably do the same for referrer policy, but an enum might be event better here.
Created attachment 388694 [details] Patch
Created attachment 388700 [details] Patch
Created attachment 388701 [details] Patch
Comment on attachment 388567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388567&action=review Went with using ReferrerPolicy for the parameters. >> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:58 >> + , m_referrerPolicy(referrerPolicy) > > Can we directly pass a ReferrerPolicy there instead of a String? > Also, can we move this to WorkerGlobalScope to increase code reuse? > Maybe Done. I initially did it using Strings because a corner case would be slower, namely the parsing would have been done needlessly if a fetch has the referrerPolicy parameter in its RequestInit. But this should not happen much in reality. >> Source/WebCore/workers/Worker.cpp:214 >> + m_contextProxy.startWorkerGlobalScope(m_scriptLoader->url(), m_name, context->userAgent(m_scriptLoader->url()), isOnline, m_scriptLoader->script(), contentSecurityPolicyResponseHeaders, m_shouldBypassMainWorldContentSecurityPolicy, m_workerCreationTime, m_scriptLoader->referrerPolicy(), m_runtimeFlags); > > Can we make ScriptLoader return a ReferrerPolicy? Unfortunately I think that makes jobFinishedLoadingScript and storing into ServiceWorkerFetchResult too complicated/inefficient. >> Source/WebCore/workers/WorkerThread.cpp:107 >> + , m_referrerPolicy(referrerPolicy) > > All other strings are isolated, so we should probably do the same for referrer policy, but an enum might be event better here. You are right, but not needed if we go with ReferrerPolicy.
Comment on attachment 388694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388694&action=review > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:56 > + : WorkerGlobalScope(url, WTFMove(origin), identifier, userAgent, isOnline, thread, shouldBypassMainWorldContentSecurityPolicy, WTFMove(topOrigin), timeOrigin, referrerPolicy, connectionProxy, socketProvider) s/const ReferrerPolicy&/ReferrerPolicy/ here and elsewhere > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:98 > + return m_referrerPolicy; We should be able to implement it in WorkerGlobalScope directly. > Source/WebCore/workers/WorkerGlobalScope.h:146 > + mutable ReferrerPolicy m_referrerPolicy; Why adding mutable? No need to make it protected, we can implement referrerPolicy getter in WorkerGlobalScope and make m_referrerPolicy private. > Source/WebCore/workers/WorkerThread.cpp:91 > + ReferrerPolicy m_referrerPolicy; Strange that we use m_XX for struct fields. Might be good to fix this style issue as a follow-up. > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:163 > + return m_referrerPolicy; Can we just compute this value from data and pass it to WorkerGlobalScope constructor? > Source/WebCore/worklets/WorkletGlobalScope.cpp:171 > + return ReferrerPolicy::NoReferrer; We should probably return m_referrerPolicy and make it final.
Created attachment 388708 [details] Patch
Comment on attachment 388694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388694&action=review >> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:56 >> + : WorkerGlobalScope(url, WTFMove(origin), identifier, userAgent, isOnline, thread, shouldBypassMainWorldContentSecurityPolicy, WTFMove(topOrigin), timeOrigin, referrerPolicy, connectionProxy, socketProvider) > > s/const ReferrerPolicy&/ReferrerPolicy/ here and elsewhere I believe I used const ReferrerPolicy& in all parameter lists. >> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:98 >> + return m_referrerPolicy; > > We should be able to implement it in WorkerGlobalScope directly. Ah, that is what I did not get, done. >> Source/WebCore/workers/WorkerGlobalScope.h:146 >> + mutable ReferrerPolicy m_referrerPolicy; > > Why adding mutable? > No need to make it protected, we can implement referrerPolicy getter in WorkerGlobalScope and make m_referrerPolicy private. Nice, as I said above I did not see this, done. >> Source/WebCore/workers/WorkerThread.cpp:91 >> + ReferrerPolicy m_referrerPolicy; > > Strange that we use m_XX for struct fields. Might be good to fix this style issue as a follow-up. Sure, can do. >> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:163 >> + return m_referrerPolicy; > > Can we just compute this value from data and pass it to WorkerGlobalScope constructor? Done. >> Source/WebCore/worklets/WorkletGlobalScope.cpp:171 >> + return ReferrerPolicy::NoReferrer; > > We should probably return m_referrerPolicy and make it final. I made it final. There is no m_referrerPolicy, WorkletGlobalScope does not derive from WorkerGlobalScope.
Comment on attachment 388694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388694&action=review >>> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:56 >>> + : WorkerGlobalScope(url, WTFMove(origin), identifier, userAgent, isOnline, thread, shouldBypassMainWorldContentSecurityPolicy, WTFMove(topOrigin), timeOrigin, referrerPolicy, connectionProxy, socketProvider) >> >> s/const ReferrerPolicy&/ReferrerPolicy/ here and elsewhere > > I believe I used const ReferrerPolicy& in all parameter lists. Oops, I misread the suggestion, will fix tomorrow.
Created attachment 388775 [details] Patch
Comment on attachment 388708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388708&action=review > Source/WebCore/workers/DedicatedWorkerGlobalScope.h:53 > + static Ref<DedicatedWorkerGlobalScope> create(const URL&, Ref<SecurityOrigin>&&, const String& name, const String& identifier, const String& userAgent, bool isOnline, DedicatedWorkerThread&, const ContentSecurityPolicyResponseHeaders&, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, const ReferrerPolicy&, IDBClient::IDBConnectionProxy*, SocketProvider*); I think we need to move to a structure. This is simply too many arguments to a function. > Source/WebCore/workers/DedicatedWorkerThread.h:53 > + Ref<WorkerGlobalScope> createWorkerGlobalScope(const URL&, Ref<SecurityOrigin>&&, const String& name, const String& identifier, const String& userAgent, bool isOnline, const ContentSecurityPolicyResponseHeaders&, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, const ReferrerPolicy&) override; Ditto.
Created attachment 388856 [details] Patch
Created attachment 388860 [details] Patch
Created attachment 388882 [details] Patch
Comment on attachment 388708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388708&action=review >> Source/WebCore/workers/DedicatedWorkerGlobalScope.h:53 >> + static Ref<DedicatedWorkerGlobalScope> create(const URL&, Ref<SecurityOrigin>&&, const String& name, const String& identifier, const String& userAgent, bool isOnline, DedicatedWorkerThread&, const ContentSecurityPolicyResponseHeaders&, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, const ReferrerPolicy&, IDBClient::IDBConnectionProxy*, SocketProvider*); > > I think we need to move to a structure. This is simply too many arguments to a function. Turned out to be quite some work somehow. Done. >> Source/WebCore/workers/DedicatedWorkerThread.h:53 >> + Ref<WorkerGlobalScope> createWorkerGlobalScope(const URL&, Ref<SecurityOrigin>&&, const String& name, const String& identifier, const String& userAgent, bool isOnline, const ContentSecurityPolicyResponseHeaders&, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, const ReferrerPolicy&) override; > > Ditto. Done.
Comment on attachment 388882 [details] Patch Clearing flags on attachment: 388882 Committed r256012: <https://trac.webkit.org/changeset/256012>