Bug 112531 – Network Panel: .swf file is always presented as loading by XHR

• Status: RESOLVED
• Resolution: INVALID
• Component: Web Inspector (Deprecated)

[pdeng6]

Investigating.

[pdeng6]

Created attachment 193486 [details]
swf in Network panel

[pdeng6]

Created attachment 193540 [details]
Patch

[pdeng6]

Root cause is that view of Resource type is different between CachedResource and Inspector. 
Currently, CachedRawResource are mapped to Inspector::XHRResource, while request from plugin is also another client of CachedRawResource, so that .swf and other resource initiated from plug-in are also mapped to XHRResource.

To avoid change the type view of CachedResource, I suggest to restrict RawResource => XHRResource mapping by specifying explicit XHR target.


thanks
Pan

[Vsevolod Vlasov]

Comment on attachment 193540 [details]
Patch

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

> Source/WebCore/inspector/InspectorPageAgent.cpp:322
> +        if (const_cast<CachedResource&>(cachedResource).resourceRequest().targetType() == ResourceRequest::TargetIsXHR)

Platform specific code is not acceptable here.
TargetIsXHR is supposed to be used in chromium network stack and should not be used by inspector.

[Vsevolod Vlasov]

Distinguishing between plugins and XHRs is tricky and I don't see a straightforward solution here.

[pdeng6]

(In reply to comment #5)
> Distinguishing between plugins and XHRs is tricky and I don't see a straightforward solution here.

I found that InspectorPageAgent::XHRResource was set in m_resourcesData during didReceiveXHRResponse, which is always happen before didReceiveResponse, then we may eliminate the plugin according to that type. (there are similar story for checking type of InspectorPageAgent::ScriptResource).
I will prepare a patch for that. :)

thanks
Pan

[pdeng6]

Created attachment 193764 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 193764 [details]
Patch

This way OPTIONS preflight requests will get Other type for XHRs.

[pdeng6]

Created attachment 194208 [details]
Patch

[pdeng6]

(In reply to comment #8)
> (From update of attachment 193764 [details])
> This way OPTIONS preflight requests will get Other type for XHRs.

thank you very much!
Right, preflight of xhr will get "Other" type in that way.
As preflight is transparent to loader client, I didn't find a way to know who it works for in current DocumentThreadLoader :(
So I added a method to know the type of ThreadableLoaderClient is XHR or not, and if it is true, tell ResourceAgent the type is "xhr" when receive the xhr preflight.
a test is also attached to make sure xhr preflight type.

thanks!
Pan

[Vsevolod Vlasov]

Comment on attachment 194208 [details]
Patch

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

You might want to discuss this with ap@ or japhet@. This would probably require adding new CachedResource type. I still think implementing this would be too tricky and the value added here is negligible.

> Source/WebCore/loader/ThreadableLoaderClient.h:54
> +        virtual bool isXMLHttpRequestClient() { return false; }

This is a layering violation.

[pdeng6]

Created attachment 195476 [details]
XHR category before fix, a bunch of non-xhr living there

[pdeng6]

Created attachment 195477 [details]
XHR category after fix, only two real xhr should live there

[pdeng6]

(In reply to comment #11)
> (From update of attachment 194208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194208&action=review
> 
> You might want to discuss this with ap@ or japhet@. This would probably require adding new CachedResource type. I still think implementing this would be too tricky and the value added here is negligible.
> 
> > Source/WebCore/loader/ThreadableLoaderClient.h:54
> > +        virtual bool isXMLHttpRequestClient() { return false; }
> 
> This is a layering violation.

I think we would better fix this, I uploaded two snapshots to access Youtube, there are a bunch of non-XHR resources are classified as XHR type.
I'd like to achieve a perfect patch solve this, but seems not easy.
@ap and japhet, any comments about the loader part?

thanks
Pan

[Nate Chapin]

Comment on attachment 194208 [details]
Patch

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

>>> Source/WebCore/loader/ThreadableLoaderClient.h:54
>>> +        virtual bool isXMLHttpRequestClient() { return false; }
>> 
>> This is a layering violation.
> 
> I think we would better fix this, I uploaded two snapshots to access Youtube, there are a bunch of non-XHR resources are classified as XHR type.
> I'd like to achieve a perfect patch solve this, but seems not easy.
> @ap and japhet, any comments about the loader part?
> 
> thanks
> Pan

I'm not entirely convinced this is a layering violation. At least, it's no more of a violation than isDocumentThreadableLoaderClient() on the line above it.

[Alexey Proskuryakov]

I also don't think that this is a layering violation. But the name should be "isXMLHttpRequest", not "isXMLHttpRequestClient".

However, the design mildly worries me. Inspector should ideally have a better abstraction, and not get to low level implementation details like ThreadableLoaderClient.

[pdeng6]

(In reply to comment #16)
> I also don't think that this is a layering violation. But the name should be "isXMLHttpRequest", not "isXMLHttpRequestClient".
> 
> However, the design mildly worries me. Inspector should ideally have a better abstraction, and not get to low level implementation details like ThreadableLoaderClient.

thanks for your comments :)
Ideally, Inspector should instrument in real client XMLHttpRequest.cpp for detailed information for this, however, pre-flight is transparent, never pass through ThreadableClient.
I'm thinking another solution may be, add a virtual method didReceivePreflightResponse to ThreadableLoaderClient, default do nothing, and private override it in XMLHttpRequest, InspectorInstrumentation will be there.
Although only inspector works in the override function, XMLhttpRequest's client won't aware it as it's private.
your comments?

thanks
Pan

[Alexey Proskuryakov]

> pre-flight is transparent, never pass through ThreadableClient

XMLHttpRequest.cpp code may run on a secondary thread, or even in a separate process in the case of shared workers. All network loads that it does have to pass through ThreadableClient.

[pdeng6]

(In reply to comment #18)
> > pre-flight is transparent, never pass through ThreadableClient
> 
> XMLHttpRequest.cpp code may run on a secondary thread, or even in a separate process in the case of shared workers. All network loads that it does have to pass through ThreadableClient.

thanks for your comments.
I mean when ThreadableLoader(DocumentThreadableLoader) receives a pre-flight response, it won't tell ThreadableClient that have happened. right?
I will upload a patch to demonstrate. :)

thanks
Pan

[pdeng6]

Created attachment 196055 [details]
Patch

[Build Bot]

Comment on attachment 196055 [details]
Patch

Attachment 196055 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17336440

New failing tests:
media/video-controls-fullscreen-volume.html

[Build Bot]

Created attachment 196061 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[pdeng6]

(In reply to comment #22)
> Created an attachment (id=196061) [details]
> Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

we are innocent for this failure.

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.

[Csaba Osztrogonác]

Comment on attachment 196055 [details]
Patch

Cleared review? from attachment 196055 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).