Bug 172578 – DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader. The rest of the methods do not.
It is unsafe it rely on the resource's loader because it gets cleared when the load completes. A resource may be reused from the memory cache once its load has completed.

This may cause crashes in CachedRawResource::didAddClient() when replaying the redirects because it would call DocumentThreadableLoader::redirectReceived() and potentially not have a loader anymore.

[Chris Dumez]

<rdar://problem/30754582>

[Chris Dumez]

Created attachment 311192 [details]
WIP Patch

[youenn fablet]

Comment on attachment 311192 [details]
WIP Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:473
> +void DocumentLoader::redirectReceived(CachedResource& resource, ResourceRequest& request, const ResourceResponse& redirectResponse, SecurityOrigin*)

I would go with some kind of enum/boolean flag.

> Source/WebCore/loader/DocumentLoader.h:327
> +    WEBCORE_EXPORT void redirectReceived(CachedResource&, ResourceRequest&, const ResourceResponse&, SecurityOrigin* newOrigin) override;

DocumentLoader should probably made final if not already and the same for redirectReceived.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:260
> +    m_origin = newOrigin;

We would create a new unique origin if passed a boolean/enum.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:268
> +    m_options.maxRedirectCount -= m_resource->redirectCount();

Is this correct in case of memory cached resources?
Maybe we should just decrement m_options.maxRedirectCount on each redirectReceived call.

> Source/WebCore/loader/MediaResourceLoader.h:86
> +    void redirectReceived(CachedResource&, ResourceRequest&, const ResourceResponse&, SecurityOrigin*) override;

Ditto for final.

[Chris Dumez]

Looks like I have a test case, I’ll clean it up and include it in my next iteration.

[Chris Dumez]

Created attachment 311240 [details]
Patch

[Chris Dumez]

Created attachment 311250 [details]
Patch

[Chris Dumez]

Fails a couple of tests:
Regressions: Unexpected text-only failures (2)
  http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr-redirect-cross-origin.html [ Failure ]
  http/tests/xmlhttprequest/redirect-cors-origin-null.html [ Failure ]

I'll investigate why.

[Build Bot]

Comment on attachment 311250 [details]
Patch

Attachment 311250 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3815539

New failing tests:
http/tests/xmlhttprequest/redirect-cors-origin-null.html
http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr-redirect-cross-origin.html

[Build Bot]

Created attachment 311258 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 311259 [details]
Patch

[youenn fablet]

Comment on attachment 311259 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:270
> +    m_options.maxRedirectCount -= m_redirectCount;

Shouldn't m_redirectCount be reset to zero here?
Can you file a bugzilla about beefing up cached redirection tests?

[Chris Dumez]

Created attachment 311276 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311276 [details]
Patch

Clearing flags on attachment: 311276

Committed r217445: <http://trac.webkit.org/changeset/217445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.