Bug 203623 - Resource Load Statistics: Flush the shared ResourceLoadObserver when the webpage is closed by JavaScript
Summary: Resource Load Statistics: Flush the shared ResourceLoadObserver when the webp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-30 13:33 PDT by John Wilander
Modified: 2020-01-18 18:13 PST (History)
7 users (show)

See Also:


Attachments
Patch (30.40 KB, patch)
2019-10-30 14:36 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (30.76 KB, patch)
2019-10-30 15:10 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (30.71 KB, patch)
2019-10-30 16:18 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (30.35 KB, patch)
2019-10-31 14:42 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (30.77 KB, patch)
2019-11-04 11:03 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-10-30 13:33:21 PDT
We should flush the shared ResourceLoadObserver when the webpage is closed by JavaScript.
Comment 1 Radar WebKit Bug Importer 2019-10-30 13:33:42 PDT
<rdar://problem/56756427>
Comment 2 John Wilander 2019-10-30 14:36:02 PDT
Created attachment 382357 [details]
Patch
Comment 3 John Wilander 2019-10-30 15:10:10 PDT
Created attachment 382363 [details]
Patch
Comment 4 Chris Dumez 2019-10-30 15:45:34 PDT
Comment on attachment 382363 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:39
> +    deleteShared();

I don't think we need this deleteShared() method at all. We could simply inline:
delete sharedObserver();

delete does the null check for you and you don't need to set to null since the next line will do it for you.

> Source/WebCore/loader/ResourceLoadObserver.cpp:50
> +ResourceLoadObserver*& ResourceLoadObserver::sharedIfExists()

We should not be adding an ampersand here, this is a public method.

> Source/WebCore/loader/ResourceLoadObserver.cpp:55
> +void ResourceLoadObserver::deleteShared()

We can delete this method altogether as I mentioned above.

> Source/WebCore/loader/ResourceLoadObserver.cpp:57
> +    if (auto*& observer = sharedIfExists()) {

This should be using sharedObserver(), not sharedIfExists().

> Source/WebCore/loader/ResourceLoadObserver.h:42
> +    WEBCORE_EXPORT static ResourceLoadObserver*& sharedIfExists();

No ampersand.

> Source/WebCore/loader/ResourceLoadObserver.h:64
> +    static void deleteShared();

Not needed.

> Source/WebCore/page/DOMWindow.cpp:1042
> +    ResourceLoadObserver::shared().updateCentralStatisticsStore();

Maybe we do this way below, only if close() does not return early?

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:286
> +        m_statisticsStore->updateCookieBlocking([this]() {

What guarantees that |this| stays alive here. Looks to me like you need to protect/ref it.

> Source/WebKit/UIProcess/WebProcessPool.cpp:2351
> +    for (auto* processPool : WebProcessPool::allProcessPools()) {

This is a method of a given WebProcessPool (e.g. not static). So why would it iterate over all process pools?

> Source/WebKit/UIProcess/WebProcessPool.cpp:2353
> +            if (process->canSendMessage())

This check is not needed

> Source/WebKit/WebProcess/WebProcess.cpp:674
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)

Could we move the #if ENABLE(RESOURCE_LOAD_STATISTICS) inside the implementation of flushResourceLoadStatistics to make the call sites look better?

> Source/WebKit/WebProcess/WebProcess.cpp:1568
> +        WebCore::ResourceLoadObserver::setShared(new WebResourceLoadObserver);

This could be on one line:
WebCore::ResourceLoadObserver::setShared(enabled ? new WebResourceLoadObserver : nullptr);

Also, are we sure that setResourceLoadStatisticsEnabled(true) cannot be called twice in a raw? In such cases we would destroy the observer unnecessarily and possibly lose data.

On the same note, should the WebResourceLoadObserver destructor flush? So that if you have pending data then disable ITP in this process, then the pending statistics are not lost and still sent to the network process?
Comment 5 John Wilander 2019-10-30 16:13:42 PDT
Thanks for having a look!

(In reply to Chris Dumez from comment #4)
> Comment on attachment 382363 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382363&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:39
> > +    deleteShared();
> 
> I don't think we need this deleteShared() method at all. We could simply
> inline:
> delete sharedObserver();

Good idea. Will fix.

> delete does the null check for you and you don't need to set to null since
> the next line will do it for you.
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:50
> > +ResourceLoadObserver*& ResourceLoadObserver::sharedIfExists()
> 
> We should not be adding an ampersand here, this is a public method.

It was needed because of the use of the ampersand in deleteShared() but now that deleteShared() is gone we don't need it anymore.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:55
> > +void ResourceLoadObserver::deleteShared()
> 
> We can delete this method altogether as I mentioned above.

Yup.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:57
> > +    if (auto*& observer = sharedIfExists()) {
> 
> This should be using sharedObserver(), not sharedIfExists().

In the case of no existing observer, wouldn't that create a new observer only to delete it the next line? Anyway, it's mute now that we're deleting deleteShared().

> > Source/WebCore/loader/ResourceLoadObserver.h:42
> > +    WEBCORE_EXPORT static ResourceLoadObserver*& sharedIfExists();
> 
> No ampersand.
> 
> > Source/WebCore/loader/ResourceLoadObserver.h:64
> > +    static void deleteShared();
> 
> Not needed.
> 
> > Source/WebCore/page/DOMWindow.cpp:1042
> > +    ResourceLoadObserver::shared().updateCentralStatisticsStore();
> 
> Maybe we do this way below, only if close() does not return early?

I was thinking of sending that IPC as early as possible but I can move it down to when we are sure to close.

> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:286
> > +        m_statisticsStore->updateCookieBlocking([this]() {
> 
> What guarantees that |this| stays alive here. Looks to me like you need to
> protect/ref it.

Will fix.

> > Source/WebKit/UIProcess/WebProcessPool.cpp:2351
> > +    for (auto* processPool : WebProcessPool::allProcessPools()) {
> 
> This is a method of a given WebProcessPool (e.g. not static). So why would
> it iterate over all process pools?

Will fix.

> > Source/WebKit/UIProcess/WebProcessPool.cpp:2353
> > +            if (process->canSendMessage())
> 
> This check is not needed

Huh. Will fix.

> > Source/WebKit/WebProcess/WebProcess.cpp:674
> > +#if ENABLE(RESOURCE_LOAD_STATISTICS)
> 
> Could we move the #if ENABLE(RESOURCE_LOAD_STATISTICS) inside the
> implementation of flushResourceLoadStatistics to make the call sites look
> better?

It actually already is so I'll just clean up the call sites.

> > Source/WebKit/WebProcess/WebProcess.cpp:1568
> > +        WebCore::ResourceLoadObserver::setShared(new WebResourceLoadObserver);
> 
> This could be on one line:
> WebCore::ResourceLoadObserver::setShared(enabled ? new
> WebResourceLoadObserver : nullptr);

Nice. Will fix.

> Also, are we sure that setResourceLoadStatisticsEnabled(true) cannot be
> called twice in a raw? In such cases we would destroy the observer
> unnecessarily and possibly lose data.

The added check:

if (WebCore::DeprecatedGlobalSettings::resourceLoadStatisticsEnabled() == enabled || m_sessionID->isEphemeral())
        return;

… should take care of that.

> On the same note, should the WebResourceLoadObserver destructor flush? So
> that if you have pending data then disable ITP in this process, then the
> pending statistics are not lost and still sent to the network process?

Good idea. I'll add it.
Comment 6 John Wilander 2019-10-30 16:18:49 PDT
Created attachment 382385 [details]
Patch
Comment 7 Alex Christensen 2019-10-31 11:05:12 PDT
Comment on attachment 382385 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:39
> +    delete sharedObserver();

Could we use a std::unique_ptr<ResourceLoadObserver> instead of manual new/delete?

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:286
> +        m_statisticsStore->updateCookieBlocking([this, protectedThis = makeRef(*this)]() {

Grumble grumble.  https://twitter.com/alexfchr/status/1083136307197440000
Here, to work around a VS bug we need to do protectedThis = protectedThis.copyRef()
Comment 8 John Wilander 2019-10-31 14:42:29 PDT
Created attachment 382504 [details]
Patch
Comment 9 John Wilander 2019-10-31 14:43:47 PDT
Comment on attachment 382504 [details]
Patch

Thanks, Alex! Especially for the solution to the Windows build failure. See if you like this better.
Comment 10 John Wilander 2019-11-04 11:03:28 PST
Created attachment 382751 [details]
Patch
Comment 11 John Wilander 2019-11-04 13:26:09 PST
Comment on attachment 382751 [details]
Patch

Thanks, Alex and Chris!
Comment 12 WebKit Commit Bot 2019-11-04 14:13:48 PST
Comment on attachment 382751 [details]
Patch

Clearing flags on attachment: 382751

Committed r252014: <https://trac.webkit.org/changeset/252014>
Comment 13 WebKit Commit Bot 2019-11-04 14:13:50 PST
All reviewed patches have been landed.  Closing bug.