Summary: | Resource Load Statistics data summary does not report data which is held up in the web content process. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, ews-watchlist, japhet, webkit-bug-importer, wilander | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Kate Cheney
2020-08-25 12:53:24 PDT
Created attachment 407220 [details]
Patch
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'? 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. Created attachment 407326 [details]
Patch
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? 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. 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. (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. (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([] { }); }); (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. Created attachment 407343 [details]
Patch
Committed r266214: <https://trac.webkit.org/changeset/266214> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407343 [details]. |