Bug 37394

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.

O c

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

Created attachment 77547 [details] Patch

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

Created attachment 77608 [details] Patch

(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.

Created attachment 77609 [details] Patch

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

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

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

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.

Created attachment 78529 [details] Patch

(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.

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

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

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.

Created attachment 78667 [details] Patch

(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.

Created attachment 78671 [details] Patch

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.

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

(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.

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