RESOLVED FIXED 172578
DocumentThreadableLoader::redirectReceived() should not rely on the resource's loader 
https://bugs.webkit.org/show_bug.cgi?id=172578
Summary DocumentThreadableLoader::redirectReceived() should not rely on the resource'...
Chris Dumez
Reported 2017-05-24 21:25:04 PDT
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.
Attachments
WIP Patch (9.80 KB, patch)
2017-05-24 21:26 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (7.05 KB, patch)
2017-05-25 10:28 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (7.08 KB, patch)
2017-05-25 11:15 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (871.26 KB, application/zip)
2017-05-25 12:06 PDT, Build Bot 		no flags
Details
Patch (7.13 KB, patch)
2017-05-25 12:12 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (7.16 KB, patch)
2017-05-25 13:15 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2017-05-24 21:25:46 PDT
<rdar://problem/30754582>
Chris Dumez
Comment 2 2017-05-24 21:26:58 PDT
Created attachment 311192 [details] WIP Patch
youenn fablet
Comment 3 2017-05-24 21:55:57 PDT
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
Comment 4 2017-05-25 09:53:39 PDT
Looks like I have a test case, I’ll clean it up and include it in my next iteration.
Chris Dumez
Comment 5 2017-05-25 10:28:55 PDT
Created attachment 311240 [details] Patch
Chris Dumez
Comment 6 2017-05-25 11:15:07 PDT
Created attachment 311250 [details] Patch
Chris Dumez
Comment 7 2017-05-25 11:15:32 PDT
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 8 2017-05-25 12:06:14 PDT
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
Comment 9 2017-05-25 12:06:16 PDT
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
Comment 10 2017-05-25 12:12:23 PDT
Created attachment 311259 [details] Patch
youenn fablet
Comment 11 2017-05-25 13:07:56 PDT
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
Comment 12 2017-05-25 13:15:47 PDT
Created attachment 311276 [details] Patch
WebKit Commit Bot
Comment 13 2017-05-25 13:53:52 PDT
Comment on attachment 311276 [details] Patch Clearing flags on attachment: 311276 Committed r217445: <http://trac.webkit.org/changeset/217445>
WebKit Commit Bot
Comment 14 2017-05-25 13:53:54 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.