Bug 37394 - inspector/timeline-network-resource.html fails when run twice
: inspector/timeline-network-resource.html fails when run twice
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 49412 50822
  Show dependency treegraph
 
Reported: 2010-04-10 16:13 PST by
Modified: 2011-01-12 05:07 PST (History)


Attachments
Patch (7.42 KB, patch)
2010-12-28 03:57 PST, Yury Semikhatsky
no flags Review Patch | Details | Formatted Diff | Diff
Patch (71.33 KB, patch)
2010-12-29 02:20 PST, Yury Semikhatsky
no flags Review Patch | Details | Formatted Diff | Diff
Patch (70.95 KB, patch)
2010-12-29 02:23 PST, Yury Semikhatsky
no flags Review Patch | Details | Formatted Diff | Diff
Patch (70.43 KB, patch)
2011-01-11 07:41 PST, Yury Semikhatsky
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.12 KB, patch)
2011-01-12 02:40 PST, Yury Semikhatsky
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.17 KB, patch)
2011-01-12 02:59 PST, Yury Semikhatsky
pfeldman: review+
Review Patch | Details | Formatted Diff | Diff
Patch that I'm going to land. (77.98 KB, patch)
2011-01-12 05:02 PST, Yury Semikhatsky
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-10 16:13:09 PST
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
------- Comment #1 From 2010-04-10 16:15:43 PST -------
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.
------- Comment #2 From 2010-12-24 08:32:22 PST -------
O c
------- Comment #3 From 2010-12-24 08:34:13 PST -------
I was running this test on Linux Gtk Debug and it would fail every second run with the same diff as Eric posted.
------- Comment #4 From 2010-12-28 03:57:58 PST -------
Created an attachment (id=77547) [details]
Patch
------- Comment #5 From 2010-12-28 04:33:39 PST -------
(From update of attachment 77547 [details])
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
------- Comment #6 From 2010-12-29 02:20:18 PST -------
Created an attachment (id=77608) [details]
Patch
------- Comment #7 From 2010-12-29 02:21:17 PST -------
(In reply to comment #6)
> Created an attachment (id=77608) [details] [details]
> Patch

The same patch with all comments addressed and the small refactoring that was discussed offline.
------- Comment #8 From 2010-12-29 02:23:34 PST -------
Created an attachment (id=77609) [details]
Patch
------- Comment #9 From 2010-12-29 02:24:41 PST -------
(In reply to comment #8)
> Created an attachment (id=77609) [details] [details]
> Patch

ChangeLog cleanup.
------- Comment #10 From 2010-12-29 03:19:52 PST -------
Attachment 77608 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7205246
------- Comment #11 From 2010-12-29 03:30:49 PST -------
Attachment 77609 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7231237
------- Comment #12 From 2010-12-29 08:33:15 PST -------
(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).

> 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.
------- Comment #13 From 2011-01-11 07:41:43 PST -------
Created an attachment (id=78529) [details]
Patch
------- Comment #14 From 2011-01-11 07:48:06 PST -------
(In reply to comment #12)
> (From update of attachment 77609 [details] [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.
------- Comment #15 From 2011-01-11 07:51:04 PST -------
Attachment 78529 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7409118
------- Comment #16 From 2011-01-11 08:10:03 PST -------
Attachment 78529 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7419109
------- Comment #17 From 2011-01-11 08:13:56 PST -------
(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).

> 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.
------- Comment #18 From 2011-01-12 02:40:43 PST -------
Created an attachment (id=78667) [details]
Patch
------- Comment #19 From 2011-01-12 02:43:00 PST -------
(In reply to comment #17)
> (From update of attachment 78529 [details] [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.
------- Comment #20 From 2011-01-12 02:59:30 PST -------
Created an attachment (id=78671) [details]
Patch
------- Comment #21 From 2011-01-12 04:39:08 PST -------
(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

> 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.
------- Comment #22 From 2011-01-12 05:02:52 PST -------
Created an attachment (id=78681) [details]
Patch that I'm going to land.
------- Comment #23 From 2011-01-12 05:03:17 PST -------
(In reply to comment #21)
> (From update of attachment 78671 [details] [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.
------- Comment #24 From 2011-01-12 05:07:16 PST -------
Committed r75604: <http://trac.webkit.org/changeset/75604>