Bug 109753

Summary: CORS preflight broken with NetworkProcess
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Page LoadingAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, dglazkov, japhet, pfeldman, vsevik, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP patch
webkit.review.bot: commit-queue-
proposed fix beidson: review+

Description Alexey Proskuryakov 2013-02-13 14:56:51 PST
NetworkProcess needs all loaders to have identifiers. But we don't currently assign an identifier if we don't plan to make FrameLoaderClient calls, such as with CORS preflight requests.

<rdar://problem/13210723>
Comment 1 Alexey Proskuryakov 2013-02-13 15:42:42 PST
One way to reproduce is with http/tests/xmlhttprequest/access-control-basic-non-simple-allow-async.html

This is a bit tricky, because there is a lot of code written to work around the current behavior instead of fixing it. DocumentThreadableLoader::loadRequest second-guesses what a lower level would do, and preemptively generates a fake identifier (it also has an incorrect comment explaining why it does that):

#if ENABLE(INSPECTOR)
        if (m_actualRequest) {
            // Because willSendRequest only gets called during redirects, we initialize the identifier and the first willSendRequest here.
            m_preflightRequestIdentifier = m_document->frame()->page()->progress()->createUniqueIdentifier();
            ResourceResponse redirectResponse = ResourceResponse();
            InspectorInstrumentation::willSendRequest(m_document->frame(), m_preflightRequestIdentifier, m_document->frame()->loader()->documentLoader(), newRequest.mutableResourceRequest(), redirectResponse);
        }
#endif

It's not true that willSendRequest only gets called during redirects - this function gets called unconditionally. So we should get rid of m_preflightRequestIdentifier, and just always generate a real ResourceLoader identifier.
Comment 2 Brady Eidson 2013-02-13 16:05:23 PST
Yes, please, do this.  I have a related bug.  Please let me know when the patch is ready I'll review asap :)
Comment 3 Alexey Proskuryakov 2013-02-13 20:07:23 PST
Created attachment 188253 [details]
WIP patch

This breaks at least one inspector test, let's see what EWS thinks about other tests.
Comment 4 WebKit Review Bot 2013-02-13 21:22:37 PST
Comment on attachment 188253 [details]
WIP patch

Attachment 188253 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16514485

New failing tests:
http/tests/inspector/network-preflight-options.html
Comment 5 WebKit Review Bot 2013-02-13 22:28:32 PST
Comment on attachment 188253 [details]
WIP patch

Attachment 188253 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16537467

New failing tests:
http/tests/inspector/network-preflight-options.html
Comment 6 Vsevolod Vlasov 2013-02-14 02:15:45 PST
Comment on attachment 188253 [details]
WIP patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:315
> +    if (m_actualRequest) {

m_actualRequest is reset to nullptr in preflightFailure(). This is the reason inspector does not receive didFinishLoading/didFailLoading events in case of failed asynchronous preflight request and the test fails.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:329
> +    if (m_actualRequest)

Ditto
Comment 7 Alexey Proskuryakov 2013-02-14 11:50:22 PST
Created attachment 188393 [details]
proposed fix

This doesn't make everything beautiful, but should be an improvement overall, in addition to fixing the bug. One thing that should improve a little is that Inspector should be getting an accurate error when CORS check fails now (it used to be a cancellation error instead).

Perhaps it would now be possible to move all InspectorInstrumentation calls to ResourceLoader now, whether it's doing preflight or not.
Comment 8 WebKit Review Bot 2013-02-14 11:52:59 PST
Attachment 188393 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/DocumentThreadableLoader.cpp', u'Source/WebCore/loader/DocumentThreadableLoader.h', u'Source/WebCore/loader/ResourceLoader.cpp', u'Source/WebCore/loader/TextTrackLoader.cpp', u'Source/WebCore/loader/TextTrackLoader.h', u'Source/WebCore/loader/cache/CachedResourceClient.h', u'Source/WebCore/loader/cache/CachedTextTrack.cpp']" exit_code: 1
Source/WebCore/loader/DocumentThreadableLoader.cpp:356:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Brady Eidson 2013-02-14 16:17:15 PST
Comment on attachment 188393 [details]
proposed fix

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

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:356
>> +    if (m_actualRequest) {
>> +        InspectorInstrumentation::didFailLoading(m_document->frame(), m_document->frame()->loader()->documentLoader(), identifier, error);
>> +    }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

Listen to style-bot here
Comment 10 Alexey Proskuryakov 2013-02-14 16:31:01 PST
Committed <http://trac.webkit.org/changeset/142936>.

> Perhaps it would now be possible to move all InspectorInstrumentation calls to ResourceLoader now, whether it's doing preflight or not.

The more I think about it, the more it seems to me that this would greatly simplify things. And we'd be able to remove those identifier parameters that I just added.

Vsevolod, would you be willing to explore this idea?