| Summary: | REGRESSION(r260791) Network process fails to suspend promptly | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, darin, ggaren, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=212608 | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 211080 | ||||||||||
| Attachments: |
|
||||||||||
Will try a static suspend() for the static WorkQueue, instead of suspending each ITP store individually. Created attachment 398002 [details]
Patch
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. 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? Why not just call the completion handler, if that was the bug? 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? They're static, so just as global as sharedStatisticsQueue Created attachment 398003 [details]
Patch
(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? Created attachment 398010 [details]
Patch
I added an API test. Committed r260928: <https://trac.webkit.org/changeset/260928> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398010 [details]. |
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().