ITP has a 5 second delay in sending website data to the network process, so a request for getResourceLoadStatisticsDataSummary() might miss new data. We should figure out a way to send any lingering data when the getResourceLoadStatisticsDataSummary() SPI is called.
<rdar://problem/66682044>
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.
(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].
<rdar://problem/67843691>