Bug 211207 – REGRESSION(r260791) Network process fails to suspend promptly

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Network process fails to suspend promptly because NetworkProcess::prepareToSuspend() does not call its completion handler.

This issue is with this code and was caused by using the same WorkQueue for all ITP stores:
"""
    forEachNetworkSession([&callbackAggregator](auto& networkSession) {
        if (auto* resourceLoadStatistics = networkSession.resourceLoadStatistics()) {
            if (!resourceLoadStatistics->isEphemeral()) {
                WTFLogAlways("CHRIS: resourceLoadStatistics::BEGIN");
                resourceLoadStatistics->suspend([callbackAggregator] { WTFLogAlways("CHRIS: resourceLoadStatistics::END"); });
            }
        }
    });
"""

On first loop iteration, resourceLoadStatistics->suspend() hangs/suspends the WorkQueue so every follow-up iteration will hang when calling resourceLoadStatistics->suspend().

[Chris Dumez]

Will try a static suspend() for the static WorkQueue, instead of suspending each ITP store individually.

[Radar WebKit Bug Importer]

<rdar://problem/62620454>

[Chris Dumez]

Created attachment 398002 [details]
Patch

[Chris Dumez]

Comment on attachment 398002 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +

Trying to write an API test for this. For now, I validated this manually.

[Alex Christensen]

Comment on attachment 398002 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:-1385
> -    ASSERT(RunLoop::isMain() && !isEphemeral());

Why not keep the RunLoop::isMain() assertion?

[Geoffrey Garen]

Why not just call the completion handler, if that was the bug?

[Geoffrey Garen]

Comment on attachment 398002 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:67
> +WebResourceLoadStatisticsStore::State WebResourceLoadStatisticsStore::suspendedState { WebResourceLoadStatisticsStore::State::Running };
> +Lock WebResourceLoadStatisticsStore::suspendedStateLock;
> +Condition WebResourceLoadStatisticsStore::suspendedStateChangeCondition;

Do these need to be globals instead of data members?

[Alex Christensen]

They're static, so just as global as sharedStatisticsQueue

[Chris Dumez]

Created attachment 398003 [details]
Patch

[Chris Dumez]

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 398002 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398002&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:67
> > +WebResourceLoadStatisticsStore::State WebResourceLoadStatisticsStore::suspendedState { WebResourceLoadStatisticsStore::State::Running };
> > +Lock WebResourceLoadStatisticsStore::suspendedStateLock;
> > +Condition WebResourceLoadStatisticsStore::suspendedStateChangeCondition;
> 
> Do these need to be globals instead of data members?

Could you clarify? They are global in that they are static. Would you like me to move those statics to the cpp?

[Chris Dumez]

Created attachment 398010 [details]
Patch

[Chris Dumez]

I added an API test.

[EWS]

Committed r260928: <https://trac.webkit.org/changeset/260928>

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