Bug 37394 - inspector/timeline-network-resource.html fails when run twice
: inspector/timeline-network-resource.html fails when run twice
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Yury Semikhatsky
:
Depends on:
Blocks: 49412 50822
  Show dependency treegraph
 
Reported: 2010-04-10 16:13 PDT by Eric Seidel
Modified: 2011-01-12 05:07 PST (History)
7 users (show)

See Also:


Attachments
Patch (7.42 KB, patch)
2010-12-28 03:57 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (71.33 KB, patch)
2010-12-29 02:20 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (70.95 KB, patch)
2010-12-29 02:23 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (70.43 KB, patch)
2011-01-11 07:41 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (78.12 KB, patch)
2011-01-12 02:40 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (78.17 KB, patch)
2011-01-12 02:59 PST, Yury Semikhatsky
pfeldman: review+
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2010-04-10 16:13:09 PDT
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 Eric Seidel 2010-04-10 16:15:43 PDT
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 Yury Semikhatsky 2010-12-24 08:32:22 PST
O c
Comment 3 Yury Semikhatsky 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 Yury Semikhatsky 2010-12-28 03:57:58 PST
Created attachment 77547 [details]
Patch
Comment 5 Ilya Tikhonovsky 2010-12-28 04:33:39 PST
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
Comment 6 Yury Semikhatsky 2010-12-29 02:20:18 PST
Created attachment 77608 [details]
Patch
Comment 7 Yury Semikhatsky 2010-12-29 02:21:17 PST
(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.
Comment 8 Yury Semikhatsky 2010-12-29 02:23:34 PST
Created attachment 77609 [details]
Patch
Comment 9 Yury Semikhatsky 2010-12-29 02:24:41 PST
(In reply to comment #8)
> Created an attachment (id=77609) [details]
> Patch

ChangeLog cleanup.
Comment 10 WebKit Review Bot 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 WebKit Review Bot 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 Pavel Feldman 2010-12-29 08:33:15 PST
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.
Comment 13 Yury Semikhatsky 2011-01-11 07:41:43 PST
Created attachment 78529 [details]
Patch
Comment 14 Yury Semikhatsky 2011-01-11 07:48:06 PST
(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.
Comment 15 WebKit Review Bot 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 WebKit Review Bot 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 Pavel Feldman 2011-01-11 08:13:56 PST
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.
Comment 18 Yury Semikhatsky 2011-01-12 02:40:43 PST
Created attachment 78667 [details]
Patch
Comment 19 Yury Semikhatsky 2011-01-12 02:43:00 PST
(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.
Comment 20 Yury Semikhatsky 2011-01-12 02:59:30 PST
Created attachment 78671 [details]
Patch
Comment 21 Pavel Feldman 2011-01-12 04:39:08 PST
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.
Comment 22 Yury Semikhatsky 2011-01-12 05:02:52 PST
Created attachment 78681 [details]
Patch that I'm going to land.
Comment 23 Yury Semikhatsky 2011-01-12 05:03:17 PST
(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.
Comment 24 Yury Semikhatsky 2011-01-12 05:07:16 PST
Committed r75604: <http://trac.webkit.org/changeset/75604>