Bug 187143 - Make sure the WebResourceLoadStatisticsStore gets destroyed on the main thread
Summary: Make sure the WebResourceLoadStatisticsStore gets destroyed on the main thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 187055
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-28 09:37 PDT by Chris Dumez
Modified: 2018-06-28 17:09 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.03 KB, patch)
2018-06-28 15:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.47 KB, patch)
2018-06-28 15:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-06-28 09:37:55 PDT
Make sure the WebResourceLoadStatisticsStore gets destroyed on the main thread. Currently, we keep capturing a Ref<> to the WebResourceLoadStatisticsStore whenever we dispatch a task to the work queue. This means that the WebResourceLoadStatisticsStore will get destroyed on a background thread if the work queue is the last one to hold a ref to the store, which would not be safe.
Comment 1 Chris Dumez 2018-06-28 15:08:39 PDT
Created attachment 343857 [details]
Patch
Comment 2 youenn fablet 2018-06-28 15:50:17 PDT
Comment on attachment 343857 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:188
> +    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), origins = WTFMove(origins)]() mutable {

origins should be isolated except if we are sure they are coming straight from IPC.
We should add a comment saying why this is ok.
And we should probably make WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated private to ensure it does not get used out of IPC.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:258
>  void WebResourceLoadStatisticsStore::requestStorageAccessUnderOpener(String&& primaryDomainInNeedOfStorageAccess, uint64_t openerPageID, String&& openerPrimaryDomain, bool isTriggeredByUserGesture)

Ditto.
Comment 3 Chris Dumez 2018-06-28 15:57:46 PDT
Created attachment 343868 [details]
Patch
Comment 4 WebKit Commit Bot 2018-06-28 17:06:06 PDT
Comment on attachment 343868 [details]
Patch

Clearing flags on attachment: 343868

Committed r233342: <https://trac.webkit.org/changeset/233342>
Comment 5 WebKit Commit Bot 2018-06-28 17:06:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2018-06-28 17:09:12 PDT
<rdar://problem/41609309>