Bug 206520 – Fix fetch/api/policies/referrer-origin-worker.html

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rob Buis]

Fix fetch/api/policies/referrer-origin-worker.html by adding
referrer-origin-worker.html.headers, which got lost in the
automated import process.

[Rob Buis]

Created attachment 388287 [details]
Patch

[youenn fablet]

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?

[Rob Buis]

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

[WebKit Commit Bot]

Comment on attachment 388287 [details]
Patch

Clearing flags on attachment: 388287

Committed r254850: <https://trac.webkit.org/changeset/254850>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58757006>

[Carlos Alberto Lopez Perez]

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

[Rob Buis]

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

[Carlos Alberto Lopez Perez]

(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

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 388417 [details]
Patch

[Rob Buis]

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

[WebKit Commit Bot]

Comment on attachment 388417 [details]
Patch

Clearing flags on attachment: 388417

Committed r254920: <https://trac.webkit.org/changeset/254920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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?

[Rob Buis]

Still needs to be fixed.

[Rob Buis]

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

[Rob Buis]

Created attachment 388522 [details]
Patch

[Rob Buis]

Created attachment 388552 [details]
Patch

[Rob Buis]

Created attachment 388567 [details]
Patch

[youenn fablet]

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.

[Rob Buis]

Created attachment 388694 [details]
Patch

[Rob Buis]

Created attachment 388700 [details]
Patch

[Rob Buis]

Created attachment 388701 [details]
Patch

[Rob Buis]

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.

[youenn fablet]

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.

[Rob Buis]

Created attachment 388708 [details]
Patch

[Rob Buis]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 388775 [details]
Patch

[Darin Adler]

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.

[Rob Buis]

Created attachment 388856 [details]
Patch

[Rob Buis]

Created attachment 388860 [details]
Patch

[Rob Buis]

Created attachment 388882 [details]
Patch

[Rob Buis]

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.

[WebKit Commit Bot]

Comment on attachment 388882 [details]
Patch

Clearing flags on attachment: 388882

Committed r256012: <https://trac.webkit.org/changeset/256012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.