http/tests/security/cors-post-redirect-307.html fails with PSON enabled

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

http/tests/security/cors-post-redirect-307.html and http/tests/security/cors-post-redirect-308.html fail with PSON enabled:
--- /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release/layout-test-results/http/tests/security/cors-post-redirect-307-expected.txt
+++ /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release/layout-test-results/http/tests/security/cors-post-redirect-307-actual.txt
@@ -1 +1 @@
-There was no origin header
+Origin header value: null

[Chris Dumez]

Looks like a legit bug in our PSON implementation. The Origin header should not be present on cross-origin redirect requests.

[Chris Dumez]

The origin gets cleared in NetworkDataTaskCocoa::willPerformHTTPRedirection() but I suspect we add the header back later on in the case where we decide to do the redirect load in a new process.

[Chris Dumez]

It is added later on by this code:
    if (origin.isEmpty()) {
        // If we don't know what origin header to attach, we attach the value
        // for an empty origin.
        request.setHTTPOrigin(SecurityOrigin::createUnique()->toString());
        return;
    }

in FrameLoader::addHTTPOriginIfNeeded().

[Chris Dumez]

Created attachment 342325 [details]
For EWS

[Chris Dumez]

Created attachment 342330 [details]
WIP Patch

[Chris Dumez]

Created attachment 342332 [details]
Patch

[Daniel Bates]

Comment on attachment 342332 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:-2835
> -    if (origin.isEmpty()) {

This code was intentionally added and a reason was given in the comment above. Please read through the comments on  <https://bugs.webkit.org/show_bug.cgi?id=20792> and ensure we are not breaking anything.

[Daniel Bates]

Obviously think about WebKit legacy.

[Chris Dumez]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 342332 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342332&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:-2835
> > -    if (origin.isEmpty()) {
> 
> This code was intentionally added and a reason was given in the comment
> above. Please read through the comments on 
> <https://bugs.webkit.org/show_bug.cgi?id=20792> and ensure we are not
> breaking anything.

Yes, I suspected this was not OK. Still uploaded it without review flag before my lab to see which tests would fail. If this is important behavior, it is sad there is no test coverage for it.

[Chris Dumez]

I have a proper fix, will upload shortly.

[Chris Dumez]

Created attachment 342454 [details]
Patch

[youenn fablet]

Comment on attachment 342454 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2839
> +        WTFReportBacktrace();

Probably to be removed.

[Chris Dumez]

(In reply to youenn fablet from comment #12)
> Comment on attachment 342454 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342454&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2839
> > +        WTFReportBacktrace();
> 
> Probably to be removed.

LoL XD

[Chris Dumez]

Created attachment 342461 [details]
Patch

[Chris Dumez]

Comment on attachment 342461 [details]
Patch

Clearing flags on attachment: 342461

Committed r232730: <https://trac.webkit.org/changeset/232730>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41018447>