Bug 196623 – Close service worker database on network process suspension

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Close service worker database on network process suspension

[youenn fablet]

<rdar://problem/48930869>

[youenn fablet]

Created attachment 366750 [details]
Patch

[youenn fablet]

Created attachment 366760 [details]
Patch

[youenn fablet]

Created attachment 366827 [details]
Patch

[youenn fablet]

Created attachment 366846 [details]
Patch

[youenn fablet]

Ping review?

[Alex Christensen]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 367075 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 367075 [details]
Patch for landing

Clearing flags on attachment: 367075

Committed r244097: <https://trac.webkit.org/changeset/244097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.