Bug 151112 - Web Inspector: Timeline Recording across page navigations behaves poorly
Summary: Web Inspector: Timeline Recording across page navigations behaves poorly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-10 11:16 PST by Joseph Pecoraro
Modified: 2015-11-18 13:46 PST (History)
8 users (show)

See Also:


Attachments
[PATCH] Work in Progress (1.40 KB, patch)
2015-11-10 11:20 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (1.49 KB, patch)
2015-11-10 11:37 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-11-10 11:16:08 PST
* SUMMARY
Timeline Recording across page navigations behaves poorly.

* STEPS TO REPRODUCE
1. Inspect about:blank
2. Start timeline recording
3. Navigate to bogojoker.com
  => Timeline data briefly shows (incomplete network requests) but ends up with no data at all in the timeline
Comment 1 Radar WebKit Bug Importer 2015-11-10 11:16:52 PST
<rdar://problem/23484607>
Comment 2 Joseph Pecoraro 2015-11-10 11:20:23 PST
Created attachment 265203 [details]
[PATCH] Work in Progress

Attached is a work in progress that highlights the issue. InspectorPageAgent is reseting the stopwatch when the main frame loads. If we are recording a timeline this will mean suddenly timestamps have lower values then expected.

I'm not proposing this as a patch in and of itself because I think we may want to specifically handle PageAgent.reload. If a user is in the frontend and reloads during an auto-record, I think it should treat that as another new auto-record and not a sub-load within an active recording.

Also, I wonder what happens if we just never reset the stopwatch.
Comment 3 Joseph Pecoraro 2015-11-10 11:25:21 PST
> I'm not proposing this as a patch in and of itself because I think we may
> want to specifically handle PageAgent.reload. If a user is in the frontend
> and reloads during an auto-record, I think it should treat that as another
> new auto-record and not a sub-load within an active recording.

Actually, the frontend handles this correctly. It creates a new recording and it appears to make the initial timestamp the start timestamp.


> Also, I wonder what happens if we just never reset the stopwatch.

In a quick experiment this behaves correctly. I just have to audit the values to see how they are used.
Comment 4 Joseph Pecoraro 2015-11-10 11:37:45 PST
Created attachment 265207 [details]
[PATCH] Proposed Fix

Yeah, I didn't see any issues. The Network Tab uses a Network Timeline that the frontend resets on main page navigation, so that tab is conveniently all network requests made for the current page load and timestamps relative to the main resource. The Timeline itself shows all network events across multiple page loads.

There is some cleanup that can be done on the frontend for how engine initiated actions (layout, style recalc, etc) are displayed in the Timeline OverviewView (SourceCodeTimelines) but this is a big step forward in being able to debug multi-page timeline recordings.
Comment 5 Timothy Hatcher 2015-11-10 12:16:30 PST
Comment on attachment 265207 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorPageAgent.cpp:-810
> -    if (frame.isMainFrame()) {
> -        auto stopwatch = m_environment.executionStopwatch();
> -        stopwatch->reset();
> -        stopwatch->start();
> -    }

Do we need to start the stopwatch if it isn't already started? I made this change to make timestamps work in the Network tab, when Timeline recoding isn't always happening. I suspect those timestamps are always zero now.
Comment 6 BJ Burg 2015-11-10 12:48:34 PST
Comment on attachment 265207 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/inspector/InspectorPageAgent.cpp:-810
>> -    }
> 
> Do we need to start the stopwatch if it isn't already started? I made this change to make timestamps work in the Network tab, when Timeline recoding isn't always happening. I suspect those timestamps are always zero now.

I have a potential design fix for multi-navigation recordings, will discuss in person.
Comment 7 Joseph Pecoraro 2015-11-10 13:51:30 PST
Comment on attachment 265207 [details]
[PATCH] Proposed Fix

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

>>> Source/WebCore/inspector/InspectorPageAgent.cpp:-810
>>> -    }
>> 
>> Do we need to start the stopwatch if it isn't already started? I made this change to make timestamps work in the Network tab, when Timeline recoding isn't always happening. I suspect those timestamps are always zero now.
> 
> I have a potential design fix for multi-navigation recordings, will discuss in person.

InspectorPageAgent::enable resets and starts the stopwatch. As long as the domain is enabled everything will behave as expected.
Comment 8 Joseph Pecoraro 2015-11-18 11:47:09 PST
> I have a potential design fix for multi-navigation recordings, will discuss
> in person.

Is the patch above still suitable?
Comment 9 WebKit Commit Bot 2015-11-18 13:46:38 PST
Comment on attachment 265207 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 265207

Committed r192585: <http://trac.webkit.org/changeset/192585>
Comment 10 WebKit Commit Bot 2015-11-18 13:46:41 PST
All reviewed patches have been landed.  Closing bug.