Bug 215822 – Resource Load Statistics data summary does not report data which is held up in the web content process.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

ITP has a 5 second delay in sending website data to the network process, so a request for getResourceLoadStatisticsDataSummary() might miss new data. We should figure out a way to send any lingering data when the getResourceLoadStatisticsDataSummary() SPI is called.

[Kate Cheney]

<rdar://problem/66682044>

[Kate Cheney]

Created attachment 407220 [details]
Patch

[Brent Fulgham]

Comment on attachment 407220 [details]
Patch

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

Looks good.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1447
> +    postTask([this, completionHandler = WTFMove(completionHandler)]() mutable {

A very important fix! :-)

> Source/WebKit/WebProcess/WebProcess.cpp:1914
> +    completionHandler();

Does this completionHandler need to be synchronized with the 'updateCentralStatisticsStore'?

[Kate Cheney]

Comment on attachment 407220 [details]
Patch

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

>> Source/WebKit/WebProcess/WebProcess.cpp:1914
>> +    completionHandler();
> 
> Does this completionHandler need to be synchronized with the 'updateCentralStatisticsStore'?

Yes it does, I will post another patch.

[Kate Cheney]

Created attachment 407326 [details]
Patch

[Brent Fulgham]

Comment on attachment 407326 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1563
> +        processPool->sendResourceLoadStatisticsDataImmediately([this, processPool, callbackAggregator] {

@Chris: Should we be makeRef'ing this so that we ensure the object is valid when the completion handler fires? Or passing a weak ptr so we can bail out if the process is shut down before all process answer the message send?

[Chris Dumez]

Comment on attachment 407326 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.h:63
> +    virtual void updateCentralStatisticsStoreWithReply(CompletionHandler<void()>&& completionHandler) { completionHandler(); }

Why aren't you updating the existing one? Seems overkill to have several functions for this..

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:365
> +            postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable {

I think the following would work:
postTaskReply(WTFMove(completionHandler));

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:373
> +        postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable {

ditto.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1563
>> +        processPool->sendResourceLoadStatisticsDataImmediately([this, processPool, callbackAggregator] {
> 
> @Chris: Should we be makeRef'ing this so that we ensure the object is valid when the completion handler fires? Or passing a weak ptr so we can bail out if the process is shut down before all process answer the message send?

Since this is asynchronous, it is indeed not safe to capture |this| like this. You definitely need to either protect this or use a weak ptr as Brent suggested.

[Chris Dumez]

Comment on attachment 407326 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.h:63
> +    virtual void updateCentralStatisticsStoreWithReply(CompletionHandler<void()>&& completionHandler) { completionHandler(); }

Why aren't you updating the existing one? Seems overkill to have several functions for this..

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:365
> +            postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable {

I think the following would work:
postTaskReply(WTFMove(completionHandler));

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:373
> +        postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable {

ditto.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1563
>> +        processPool->sendResourceLoadStatisticsDataImmediately([this, processPool, callbackAggregator] {
> 
> @Chris: Should we be makeRef'ing this so that we ensure the object is valid when the completion handler fires? Or passing a weak ptr so we can bail out if the process is shut down before all process answer the message send?

Since this is asynchronous, it is indeed not safe to capture |this| like this. You definitely need to either protect this or use a weak ptr as Brent suggested.

[Kate Cheney]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 407326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407326&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.h:63
> > +    virtual void updateCentralStatisticsStoreWithReply(CompletionHandler<void()>&& completionHandler) { completionHandler(); }
> 
> Why aren't you updating the existing one? Seems overkill to have several
> functions for this..

Can a timer take functions with arguments? That is why I made another, this one is used in WebResourceLoadObserver.cpp:

m_notificationTimer(*this, &WebResourceLoadObserver::updateCentralStatisticsStore)


> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:365
> > +            postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable {
> 
> I think the following would work:
> postTaskReply(WTFMove(completionHandler));
> 

Will fix.

> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:373
> > +        postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable {
> 
> ditto.

Will fix.

> 
> >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1563
> >> +        processPool->sendResourceLoadStatisticsDataImmediately([this, processPool, callbackAggregator] {
> > 
> > @Chris: Should we be makeRef'ing this so that we ensure the object is valid when the completion handler fires? Or passing a weak ptr so we can bail out if the process is shut down before all process answer the message send?
> 
> Since this is asynchronous, it is indeed not safe to capture |this| like
> this. You definitely need to either protect this or use a weak ptr as Brent
> suggested.

Ok, will change.

[Chris Dumez]

(In reply to katherine_cheney from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 407326 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=407326&action=review
> > 
> > > Source/WebCore/loader/ResourceLoadObserver.h:63
> > > +    virtual void updateCentralStatisticsStoreWithReply(CompletionHandler<void()>&& completionHandler) { completionHandler(); }
> > 
> > Why aren't you updating the existing one? Seems overkill to have several
> > functions for this..
> 
> Can a timer take functions with arguments? That is why I made another, this
> one is used in WebResourceLoadObserver.cpp:
> 
> m_notificationTimer(*this,
> &WebResourceLoadObserver::updateCentralStatisticsStore)

If you look at the Timer constructor, you can see it can take in a lambda so I would imagine this would work:
m_notificationTimer([this] { updateCentralStatisticsStore([] { }); });

[Kate Cheney]

(In reply to Chris Dumez from comment #10)
> (In reply to katherine_cheney from comment #9)
> > (In reply to Chris Dumez from comment #8)
> > > Comment on attachment 407326 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=407326&action=review
> > > 
> > > > Source/WebCore/loader/ResourceLoadObserver.h:63
> > > > +    virtual void updateCentralStatisticsStoreWithReply(CompletionHandler<void()>&& completionHandler) { completionHandler(); }
> > > 
> > > Why aren't you updating the existing one? Seems overkill to have several
> > > functions for this..
> > 
> > Can a timer take functions with arguments? That is why I made another, this
> > one is used in WebResourceLoadObserver.cpp:
> > 
> > m_notificationTimer(*this,
> > &WebResourceLoadObserver::updateCentralStatisticsStore)
> 
> If you look at the Timer constructor, you can see it can take in a lambda so
> I would imagine this would work:
> m_notificationTimer([this] { updateCentralStatisticsStore([] { }); });

!! genius !! I will change it to be the same function.

[Kate Cheney]

Created attachment 407343 [details]
Patch

[EWS]

Committed r266214: <https://trac.webkit.org/changeset/266214>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407343 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67843691>