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>
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.
Yes, please, do this. I have a related bug. Please let me know when the patch is ready I'll review asap :)
Created attachment 188253 [details] WIP patch This breaks at least one inspector test, let's see what EWS thinks about other tests.
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 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 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
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.
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 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
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?