Bug 186644

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