Bug 186644 – Crash under WebCore::SWServer::registrationStoreImportComplete()

• Component: Service Workers
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

<rdar://problem/40982257>

[Chris Dumez]

Created attachment 342788 [details]
Patch

[youenn fablet]

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::

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 342814 [details]
Patch

[Brady Eidson]

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!)

[WebKit Commit Bot]

Comment on attachment 342814 [details]
Patch

Clearing flags on attachment: 342814

Committed r232874: <https://trac.webkit.org/changeset/232874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

I am working on a basic API test that uses a non default (but persistent) data store to cover RegistrationDatabase destruction.

[youenn fablet]

> > 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?

[Per Arne Vollan]

(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.