inspector/timeline-network-resource.html fails when run twice

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

inspector/timeline-network-resource.html fails when run twice


--- /tmp/layout-test-results/inspector/timeline-network-resource-expected.txt	2010-04-10 16:09:53.000000000 -0700
+++ /tmp/layout-test-results/inspector/timeline-network-resource-actual.txt	2010-04-10 16:09:53.000000000 -0700
@@ -13,21 +13,6 @@
 + usedHeapSize : * DEFINED *
 + totalHeapSize : * DEFINED *
 
-ResourceReceiveResponse Properties:
-+ startTime : * DEFINED *
-+ data : {
-+- identifier : * DEFINED *
-+- statusCode : 0
-+- mimeType : * DEFINED *
-+- expectedContentLength : 210
-+- url : * DEFINED *
-+ }
-+ children : * DEFINED *
-+ endTime : * DEFINED *
-+ type : 13
-+ usedHeapSize : * DEFINED *
-+ totalHeapSize : * DEFINED *
-
 ResourceFinish Properties:
 + startTime : * DEFINED *
 + data : {

http://trac.webkit.org/browser/trunk/LayoutTests/inspector/timeline-network-resource.html

[Eric Seidel (no email)]

run-webkit-tests inspector/timeline-network-resource.html inspector/timeline-network-resource.html
will reproduce the failure.

I noticed this while trying to redprocue bug 36946.

[Yury Semikhatsky]

O c

[Yury Semikhatsky]

I was running this test on Linux Gtk Debug and it would fail every second run with the same diff as Eric posted.

[Yury Semikhatsky]

Created attachment 77547 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 77547 [details]
Patch

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

looks good to me

> LayoutTests/inspector/timeline-network-resource.html:19
> +function runAfterScriptIsEvaluated()
>  {
> -    function step()
> -    {
> -        if (!window.scriptEvaluated)
> -            setTimeout(step, 100);
> -        else
> -            continuation();
> -    }
> -    setTimeout(step, 100);
> +      printTimelineRecords(null, null, function(record) {
> +          if (record.type === timelineAgentRecordType["ResourceSendRequest"])
> +              printSend(record);
> +          else if (record.type === timelineAgentRecordType["ResourceReceiveResponse"])
> +              printReceive(record);
> +          else if (record.type === timelineAgentRecordType["ResourceFinish"])
> +              printFinish(record);
> +      });
>  }

wrong indentation

> LayoutTests/inspector/timeline-network-resource.js:6
>  window.scriptEvaluated = true;

is not required anymore

[Yury Semikhatsky]

Created attachment 77608 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #6)
> Created an attachment (id=77608) [details]
> Patch

The same patch with all comments addressed and the small refactoring that was discussed offline.

[Yury Semikhatsky]

Created attachment 77609 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #8)
> Created an attachment (id=77609) [details]
> Patch

ChangeLog cleanup.

[WebKit Review Bot]

Attachment 77608 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7205246

[WebKit Review Bot]

Attachment 77609 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7231237

[Pavel Feldman]

Comment on attachment 77609 [details]
Patch

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

> WebCore/inspector/InspectorController.cpp:803
> +    request.setReportLoadTiming(true);

This call should be made in case of no front-end as well. Otherwise WebTiming breaks. Condition should be: enable load timing in case of (isMainResource || m_frontend).

> WebCore/inspector/InspectorController.cpp:807
> +    request.setReportRawHeaders(true);

raw headers should be collected only for the case of m_frontend.

> WebCore/inspector/InspectorController.h:280
> +    void willSendRequest(unsigned long identifier, ResourceRequest&, const ResourceResponse& redirectResponse);

Please make sure that order in cpp file matches the order in header.

> WebCore/inspector/InspectorInstrumentation.cpp:380
> +    ic->ensureSettingsLoaded();

Could you add a comment on why this call is important to make here?

> WebCore/inspector/InspectorInstrumentation.cpp:427
> +InspectorInstrumentationCookie InspectorInstrumentation::willReceiveResourceResponseImpl(InspectorController* inspectorController, unsigned long identifier, DocumentLoader* loader, const ResourceResponse& response)

Should willReceiveResponse call didReceiveResponse in inspector?

> WebCore/inspector/InspectorInstrumentation.cpp:437
> +        resourceAgent->didReceiveResponse(identifier, loader, response);
> +    inspectorController->didReceiveResponse(identifier, response);

I feel that we should inline addConsoleMessage here instead. No need to make inspector controller involved.

> WebCore/inspector/InspectorInstrumentation.cpp:463
> +    ic->didFailLoading(identifier, error);

ditto

> WebCore/inspector/InspectorInstrumentation.cpp:472
> +    ic->resourceRetrievedByXMLHttpRequest(url, sendURL, sendLineNumber);

Almost ditto, but depends on inspector state, I know...

> WebCore/inspector/InspectorInstrumentation.h:458
> +    if (!frame)

Are you not using inspectorControllerForFrame in order to get called in case of no front-ends? You should then be explicit and document it here.

> WebCore/inspector/InspectorInstrumentation.h:475
> +#if ENABLE(INSPECTOR)

We only inline methods and delegate to Impl in case we do a fast InspectorController fetch. I don't see how this inline helps.

> WebCore/inspector/InspectorInstrumentation.h:483
> +    didLoadResourceFromMemoryCacheImpl(page->inspectorController(), loader, resource);

ditto

> WebCore/inspector/front-end/NetworkManager.js:92
> +            this.mainResourceIdentifier = identifier;

Now mainResourceIdentifier might conflict mainResource defined in didCommitLoadForFrame. You should instead mark resource as main here and assign WebInspector.mainResource to it (and nuke didCommitLoadForFrame).

> WebCore/loader/ResourceLoadNotifier.cpp:69
> +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willReceiveResourceResponse(loader->frameLoader()->frame(), loader->identifier(), loader->documentLoader(), r);

We are now going to receive dupes / miss notifications. Please do not modify original inspector controller resource reporting in this change.

> WebCore/loader/ResourceLoadNotifier.cpp:-139
> -        page->inspectorController()->didReceiveResponse(identifier, loader, r);

This clearly is a regression - you should call InspectorInstrumentation here.

> WebCore/loader/ResourceLoadNotifier.cpp:158
> +        InspectorInstrumentationCookie cookie = InspectorInstrumentation::willReceiveResourceResponse(loader->frameLoader()->frame(), identifier, loader, response);

You should not make calls to inspector controller in the new places.

[Yury Semikhatsky]

Created attachment 78529 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #12)
> (From update of attachment 77609 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77609&action=review
> 
> > WebCore/inspector/InspectorController.cpp:803
> > +    request.setReportLoadTiming(true);
> 
> This call should be made in case of no front-end as well. Otherwise WebTiming breaks. Condition should be: enable load timing in case of (isMainResource || m_frontend).
> 
Done.

> > WebCore/inspector/InspectorController.cpp:807
> > +    request.setReportRawHeaders(true);
> 
> raw headers should be collected only for the case of m_frontend.
> 
Done.

> > WebCore/inspector/InspectorController.h:280
> > +    void willSendRequest(unsigned long identifier, ResourceRequest&, const ResourceResponse& redirectResponse);
> 
> Please make sure that order in cpp file matches the order in header.
> 
Done.

> > WebCore/inspector/InspectorInstrumentation.cpp:380
> > +    ic->ensureSettingsLoaded();
> 
> Could you add a comment on why this call is important to make here?
> 
I believe we don't need this call here at all. It should be done in connectFrontend and on XHR response.


> > WebCore/inspector/InspectorInstrumentation.cpp:427
> > +InspectorInstrumentationCookie InspectorInstrumentation::willReceiveResourceResponseImpl(InspectorController* inspectorController, unsigned long identifier, DocumentLoader* loader, const ResourceResponse& response)
> 
> Should willReceiveResponse call didReceiveResponse in inspector?
> 
Fixed. 

> > WebCore/inspector/InspectorInstrumentation.cpp:437
> > +        resourceAgent->didReceiveResponse(identifier, loader, response);
> > +    inspectorController->didReceiveResponse(identifier, response);
> 
> I feel that we should inline addConsoleMessage here instead. No need to make inspector controller involved.
> 
I'd rather move console-related stuff into console agent in the next change.

> > WebCore/inspector/InspectorInstrumentation.cpp:463
> > +    ic->didFailLoading(identifier, error);
> 
> ditto
> 
ditto

> > WebCore/inspector/InspectorInstrumentation.cpp:472
> > +    ic->resourceRetrievedByXMLHttpRequest(url, sendURL, sendLineNumber);
> 
> Almost ditto, but depends on inspector state, I know...
> 
> > WebCore/inspector/InspectorInstrumentation.h:458
> > +    if (!frame)
> 
> Are you not using inspectorControllerForFrame in order to get called in case of no front-ends? You should then be explicit and document it here.
>
Done.
 
> > WebCore/inspector/InspectorInstrumentation.h:475
> > +#if ENABLE(INSPECTOR)
> 
> We only inline methods and delegate to Impl in case we do a fast InspectorController fetch. I don't see how this inline helps.
> 
We need to put #if ENABLE(INSPECTOR) guard into the inlined method to avoid overhead when inspector is disabled.

> > WebCore/inspector/InspectorInstrumentation.h:483
> > +    didLoadResourceFromMemoryCacheImpl(page->inspectorController(), loader, resource);
> 
> ditto
> 
ditto.

> > WebCore/inspector/front-end/NetworkManager.js:92
> > +            this.mainResourceIdentifier = identifier;
> 
> Now mainResourceIdentifier might conflict mainResource defined in didCommitLoadForFrame. You should instead mark resource as main here and assign WebInspector.mainResource to it (and nuke didCommitLoadForFrame).
> 
> > WebCore/loader/ResourceLoadNotifier.cpp:69
> > +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willReceiveResourceResponse(loader->frameLoader()->frame(), loader->identifier(), loader->documentLoader(), r);
> 
> We are now going to receive dupes / miss notifications. Please do not modify original inspector controller resource reporting in this change.
> 
Fixed.

> > WebCore/loader/ResourceLoadNotifier.cpp:-139
> > -        page->inspectorController()->didReceiveResponse(identifier, loader, r);
> 
> This clearly is a regression - you should call InspectorInstrumentation here.
> 
> > WebCore/loader/ResourceLoadNotifier.cpp:158
> > +        InspectorInstrumentationCookie cookie = InspectorInstrumentation::willReceiveResourceResponse(loader->frameLoader()->frame(), identifier, loader, response);
> 
> You should not make calls to inspector controller in the new places.
Fixed.

[WebKit Review Bot]

Attachment 78529 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7409118

[WebKit Review Bot]

Attachment 78529 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7419109

[Pavel Feldman]

Comment on attachment 78529 [details]
Patch

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

> Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:51
> +    m_inspectorController->didReceiveResponse(identifier, response);

You should call InspectorInstrumentation::didReceiveResponse (here and in other places).

> Source/WebCore/inspector/InspectorController.h:162
> +    void didReceiveResponse(unsigned long identifier, const ResourceResponse&);

Make this private as well.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:443
> +    ic->didReceiveResponse(identifier, response);

Add // FIXME: move this to console agent.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:464
> +    ic->didFailLoading(identifier, error);

ditto

> Source/WebCore/inspector/front-end/NetworkManager.js:-228
> -                mainResource.isMainResource = true;

You will need to rebaseline this.

> WebKit/chromium/src/WebDevToolsAgentImpl.cpp:295
> +        InspectorInstrumentation::willSendRequest(mainFrame(), resourceId, request.toMutableResourceRequest(), ResourceResponse());

mainFrame will be used by the InspectorResourceAgent and plugin resources will belong to the wrong node in the frame tree.

[Yury Semikhatsky]

Created attachment 78667 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #17)
> (From update of attachment 78529 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78529&action=review
> 
> > Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:51
> > +    m_inspectorController->didReceiveResponse(identifier, response);
> 
> You should call InspectorInstrumentation::didReceiveResponse (here and in other places).
> 
Done.

> > Source/WebCore/inspector/InspectorController.h:162
> > +    void didReceiveResponse(unsigned long identifier, const ResourceResponse&);
> 
> Make this private as well.
> 
Done.

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:443
> > +    ic->didReceiveResponse(identifier, response);
> 
> Add // FIXME: move this to console agent.
> 
Done.

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:464
> > +    ic->didFailLoading(identifier, error);
> 
> ditto
>
Done.
 
> > Source/WebCore/inspector/front-end/NetworkManager.js:-228
> > -                mainResource.isMainResource = true;
> 
> You will need to rebaseline this.
> 
Done.

> > WebKit/chromium/src/WebDevToolsAgentImpl.cpp:295
> > +        InspectorInstrumentation::willSendRequest(mainFrame(), resourceId, request.toMutableResourceRequest(), ResourceResponse());
> 
> mainFrame will be used by the InspectorResourceAgent and plugin resources will belong to the wrong node in the frame tree.

The frame is only used to get corresponding inspectorcontroller instance, all frame<->resource matching is done by means of loader.

[Yury Semikhatsky]

Created attachment 78671 [details]
Patch

[Pavel Feldman]

Comment on attachment 78671 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.cpp:532
> +        bool isNowOnline = networkStateNotifier().onLine();

ditto

> Source/WebCore/inspector/InspectorInstrumentation.cpp:540
> +        ApplicationCacheHost::Status status = frame->loader()->documentLoader()->applicationCacheHost()->status();

It is weird that instrumentation delegate dives into the appcache domain logic.

> Source/WebCore/inspector/InspectorInstrumentation.h:630
> +inline void InspectorInstrumentation::updateApplicationCacheStatus(Frame* frame)

You might want to change WebKit/chromium/src/ApplicationCacheHost.cpp as well.

[Yury Semikhatsky]

Created attachment 78681 [details]
Patch that I'm going to land.

[Yury Semikhatsky]

(In reply to comment #21)
> (From update of attachment 78671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78671&action=review
> 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:532
> > +        bool isNowOnline = networkStateNotifier().onLine();
> 
> ditto
> 
Done.

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:540
> > +        ApplicationCacheHost::Status status = frame->loader()->documentLoader()->applicationCacheHost()->status();
> 
> It is weird that instrumentation delegate dives into the appcache domain logic.
> 
Done.

> > Source/WebCore/inspector/InspectorInstrumentation.h:630
> > +inline void InspectorInstrumentation::updateApplicationCacheStatus(Frame* frame)
> 
> You might want to change WebKit/chromium/src/ApplicationCacheHost.cpp as well.
Done.

[Yury Semikhatsky]

Committed r75604: <http://trac.webkit.org/changeset/75604>