Bug 190324 – NetworkCache::Storage should be WTF::DestructionThread::Main

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

[youenn fablet]

This will simplify the code and make it more robust.

[youenn fablet]

Created attachment 351705 [details]
Patch

[EWS Watchlist]

Comment on attachment 351705 [details]
Patch

Attachment 351705 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9467282

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-add.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-add.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-delete.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-put.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-put.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-cache.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-matchAll.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-keys.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/redirected-response.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-storage.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/interfaces-sw.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-matchAll.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-keys.https.html

[EWS Watchlist]

Created attachment 351707 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351705 [details]
Patch

Attachment 351705 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9467586

New failing tests:
http/tests/misc/resource-timing-resolution.html

[EWS Watchlist]

Created attachment 351709 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[youenn fablet]

Created attachment 351714 [details]
Patch

[youenn fablet]

Created attachment 351715 [details]
Patch

[youenn fablet]

Created attachment 351716 [details]
Patch

[EWS Watchlist]

Comment on attachment 351716 [details]
Patch

Attachment 351716 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9468829

New failing tests:
css3/filters/backdrop/add-remove-add-backdrop-filter.html

[EWS Watchlist]

Created attachment 351717 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

Comment on attachment 351716 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:-242
>  Storage::~Storage()
>  {
> -    ASSERT(RunLoop::isMain());

Um... why are you removing the assertion that checks that it is destroyed on the main thread?

[youenn fablet]

(In reply to Michael Catanzaro from comment #11)
> Comment on attachment 351716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351716&action=review
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:-242
> >  Storage::~Storage()
> >  {
> > -    ASSERT(RunLoop::isMain());
> 
> Um... why are you removing the assertion that checks that it is destroyed on
> the main thread?

This is guaranteed by ThreadSafeRefCounted<DestructionThread::Main>.
I can keep it if that helps.
If useful here, it might be best to add such assertion in ThreadSafeRefCounted destructor instead of here.

[youenn fablet]

Created attachment 351724 [details]
Patch

[Michael Catanzaro]

(In reply to youenn fablet from comment #12)
> This is guaranteed by ThreadSafeRefCounted<DestructionThread::Main>.
> I can keep it if that helps.

Up to you, but I think I would keep it. Of course every assertion in the codebase should be guaranteed to never fail already... we have them because we know sometimes things go wrong.

> If useful here, it might be best to add such assertion in
> ThreadSafeRefCounted destructor instead of here.

Sounds good.

[Antti Koivisto]

> If useful here, it might be best to add such assertion in
> ThreadSafeRefCounted destructor instead of here.

I don't think ThreadSafeRefCounted implies that the object needs to be destroyed in the main thread.

[youenn fablet]

(In reply to Antti Koivisto from comment #15)
> > If useful here, it might be best to add such assertion in
> > ThreadSafeRefCounted destructor instead of here.
> 
> I don't think ThreadSafeRefCounted implies that the object needs to be
> destroyed in the main thread.

ThreadSafeRefCounted does not.
ThreadSafeRefCounted<WTF::DestructionThread::Main> does.
I kept the assert for now though.

[WebKit Commit Bot]

Comment on attachment 351724 [details]
Patch

Clearing flags on attachment: 351724

Committed r236936: <https://trac.webkit.org/changeset/236936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45102139>