Bug 86406 – Web Inspector: Annotate timeline records with a frame identifier

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

[Michael Schneider]

The timeline events of all frames are interleaved with each other, with no way to know on which frame they originated from.

As the timeline records are a very rich data source and enables to automatically analyze what the browser does, adding a frame id to would greatly simplify tools that analyze the timeline data.

[Bryan]

Created attachment 145920 [details]
Patch to add frameId to timeline records

This adds the ability to track the frame that each timeline event is associated with. Not all events have an associated frame. Only those events with an associated frame will have a frameId.

This patch is based on the suggestions and recommendations provided by caseq and pfeldman. This is a preliminary version of the change, to get some initial feedback. No tests have been written yet.

[WebKit Review Bot]

Attachment 145920 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bryan]

Created attachment 145925 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 145925 [details]
Patch

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

A bunch of style nits, but general approach looks good.

> b/Source/WebCore/inspector/InspectorController.cpp:354
> +    m_profilerAgent->disable(&error);

indent

> b/Source/WebCore/inspector/InspectorTimelineAgent.cpp:333
> +      String contextId(m_pageAgent->frameId(frame));
> +      recordRaw->setString("frameId", contextId);

indent

> b/Source/WebCore/inspector/InspectorTimelineAgent.cpp:420
> +      String contextId(m_pageAgent->frameId(frame));
> +      record->setString("frameId", contextId);

indent

> b/Source/WebCore/inspector/front-end/TimelinePresentationModel.js:110
> +    if (!mainFrame || mainFrame.id() != record.frameId) 

!= => !==

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:442
> +      timelineAgent->willLoadXHR(request->url(), frameForScriptExecutionContext(context));

indent

> b/Source/WebCore/inspector/WorkerInspectorController.cpp:105
> +    m_timelineAgent = InspectorTimelineAgent::create(m_instrumentingAgents.get(), NULL, m_state.get(), InspectorTimelineAgent::WorkerInspector, InspectorTimelineAgent::FrameInstrumentationNotSupported);

NULL => 0

> b/Source/WebCore/inspector/InspectorInstrumentation.h:612
> +      return willCallFunctionImpl(instrumentingAgents, scriptName, scriptLine, context);

indent

> b/Source/WebCore/inspector/InspectorInstrumentation.h:651
> +        return willDispatchEventImpl(instrumentingAgents, event, window, node, ancestors, document->frame());

frame looks redundant here, we can use window->frame(). actually, it also looks redundant in the instrumentation interface.

> b/Source/WebCore/inspector/InspectorInstrumentation.h:689
> +        return willDispatchEventOnWindowImpl(instrumentingAgents, event, window, frame);

ditto

> b/Source/WebCore/inspector/InspectorTimelineAgent.h:154
> +        Frame* frame;

Storing a raw pointer to a frame in something that is not a FrameDestructionObserver looks a bit scary. Can we resolve frames to ids early?

[Andrey Kosyakov]

Comment on attachment 145925 [details]
Patch

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

>> b/Source/WebCore/inspector/front-end/TimelinePresentationModel.js:110
>> +    if (!mainFrame || mainFrame.id() != record.frameId) 
> 
> != => !==

Also, ResourceTreeFrame.id is a getter, not a function. If it doesn't break tests, we're in trouble :)

[Bryan]

Created attachment 146070 [details]
Patch with fixes for comments and updated layout tests.

[Bryan]

Comment on attachment 146070 [details]
Patch with fixes for comments and updated layout tests.

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

A couple comments about whether certain objects can be null, and in turn whether I should protect against a null pointer dereference in those locations. Thanks!

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:595
> +        timelineAgent->willSendResourceRequest(identifier, request, loader->frame());

Is it possible that the loader may be null here? If so I'll add code to prevent a null pointer dereference.

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:702
> +        timelineAgent->didFinishLoadingResource(identifier, false, finishTime, loader->frame());

Same question.

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:710
> +        timelineAgent->didFinishLoadingResource(identifier, true, 0, loader->frame());

Same

> b/Source/WebCore/inspector/InspectorInstrumentation.h:590
> +        didScheduleResourceRequestImpl(instrumentingAgents, url, document->frame());

Is it possible for document to be null here?

> b/Source/WebCore/inspector/InspectorInstrumentation.h:853
> +        return willRecalculateStyleImpl(instrumentingAgents, document->frame());

Null issue here?

> b/Source/WebCore/inspector/InspectorInstrumentation.h:1177
> +        return willWriteHTMLImpl(instrumentingAgents, length, startLine, document->frame());

Same here? (And other locations as well)

[WebKit Review Bot]

Attachment 146070 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Comment on attachment 146070 [details]
Patch with fixes for comments and updated layout tests.

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

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:321
> +        timelineAgent->willDispatchEvent(event, window ? window->frame() : 0);

On a closer look at the call site of InspectorInstrumentation::willDispatchEvent(), I think that window, as passed to us, is not the best way to get frame. Check out WindowEventContext. I'd rather get the frame from the document.

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:349
> +        timelineAgent->willDispatchEvent(event, window ? window->frame() : 0);

This call, on the other hand, should never receive a null window.

>> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:595
>> +        timelineAgent->willSendResourceRequest(identifier, request, loader->frame());
> 
> Is it possible that the loader may be null here? If so I'll add code to prevent a null pointer dereference.

I don't think so -- if loader were null, we would crash in the resource agent anyway.

>> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:702
>> +        timelineAgent->didFinishLoadingResource(identifier, false, finishTime, loader->frame());
> 
> Same question.

From what I can see, we have documentLoader while any resource notifications are dispatched.

>> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:710
>> +        timelineAgent->didFinishLoadingResource(identifier, true, 0, loader->frame());
> 
> Same

As above.

>> b/Source/WebCore/inspector/InspectorInstrumentation.h:590
>> +        didScheduleResourceRequestImpl(instrumentingAgents, url, document->frame());
> 
> Is it possible for document to be null here?

instrumentingAgentsForDocument would crash if document were null.

>> b/Source/WebCore/inspector/InspectorInstrumentation.h:853
>> +        return willRecalculateStyleImpl(instrumentingAgents, document->frame());
> 
> Null issue here?

ditto

>> b/Source/WebCore/inspector/InspectorInstrumentation.h:1177
>> +        return willWriteHTMLImpl(instrumentingAgents, length, startLine, document->frame());
> 
> Same here? (And other locations as well)

as above.

[Andrey Kosyakov]

Comment on attachment 146070 [details]
Patch with fixes for comments and updated layout tests.

Also, please provide ChangeLog entries. Tools/Scripts/prepare-ChangeLog will do that for you (also normally invoked as part of Tools/Scripts/webkit-patch upload)

[Bryan]

Created attachment 146357 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 146357 [details]
Patch

LGTM. Vsevolod, could you please take a look?

[Vsevolod Vlasov]

Comment on attachment 146357 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Andrey Kosyakov

Please fix this before landing.

[Andrey Kosyakov]

Committed r119826: <http://trac.webkit.org/changeset/119826>