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
<rdar://problem/40982257>
Created attachment 342788 [details] Patch
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 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
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 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.
(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.
Created attachment 342814 [details] Patch
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 on attachment 342814 [details] Patch Clearing flags on attachment: 342814 Committed r232874: <https://trac.webkit.org/changeset/232874>
All reviewed patches have been landed. Closing bug.
I am working on a basic API test that uses a non default (but persistent) data store to cover RegistrationDatabase destruction.
> > 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?
(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.