Bug 192248 - Web Inspector: REGRESSION(r238330): Timeline auto-capture does not work after page transition
Summary: Web Inspector: REGRESSION(r238330): Timeline auto-capture does not work after...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-30 13:18 PST by Joseph Pecoraro
Modified: 2018-12-04 12:06 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.69 KB, patch)
2018-11-30 17:46 PST, Joseph Pecoraro
drousso: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (3.74 KB, patch)
2018-12-03 16:43 PST, Joseph Pecoraro
drousso: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.71 MB, application/zip)
2018-12-04 05:42 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2018-11-30 13:18:41 PST
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
Comment 1 Radar WebKit Bug Importer 2018-11-30 17:31:26 PST
<rdar://problem/46390199>
Comment 2 Joseph Pecoraro 2018-11-30 17:46:29 PST
Created attachment 356273 [details]
[PATCH] Proposed Fix
Comment 3 Devin Rousso 2018-12-02 20:34:09 PST
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);
    }
Comment 4 Devin Rousso 2018-12-02 20:36:14 PST
(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
Comment 5 Joseph Pecoraro 2018-12-03 11:07:32 PST
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.
Comment 6 Joseph Pecoraro 2018-12-03 16:43:49 PST
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.
Comment 7 Devin Rousso 2018-12-03 21:47:03 PST
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).
Comment 8 EWS Watchlist 2018-12-04 05:42:46 PST
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
Comment 9 EWS Watchlist 2018-12-04 05:42:47 PST
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
Comment 10 Joseph Pecoraro 2018-12-04 11:05:18 PST
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.
Comment 11 Joseph Pecoraro 2018-12-04 11:08:36 PST
https://trac.webkit.org/r238864