Bug 205522 – Resource Load Statistics: Add timing information to WebPageProxy::logFrameNavigation() to detect delayed client-side redirects

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

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().

[Radar WebKit Bug Importer]

<rdar://problem/58125759>

[John Wilander]

Created attachment 386277 [details]
Patch

[John Wilander]

Created attachment 386892 [details]
Patch

[John Wilander]

The Windows test failures look unrelated. http/wpt/css/css-highlight-api/ failures and one more CSS failure.

[Chris Dumez]

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?

[John Wilander]

(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.

[John Wilander]

Created attachment 387118 [details]
Patch

[Chris Dumez]

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.

[John Wilander]

(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!

[WebKit Commit Bot]

Comment on attachment 387118 [details]
Patch

Clearing flags on attachment: 387118

Committed r254296: <https://trac.webkit.org/changeset/254296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.