Bug 186644 - Crash under WebCore::SWServer::registrationStoreImportComplete()
Summary: Crash under WebCore::SWServer::registrationStoreImportComplete()
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: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-14 21:00 PDT by Chris Dumez
Modified: 2018-06-15 12:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.84 KB, patch)
2018-06-14 21:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.73 MB, application/zip)
2018-06-14 23:09 PDT, EWS Watchlist
no flags Details
Patch (15.69 KB, patch)
2018-06-15 08:35 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-14 21:00:55 PDT
Crash under WebCore::SWServer::registrationStoreImportComplete():
Thread 0 Crashed:
0   WebCore                       	0x00000001b95a4430 WebCore::SWServer::registrationStoreImportComplete() + 40 (SWServer.cpp:112)
1   JavaScriptCore                	0x00000001b6a66bf8 WTF::CrossThreadTaskHandler::handleTaskRepliesOnMainThread() + 324 (Function.h:56)
2   JavaScriptCore                	0x00000001b6a66bf8 WTF::CrossThreadTaskHandler::handleTaskRepliesOnMainThread() + 324 (Function.h:56)
3   JavaScriptCore                	0x00000001b69277b8 WTF::dispatchFunctionsFromMainThread() + 308 (Function.h:56)
4   Foundation                    	0x00000001af788304 __NSThreadPerformPerform + 336 (NSThread.m:1259)
5   CoreFoundation                	0x00000001aecbeb98 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24 (CFRunLoop.c:1980)
6   CoreFoundation                	0x00000001aecbeb18 __CFRunLoopDoSource0 + 88 (CFRunLoop.c:2015)
7   CoreFoundation                	0x00000001aecbe3f0 __CFRunLoopDoSources0 + 176 (CFRunLoop.c:2051)
8   CoreFoundation                	0x00000001aecbc090 __CFRunLoopRun + 1052 (CFRunLoop.c:2922)
9   CoreFoundation                	0x00000001aebf5570 CFRunLoopRunSpecific + 436 (CFRunLoop.c:3247)
10  Foundation                    	0x00000001af676f34 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 300 (NSRunLoop.m:367)
11  Foundation                    	0x00000001af6e8670 -[NSRunLoop(NSRunLoop) run] + 88 (NSRunLoop.m:389)
12  libxpc.dylib                  	0x00000001ae97f5ec _xpc_objc_main + 532 (main.m:170)
13  libxpc.dylib                  	0x00000001ae981744 xpc_main + 184 (init.c:1471)
14  com.apple.WebKit.Storage      	0x000000010262f59c main + 380 (XPCServiceMain.mm:160)
15  libdyld.dylib                 	0x00000001ae76cf30 start + 4
Comment 1 Chris Dumez 2018-06-14 21:01:07 PDT
<rdar://problem/40982257>
Comment 2 Chris Dumez 2018-06-14 21:09:10 PDT
Created attachment 342788 [details]
Patch
Comment 3 youenn fablet 2018-06-14 21:59:42 PDT
Comment on attachment 342788 [details]
Patch

I wonder whether we should not make CrossThreadTaskHandler a ThreadSafeRefCounted<CrossThreadTaskHandler, DestructionThread::MainThread>
That way, we might be able to continue using CrossThreadTaskHandler.
That might also help IDBServer which we might also want to remove like we do for SWServer and CacheStorageEngine when the session gets destroyed.

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

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:124
> +        callOnMainThread([protectedThis = WTFMove(protectedThis)] { });

Is there a risk for the lambda to be destroyed in the background thread without being executed?
It might be safer to use DestructionThread::MainThread instead.

> Source/WebCore/workers/service/server/RegistrationDatabase.h:61
> +    RegistrationDatabase(RegistrationStore&, const String& databaseDirectory);

Should probably be a String&& here and above.

> Source/WebCore/workers/service/server/RegistrationDatabase.h:63
> +    void postTaskToWorkQueue(WTF::Function<void()>&&);

No need for WTF::
Comment 4 EWS Watchlist 2018-06-14 23:09:45 PDT
Comment on attachment 342788 [details]
Patch

Attachment 342788 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8191493

New failing tests:
http/tests/preload/onload_event.html
Comment 5 EWS Watchlist 2018-06-14 23:09:56 PDT
Created attachment 342794 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 6 Chris Dumez 2018-06-15 08:27:13 PDT
Comment on attachment 342788 [details]
Patch

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

>> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:124
>> +        callOnMainThread([protectedThis = WTFMove(protectedThis)] { });
> 
> Is there a risk for the lambda to be destroyed in the background thread without being executed?
> It might be safer to use DestructionThread::MainThread instead.

I think my code is safe, this is a callOnMainThread, not a ScriptExecutionContext::postTask(). That said DestructionThread::Main will likely make the code look nicer. I did not remember you had landed this. I'll use it.

>> Source/WebCore/workers/service/server/RegistrationDatabase.h:61
>> +    RegistrationDatabase(RegistrationStore&, const String& databaseDirectory);
> 
> Should probably be a String&& here and above.

Not new code but I guess.

>> Source/WebCore/workers/service/server/RegistrationDatabase.h:63
>> +    void postTaskToWorkQueue(WTF::Function<void()>&&);
> 
> No need for WTF::

Really depends, Windows often does not like Function without WTF:: prefix iirc.
Comment 7 Chris Dumez 2018-06-15 08:30:21 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 342788 [details]
> Patch
> 
> I wonder whether we should not make CrossThreadTaskHandler a
> ThreadSafeRefCounted<CrossThreadTaskHandler, DestructionThread::MainThread>
> That way, we might be able to continue using CrossThreadTaskHandler.
> That might also help IDBServer which we might also want to remove like we do
> for SWServer and CacheStorageEngine when the session gets destroyed.

I'd personally prefer to move away from CrossThreadTaskHandler and eventually remove it. CrossThreadTaskHandler has more complexity (2 job queues with locking, 1 WTF::Thread) than a simple WorkQueue, and is barely used (only IDB and here). Also, fixing CrossThreadTaskHandler is not as easy as making it ThreadSafeRefCounted. For example, the background thread keeps running until the job queue is killed. However, nothing ever calls kill() on the job queue, and nothing keeps the job queue alive.
Comment 8 Chris Dumez 2018-06-15 08:35:18 PDT
Created attachment 342814 [details]
Patch
Comment 9 Brady Eidson 2018-06-15 09:05:51 PDT
Comment on attachment 342788 [details]
Patch

CrossThreadTaskHandler predated a lot of niceties we now have available. I think Chris's patch is fine, and we could refactor-out the last use of CrossThreadTaskHandler later using the same technique (obviously outside the scope of this patch!)
Comment 10 WebKit Commit Bot 2018-06-15 09:16:42 PDT
Comment on attachment 342814 [details]
Patch

Clearing flags on attachment: 342814

Committed r232874: <https://trac.webkit.org/changeset/232874>
Comment 11 WebKit Commit Bot 2018-06-15 09:16:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Chris Dumez 2018-06-15 09:17:59 PDT
I am working on a basic API test that uses a non default (but persistent) data store to cover RegistrationDatabase destruction.
Comment 13 youenn fablet 2018-06-15 11:45:59 PDT
> > No need for WTF::
> 
> Really depends, Windows often does not like Function without WTF:: prefix
> iirc.

I thought it was fixed, Per Arne, can you confirm?
Comment 14 Per Arne Vollan 2018-06-15 12:00:50 PDT
(In reply to youenn fablet from comment #13)
> > > No need for WTF::
> > 
> > Really depends, Windows often does not like Function without WTF:: prefix
> > iirc.
> 
> I thought it was fixed, Per Arne, can you confirm?

I am not sure, but this patch seems to have no problems building on Windows.