Bug 203623 – Resource Load Statistics: Flush the shared ResourceLoadObserver when the webpage is closed by JavaScript

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should flush the shared ResourceLoadObserver when the webpage is closed by JavaScript.

[Radar WebKit Bug Importer]

<rdar://problem/56756427>

[John Wilander]

Created attachment 382357 [details]
Patch

[John Wilander]

Created attachment 382363 [details]
Patch

[Chris Dumez]

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?

[John Wilander]

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.

[John Wilander]

Created attachment 382385 [details]
Patch

[Alex Christensen]

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

[John Wilander]

Created attachment 382504 [details]
Patch

[John Wilander]

Comment on attachment 382504 [details]
Patch

Thanks, Alex! Especially for the solution to the Windows build failure. See if you like this better.

[John Wilander]

Created attachment 382751 [details]
Patch

[John Wilander]

Comment on attachment 382751 [details]
Patch

Thanks, Alex and Chris!

[WebKit Commit Bot]

Comment on attachment 382751 [details]
Patch

Clearing flags on attachment: 382751

Committed r252014: <https://trac.webkit.org/changeset/252014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.