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.
<rdar://problem/30754582>
Created attachment 311192 [details] WIP Patch
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.
Looks like I have a test case, I’ll clean it up and include it in my next iteration.
Created attachment 311240 [details] Patch
Created attachment 311250 [details] Patch
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.
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
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
Created attachment 311259 [details] Patch
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?
Created attachment 311276 [details] Patch
Comment on attachment 311276 [details] Patch Clearing flags on attachment: 311276 Committed r217445: <http://trac.webkit.org/changeset/217445>
All reviewed patches have been landed. Closing bug.