Bug 205522 - Resource Load Statistics: Add timing information to WebPageProxy::logFrameNavigation() to detect delayed client-side redirects
Summary: Resource Load Statistics: Add timing information to WebPageProxy::logFrameNav...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-20 14:32 PST by John Wilander
Modified: 2020-01-10 15:09 PST (History)
4 users (show)

See Also:


Attachments
Patch (31.25 KB, patch)
2019-12-20 16:51 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (40.08 KB, patch)
2020-01-06 14:48 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.36 KB, patch)
2020-01-08 11:40 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-12-20 14:32:54 PST
We should detect delayed client-side redirects. This involves setting a timestamp in WebPageProxy::didFinishDocumentLoadForFrame() and using that timestamp to calculate the delay and pass it on in WebPageProxy::logFrameNavigation().
Comment 1 Radar WebKit Bug Importer 2019-12-20 14:33:17 PST
<rdar://problem/58125759>
Comment 2 John Wilander 2019-12-20 16:51:50 PST
Created attachment 386277 [details]
Patch
Comment 3 John Wilander 2020-01-06 14:48:50 PST
Created attachment 386892 [details]
Patch
Comment 4 John Wilander 2020-01-06 16:44:00 PST
The Windows test failures look unrelated. http/wpt/css/css-highlight-api/ failures and one more CSS failure.
Comment 5 Chris Dumez 2020-01-07 11:59:51 PST
Comment on attachment 386892 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.h:2656
> +    MonotonicTime m_didFinishDocumentLoadForMainFrameTimestamp;

I wish we did not have to add a WebPageProxy data member for this. Could this maybe be stored somewhere else, in ITP-specific code?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:238
> +        terminationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) { }];

What's the point of this termination observer that seems to do nothing?

> LayoutTests/ChangeLog:9
> +        Results updated with additional data now that delayed redirects are captured.

Shouldn't we add a test specifically for what you're fixing though?
Comment 6 John Wilander 2020-01-07 12:38:08 PST
(In reply to Chris Dumez from comment #5)
> Comment on attachment 386892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386892&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:2656
> > +    MonotonicTime m_didFinishDocumentLoadForMainFrameTimestamp;
> 
> I wish we did not have to add a WebPageProxy data member for this. Could
> this maybe be stored somewhere else, in ITP-specific code?

The timestamp is tied to the webpage and this is in the UI process where we don't really have any ITP code except for piping and API surface. Any idea of where it could go?

> > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:238
> > +        terminationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) { }];
> 
> What's the point of this termination observer that seems to do nothing?

Looks like it can be removed. Maybe it was only ever there for the purposes of ITP.

> > LayoutTests/ChangeLog:9
> > +        Results updated with additional data now that delayed redirects are captured.
> 
> Shouldn't we add a test specifically for what you're fixing though?

Sure, I can do that.
Comment 7 John Wilander 2020-01-08 11:40:56 PST
Created attachment 387118 [details]
Patch
Comment 8 Chris Dumez 2020-01-09 12:26:14 PST
Comment on attachment 387118 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:225
> +        Seconds minDelayAfterMainFrameDocumentLoadToNotBeARedirect { 5_s };

This seems really long to me.
Comment 9 John Wilander 2020-01-09 12:32:43 PST
(In reply to Chris Dumez from comment #8)
> Comment on attachment 387118 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387118&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:225
> > +        Seconds minDelayAfterMainFrameDocumentLoadToNotBeARedirect { 5_s };
> 
> This seems really long to me.

I would agree if it wasn't for real measurement (on a fast machine). Here are three samples I still had in my logs:

1.920775 s
2.165201 s
2.231257 s

Thanks for the review!
Comment 10 WebKit Commit Bot 2020-01-09 14:01:56 PST
Comment on attachment 387118 [details]
Patch

Clearing flags on attachment: 387118

Committed r254296: <https://trac.webkit.org/changeset/254296>
Comment 11 WebKit Commit Bot 2020-01-09 14:01:58 PST
All reviewed patches have been landed.  Closing bug.