Bug 109753 – CORS preflight broken with NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

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>

[Alexey Proskuryakov]

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.

[Brady Eidson]

Yes, please, do this.  I have a related bug.  Please let me know when the patch is ready I'll review asap :)

[Alexey Proskuryakov]

Created attachment 188253 [details]
WIP patch

This breaks at least one inspector test, let's see what EWS thinks about other tests.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Vsevolod Vlasov]

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

[Alexey Proskuryakov]

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.

[WebKit Review Bot]

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.

[Brady Eidson]

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

[Alexey Proskuryakov]

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?