Bug 157504 – Web Inspector: Backend should initiate timeline recordings on page navigations to ensure nothing is missed

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Backend should initiate timeline recordings on page navigations to ensure nothing is missed.

* TEST
<script>
function foo() { bar(); }
function bar() {
    var until = Date.now() + 5;
    while (Date.now() < until);
    console.log("after");
}
foo();
</script>

* STEPS TO REPRODUCE
1. Inspect test page
2. Show Timeline Tab
3. Select the Timelines you want (include at least Scripts)
4. Reload
  => Expected Timeline Recording data for foo/bar but there was none

* NOTES
- Frontend can inform the backend to enable/disable auto capturing
- Frontend can inform the backend which instruments to enable/disable for auto capturing
- This can be reused by console.profile/console.profileEnd as a programmatic way to start/stop the instruments from the backend

[Radar WebKit Bug Importer]

<rdar://problem/26188642>

[Joseph Pecoraro]

Created attachment 278478 [details]
[PATCH] Proposed Fix

Writing tests that reload or navigate the inspected page is currently extremely flakey, so I didn't attempt one yet.

[Blaze Burg]

Comment on attachment 278478 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/protocol/Timeline.json:104
> +            "description": "Toggle auto capture state. If <code>true</code> the backend will disable breakpoints and start capturing on navigation. The backend will fire the <code>autoCaptureStarted</code> event when an auto capture starts. The frontend should stop the auto capture when appropriate and re-enable breakpoints.",

Why is disabling breakpoints the right thing to do? I don't think hitting a breakpoint on auto-capture would be a bad thing and we don't normally disable breakpoints for timeline recordings. This could, for example, cause probes or other breakpoint actions to be skipped when they are expected to be executed.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:135
> +    m_autoCaptureInstruments = &const_cast<InspectorArray&>(instruments);

Input validation should probably go here so if it fails we can send a failed command response to the frontend.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:431
> +    for (auto instrument : *m_autoCaptureInstruments.get()) {

See above, this parsing/validation should not be deferred til later.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:-625
> -        if (!event.target.isMainFrame() || (this._isCapturing && !this._autoCapturingMainResource))

It's not obvious in isolation what the event passed in is for. Is this called when the frame navigates? Maybe add a comment or assert so it's easier to reason about.

EDIT: I think the event listener should be this._provisionalLoadStarted, and then call into startAutoCapturing if we definitely want it to start. That will make this much easier to follow (mostly by moving early exits into the event listener).

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:664
> +        if (mainResource === this._autoCapturingMainResource)

this member would make a lot more sense as this._mainResourceForAutoCapturing. As is, it's confusing whether it's a flag or storing an object.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:674
> +        this._stopAutoRecordingAfterMaxTimeout();

Can you rename this to be consistent with the new code?

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:971
> +            case WebInspector.TimelineRecord.Type.Script:

Do we do this mapping anywhere else? It might be worth extracting to a class method if so.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:988
> +        TimelineAgent.setAutoCaptureInstruments([...instrumentSet]);

You could do Array.from(instrumentSet). Maybe setAutoCaptureInstruments should be doing that in the binding code anyway...

[Timothy Hatcher]

Comment on attachment 278478 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/inspector/protocol/Timeline.json:104
>> +            "description": "Toggle auto capture state. If <code>true</code> the backend will disable breakpoints and start capturing on navigation. The backend will fire the <code>autoCaptureStarted</code> event when an auto capture starts. The frontend should stop the auto capture when appropriate and re-enable breakpoints.",
> 
> Why is disabling breakpoints the right thing to do? I don't think hitting a breakpoint on auto-capture would be a bad thing and we don't normally disable breakpoints for timeline recordings. This could, for example, cause probes or other breakpoint actions to be skipped when they are expected to be executed.

We do disable breakpoints now when Timeline recording to give you high performance profiling. Given that we only auto-capture when the Timelines tab is shown, it is clear you are recording not trying to debug. See the UI we show in the Debugger tab's sidebar when you are recording in Timelines. Maybe we need similar UI for probes?

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:664
>> +        if (mainResource === this._autoCapturingMainResource)
> 
> this member would make a lot more sense as this._mainResourceForAutoCapturing. As is, it's confusing whether it's a flag or storing an object.

Agreed.

[Joseph Pecoraro]

Comment on attachment 278478 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:431
>> +    for (auto instrument : *m_autoCaptureInstruments.get()) {
> 
> See above, this parsing/validation should not be deferred til later.

I was content with "enable the instruments we know about" here. So that future instruments wouldn't be problematic. But, I can do validation.

[Joseph Pecoraro]

Created attachment 278530 [details]
[PATCH] Proposed Fix - Addressed Comments

[Blaze Burg]

(In reply to comment #4)
> Comment on attachment 278478 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278478&action=review
> 
> >> Source/JavaScriptCore/inspector/protocol/Timeline.json:104
> >> +            "description": "Toggle auto capture state. If <code>true</code> the backend will disable breakpoints and start capturing on navigation. The backend will fire the <code>autoCaptureStarted</code> event when an auto capture starts. The frontend should stop the auto capture when appropriate and re-enable breakpoints.",
> > 
> > Why is disabling breakpoints the right thing to do? I don't think hitting a breakpoint on auto-capture would be a bad thing and we don't normally disable breakpoints for timeline recordings. This could, for example, cause probes or other breakpoint actions to be skipped when they are expected to be executed.
> 
> We do disable breakpoints now when Timeline recording to give you high
> performance profiling. Given that we only auto-capture when the Timelines
> tab is shown, it is clear you are recording not trying to debug. See the UI
> we show in the Debugger tab's sidebar when you are recording in Timelines.
> Maybe we need similar UI for probes?

OK, I didn't remember that UI change.

[Blaze Burg]

Comment on attachment 278530 [details]
[PATCH] Proposed Fix - Addressed Comments

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

r=me

> Source/WebCore/ChangeLog:17
> +        Inform the TimelineAgent when we the main frame starts a new load.

Nit: "...whenever the main frame..."

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:656
> +            return this._legacyAttemptStartAutoCapturingForFrame(event);

This should be `frame`, not `event`.

[Build Bot]

Comment on attachment 278530 [details]
[PATCH] Proposed Fix - Addressed Comments

Attachment 278530 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1300378

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 278536 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Created attachment 278547 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 278547 [details]
[PATCH] For Landing

Clearing flags on attachment: 278547

Committed r200651: <http://trac.webkit.org/changeset/200651>