Bug 183925 - CacheStorage::Engine should not ref itself when hopping to a background thread
Summary: CacheStorage::Engine should not ref itself when hopping to a background thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-22 17:14 PDT by youenn fablet
Modified: 2018-03-23 11:02 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.83 KB, patch)
2018-03-22 17:18 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (9.79 KB, patch)
2018-03-23 07:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (9.80 KB, patch)
2018-03-23 10:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-03-22 17:14:11 PDT
Instead we can use a weakThis
Comment 1 youenn fablet 2018-03-22 17:14:36 PDT
<rdar://problem/38580483>
Comment 2 youenn fablet 2018-03-22 17:18:05 PDT
Created attachment 336340 [details]
Patch
Comment 3 youenn fablet 2018-03-23 07:54:47 PDT
Created attachment 336370 [details]
Patch
Comment 4 Chris Dumez 2018-03-23 08:47:58 PDT
Comment on attachment 336370 [details]
Patch

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

r=me with comments

> Source/WebKit/ChangeLog:10
> +        Use weak pointer when hopping ot background threads.

typo: ot

> Source/WebKit/ChangeLog:14
> +        Made sure to use just oen Engine for all private sessions.

typo: oen

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:330
> +            RunLoop::main().dispatch([this, weakThis = makeWeakPtr(this), identifier]() mutable {

I do not think we should be calling makeWeakPtr() from a background thread. I doubt it is thread safe (In particular, createWeakPtr() that is being implicitly called). I'd suggest WTFMove() ing weakThis here.
Comment 5 youenn fablet 2018-03-23 10:09:27 PDT
Created attachment 336385 [details]
Patch
Comment 6 youenn fablet 2018-03-23 10:09:59 PDT
(In reply to youenn fablet from comment #5)
> Created attachment 336385 [details]
> Patch

Thanks for the review.
Fixed the issues in this patch.
Comment 7 WebKit Commit Bot 2018-03-23 11:02:34 PDT
Comment on attachment 336385 [details]
Patch

Clearing flags on attachment: 336385

Committed r229904: <https://trac.webkit.org/changeset/229904>
Comment 8 WebKit Commit Bot 2018-03-23 11:02:36 PDT
All reviewed patches have been landed.  Closing bug.