Bug 196623

Summary: Close service worker database on network process suspension
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, ddkilzer, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2019-04-04 13:36:51 PDT 
Close service worker database on network process suspension
Comment 1 youenn fablet 2019-04-04 13:37:15 PDT 
<rdar://problem/48930869>
Comment 2 youenn fablet 2019-04-04 13:42:30 PDT 
Created attachment 366750 [details]
Patch
Comment 3 youenn fablet 2019-04-04 14:39:53 PDT 
Created attachment 366760 [details]
Patch
Comment 4 youenn fablet 2019-04-05 10:54:40 PDT 
Created attachment 366827 [details]
Patch
Comment 5 youenn fablet 2019-04-05 14:53:51 PDT 
Created attachment 366846 [details]
Patch
Comment 6 youenn fablet 2019-04-09 10:35:06 PDT 
Ping review?
Comment 7 Alex Christensen 2019-04-09 11:30:08 PDT 
Comment on attachment 366846 [details]
Patch

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

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:288
> +    postTaskToWorkQueue([this, completionHandler = WTFMove(completionHandler)]() mutable {

It might be useful to have an ASSERT(isMainThread()) before this.
Also, what guarantees "this" is still alive when it is used?  Please use a RefPtr or a WeakPtr to avoid UAF.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1793
> +    EXPECT_TRUE([[NSFileManager defaultManager] fileExistsAtPath:swDBPath.path]);

Should we remove this file to clean up so it doesn't affect future tests?
Comment 8 youenn fablet 2019-04-09 13:08:54 PDT 
Thanks for the review.

(In reply to Alex Christensen from comment #7)
> Comment on attachment 366846 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366846&action=review
> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:288
> > +    postTaskToWorkQueue([this, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> It might be useful to have an ASSERT(isMainThread()) before this.

Let's add one in postTaskToWorkQueue.

> Also, what guarantees "this" is still alive when it is used?  Please use a
> RefPtr or a WeakPtr to avoid UAF.

postTaskToWorkQueue is refing "this" for us.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1793
> > +    EXPECT_TRUE([[NSFileManager defaultManager] fileExistsAtPath:swDBPath.path]);
> 
> Should we remove this file to clean up so it doesn't affect future tests?

I followed the existing file test pattern which is to clean the data store before each test.
Comment 9 youenn fablet 2019-04-09 15:59:32 PDT 
Created attachment 367075 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-04-09 16:37:23 PDT 
Comment on attachment 367075 [details]
Patch for landing

Clearing flags on attachment: 367075

Committed r244097: <https://trac.webkit.org/changeset/244097>
Comment 11 WebKit Commit Bot 2019-04-09 16:37:25 PDT 
All reviewed patches have been landed.  Closing bug.