Web Inspector: REGRESSION(r238330): Timeline auto-capture does not work after page transition

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Timeline auto-capture does not work after page transition.

Steps to Reproduce:
1. inspect <https://webkit.org>
2. show Timelines tab
3. refresh the page (timeline auto-capture will start)
4. wait for timeline to stop capturing (or manually stop it)
5. navigate to <https://apple.com>
 => auto-capture didn't start
 => refreshing the page will not start auto-capture

[Radar WebKit Bug Importer]

<rdar://problem/46390199>

[Joseph Pecoraro]

Created attachment 356273 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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

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

r-, as this causes a different issue.  Using the same reproduction steps, when navigating to <https://apple.com>, auto-capture starts, but no records are shown other than the initial heap snapshot and the main frame network entry.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:68
> +        if (target.TimelineAgent) {

Style: early-return if this is false.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:80
> +            this._legacyAttemptStartAutoCapturingForFrame(WI.networkManager.mainFrame);

As far as I understand, this is only needed if `target.TimelineAgent.setAutoCaptureEnabled` isn't supported.  Should we be using `_attemptAutoCapturingForFrame` instead?

    transitionPageTarget()
    {
        this._attemptAutoCapturingForFrame(WI.networkManager.mainFrame);
    }

[Devin Rousso]

(In reply to Devin Rousso from comment #3)
> r-, as this causes a different issue.  Using the same reproduction steps,
> when navigating to <https://apple.com>, auto-capture starts, but no records
> are shown other than the initial heap snapshot and the main frame network
> entry.
I also see lots errors (ordered by frequency):
 - Models/CallFrame.js:246:31: CONSOLE ERROR We should have detected source code for something with a url
 - Views/TimelineDataGridNode.js:236:23: CONSOLE ERROR
 - Views/TimelineRecordBar.js:251:23: CONSOLE ERROR

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:68
>> +        if (target.TimelineAgent) {
> 
> Style: early-return if this is false.

All other initializeTargets avoid early returns, so I'm following the pattern there (since some might check for multiple agents).

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:80
>> +            this._legacyAttemptStartAutoCapturingForFrame(WI.networkManager.mainFrame);
> 
> As far as I understand, this is only needed if `target.TimelineAgent.setAutoCaptureEnabled` isn't supported.  Should we be using `_attemptAutoCapturingForFrame` instead?
> 
>     transitionPageTarget()
>     {
>         this._attemptAutoCapturingForFrame(WI.networkManager.mainFrame);
>     }

_attemptAutoCapturingForFrame assumes the backend started (or will start) a capture. That will not be the case here.

[Joseph Pecoraro]

Created attachment 356434 [details]
[PATCH] Proposed Fix

Issue was when we were starting the legacy recording we hadn't yet transitioned the frame, so the old main resource (with old start time) was included with all the new resources after navigation. This change starts the legacy recording after we've transitioned main resources so all the resources included in the recording are from the new page.

[Devin Rousso]

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

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:69
> +        if (target.TimelineAgent) {

Should we assert `!this._transitioningPageTarget` as a sanity check, since we'd expect to call `transitionPageTarget` shortly thereafter?  Is it possible for targets to switch quickly enough that `_mainResourceDidChange` wouldn't get called to reset `_transitioningPageTarget`?

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:849
> +        if (this._transitioningPageTarget) {

Just curious, is there a case where this wouldn't be true during some sort of navigation?  My guess is that this isn't true for refreshes, but will be true for all other forms of navigation (e.g. where the URL changes).

[EWS Watchlist]

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

Attachment 356434 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10261941

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-closed-window.https.html

[EWS Watchlist]

Created attachment 356488 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:69
>> +        if (target.TimelineAgent) {
> 
> Should we assert `!this._transitioningPageTarget` as a sanity check, since we'd expect to call `transitionPageTarget` shortly thereafter?  Is it possible for targets to switch quickly enough that `_mainResourceDidChange` wouldn't get called to reset `_transitioningPageTarget`?

Initialize target is the first thing that happens for a particular target, and we set-up the state of the target. In the case of a page transition, its going to happen before transitionPageTarget has happened. In the case of any other target (not possible yet) we don't know what the state of the manager will be.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:849
>> +        if (this._transitioningPageTarget) {
> 
> Just curious, is there a case where this wouldn't be true during some sort of navigation?  My guess is that this isn't true for refreshes, but will be true for all other forms of navigation (e.g. where the URL changes).

Reloads. Any non-process-swap navigation.

[Joseph Pecoraro]

https://trac.webkit.org/r238864