Bug 180170 – Add Internals.terminateServiceWorker, and the ability to restart service workers for postMessage

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Add Internals.terminateServiceWorker, and the ability to restart service workers for postMessage

[Brady Eidson]

Created attachment 328016 [details]
Patch

[Brady Eidson]

Created attachment 328017 [details]
Patch

[Brady Eidson]

Created attachment 328020 [details]
Patch

[Brady Eidson]

EWS on this might depend on the patch in 180216 landing first - but this is still reviewable

[Chris Dumez]

Comment on attachment 328020 [details]
Patch

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

> Source/WebCore/dom/ActiveDOMObject.cpp:61
> +    ASSERT(m_scriptExecutionContext->isContextThread());

Why did you drop the null check? It is pretty common for the script execution context to be destroyed before the activeDOMObject AFAIK. The comment above the if check also says so.

[youenn fablet]

Comment on attachment 328020 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:258
> +#if ENABLE(SERVICE_WORKER)

#if probably not needed

> Source/WebCore/testing/Internals.cpp:4283
> +    ServiceWorkerProvider::singleton().serviceWorkerConnectionForSession(contextDocument()->sessionID()).syncTerminateWorker(worker.identifier());

I guess there is a reason to use syncTerminateWorker, but does it bring value here?
Cannot we do an asynchronous terminate worker, and if script needs to wait for the termination, use a promise and a completion handler to resolve the promise?
Also, since I guess there is some sync IPC below, why do we need it to be synchronous?

> Source/WebCore/workers/service/context/SWContextManager.cpp:97
> +void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Function<void()>&& completionHandler)

How about CompletionHandler<void()>

> Source/WebCore/workers/service/context/SWContextManager.cpp:115
> +        if (completionHandler)

Do we need that check?

> Source/WebCore/workers/service/server/SWServer.cpp:394
> +    m_workersByID.set(data.serviceWorkerIdentifier, result.iterator->value.ptr());

m_workersByID.add is probably sufficient.
Or you could add and verify that the result is a new entry.

[Chris Dumez]

Comment on attachment 328020 [details]
Patch

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

> Source/WebCore/workers/service/context/SWContextManager.cpp:-72
> -    if (!serviceWorker)

Why is this safe? I am worried things are racy enough that this can happen when a page unregisters a service worker while another page is posting a message to said worker.

> Source/WebCore/workers/service/context/SWContextManager.cpp:97
> +void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Function<void()>&& completionHandler)

Could be a WTF::CompletionHandler.

> Source/WebCore/workers/service/context/SWContextManager.cpp:111
> +        RunLoop::main().dispatch([worker = WTFMove(worker)] { });

callOnMainThread() since in WebCore?

[Brady Eidson]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 328020 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328020&action=review
> 
> > Source/WebCore/dom/ActiveDOMObject.cpp:61
> > +    ASSERT(m_scriptExecutionContext->isContextThread());
> 
> Why did you drop the null check? It is pretty common for the script
> execution context to be destroyed before the activeDOMObject AFAIK. The
> comment above the if check also says so.

The first line in the destructor is a null check - this is redundant

[Chris Dumez]

Comment on attachment 328020 [details]
Patch

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

>>> Source/WebCore/dom/ActiveDOMObject.cpp:61
>>> +    ASSERT(m_scriptExecutionContext->isContextThread());
>> 
>> Why did you drop the null check? It is pretty common for the script execution context to be destroyed before the activeDOMObject AFAIK. The comment above the if check also says so.
> 
> The first line in the destructor is a null check - this is redundant

Well, then at least move the comment about why we need to null check to the null check at the beginning of the method :P

[Brady Eidson]

(In reply to youenn fablet from comment #6)
> Comment on attachment 328020 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328020&action=review
> 
> > Source/WebCore/testing/Internals.cpp:258
> > +#if ENABLE(SERVICE_WORKER)
> 
> #if probably not needed
> 
> > Source/WebCore/testing/Internals.cpp:4283
> > +    ServiceWorkerProvider::singleton().serviceWorkerConnectionForSession(contextDocument()->sessionID()).syncTerminateWorker(worker.identifier());
> 
> I guess there is a reason to use syncTerminateWorker, but does it bring
> value here?
> Cannot we do an asynchronous terminate worker, and if script needs to wait
> for the termination, use a promise and a completion handler to resolve the
> promise?
> Also, since I guess there is some sync IPC below, why do we need it to be
> synchronous?

For testing, sync terminate is much more reliable. Also, it's what the Blink one does, so this allows us to import theirs. Doing a promise one (which is actually what I originally started to do!) would make our tests incompatible.

> 
> > Source/WebCore/workers/service/context/SWContextManager.cpp:97
> > +void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Function<void()>&& completionHandler)
> 
> How about CompletionHandler<void()>

Because I've never heard of it/programmed with it before. Let me go so what value it might add.

> 
> > Source/WebCore/workers/service/context/SWContextManager.cpp:115
> > +        if (completionHandler)
> 
> Do we need that check?

We don't call null functions.

> 
> > Source/WebCore/workers/service/server/SWServer.cpp:394
> > +    m_workersByID.set(data.serviceWorkerIdentifier, result.iterator->value.ptr());
> 
> m_workersByID.add is probably sufficient.
> Or you could add and verify that the result is a new entry.

Set is more efficient than add, though?

[Brady Eidson]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 328020 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328020&action=review
> 
> > Source/WebCore/workers/service/context/SWContextManager.cpp:-72
> > -    if (!serviceWorker)
> 
> Why is this safe? I am worried things are racy enough that this can happen
> when a page unregisters a service worker while another page is posting a
> message to said worker.

The only one guiding a SW context process is the StorageProcess.

In the scenario where a message is posted to a worker that either doesn't exist or is in the process of terminating, the StorageProcess sends two messages to the context process in a row - First message creates the worker, second message posts to it.

It would always send those two messages in a row with no intervening message. e.g. The first message would create the worker, and there would be no opportunity for it to go away by the time the second message arrives.

Hence the assert.

> 
> > Source/WebCore/workers/service/context/SWContextManager.cpp:97
> > +void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Function<void()>&& completionHandler)
> 
> Could be a WTF::CompletionHandler.

Again, I've never heard of this. Will look to see what it is.

> 
> > Source/WebCore/workers/service/context/SWContextManager.cpp:111
> > +        RunLoop::main().dispatch([worker = WTFMove(worker)] { });
> 
> callOnMainThread() since in WebCore?

sigh yes.

[Brady Eidson]

(In reply to Brady Eidson from comment #10)
> (In reply to youenn fablet from comment #6)
> 
> > 
> > > Source/WebCore/workers/service/context/SWContextManager.cpp:115
> > > +        if (completionHandler)
> > 
> > Do we need that check?
> 
> We don't call null functions.

This is apparently no longer true.

[Brady Eidson]

Created attachment 328037 [details]
Patch

[Brady Eidson]

Created attachment 328038 [details]
Patch

[youenn fablet]

> For testing, sync terminate is much more reliable. Also, it's what the Blink
> one does, so this allows us to import theirs. Doing a promise one (which is
> actually what I originally started to do!) would make our tests incompatible.

I see, I'll comment on the GitHub issue.
It seems more webby to have it promise based, even if it is just a testing thing.


> > m_workersByID.add is probably sufficient.
> > Or you could add and verify that the result is a new entry.
> 
> Set is more efficient than add, though?

I recently checked that add is more efficient.

[Chris Dumez]

Comment on attachment 328038 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:258
> +#if ENABLE(SERVICE_WORKER)

Not needed.

> Source/WebCore/workers/service/server/SWServer.h:182
> +    HashMap<ServiceWorkerIdentifier, SWServerWorker*> m_workersByID;

I do not see anybody removing workers from this HashMap anymore.

Could this still use a Ref<> as value?

> Source/WebCore/workers/service/server/SWServer.h:184
> +    HashMap<ServiceWorkerIdentifier, Ref<SWServerWorker>> m_terminatingWorkersByID;

Do we really need these 2 hash maps? Why can't we look up the worker in m_workersByID and check its state?

[Chris Dumez]

> > > m_workersByID.add is probably sufficient.
> > > Or you could add and verify that the result is a new entry.
> > 
> > Set is more efficient than add, though?
> 
> I recently checked that add is more efficient.

Agreed:
template<typename KeyArg, typename MappedArg, typename HashArg, typename KeyTraitsArg, typename MappedTraitsArg>
template<typename K, typename V>
auto HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg>::inlineSet(K&& key, V&& value) -> AddResult
{
    AddResult result = inlineAdd(std::forward<K>(key), std::forward<V>(value));
    if (!result.isNewEntry) {
        // The inlineAdd call above found an existing hash table entry; we need to set the mapped value.
        result.iterator->value = std::forward<V>(value);
    }
    return result;
}

a set() is an add() + a branch and an assignment.

[Brady Eidson]

(In reply to youenn fablet from comment #15)
> > For testing, sync terminate is much more reliable. Also, it's what the Blink
> > one does, so this allows us to import theirs. Doing a promise one (which is
> > actually what I originally started to do!) would make our tests incompatible.
> 
> I see, I'll comment on the GitHub issue.
> It seems more webby to have it promise based, even if it is just a testing
> thing.

I agree, but Internals methods usually don't have the constraint of being web.

In the meantime, I don't think that should hold this up. (In fact I'm sure it shouldn't!)

It's trivial to make promise-based later.

> > > m_workersByID.add is probably sufficient.
> > > Or you could add and verify that the result is a new entry.
> > 
> > Set is more efficient than add, though?
> 
> I recently checked that add is more efficient.

How is that possible? Seems to go against conventional wisdom, based on the overhead of the return value alone.

[Brady Eidson]

(In reply to Chris Dumez from comment #17)
> > > > m_workersByID.add is probably sufficient.
> > > > Or you could add and verify that the result is a new entry.
> > > 
> > > Set is more efficient than add, though?
> > 
> > I recently checked that add is more efficient.
> 
> Agreed:
> template<typename KeyArg, typename MappedArg, typename HashArg, typename
> KeyTraitsArg, typename MappedTraitsArg>
> template<typename K, typename V>
> auto HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg,
> MappedTraitsArg>::inlineSet(K&& key, V&& value) -> AddResult
> {
>     AddResult result = inlineAdd(std::forward<K>(key),
> std::forward<V>(value));
>     if (!result.isNewEntry) {
>         // The inlineAdd call above found an existing hash table entry; we
> need to set the mapped value.
>         result.iterator->value = std::forward<V>(value);
>     }
>     return result;
> }
> 
> a set() is an add() + a branch and an assignment.

Okay, building set on top of add has this make sense.

I was expecting (and others over the years) that add was built on top of set.

Will change in a bit.

[Brady Eidson]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 328038 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328038&action=review
> 
> > Source/WebCore/testing/Internals.cpp:258
> > +#if ENABLE(SERVICE_WORKER)
> 
> Not needed.
> 
> > Source/WebCore/workers/service/server/SWServer.h:182
> > +    HashMap<ServiceWorkerIdentifier, SWServerWorker*> m_workersByID;
> 
> I do not see anybody removing workers from this HashMap anymore.
> 
> Could this still use a Ref<> as value?
>

*sigh*

With a lot of other stuff landed today I have some rejects from reapplying patches.

Will revisit soon.
 
> > Source/WebCore/workers/service/server/SWServer.h:184
> > +    HashMap<ServiceWorkerIdentifier, Ref<SWServerWorker>> m_terminatingWorkersByID;
> 
> Do we really need these 2 hash maps? Why can't we look up the worker in
> m_workersByID and check its state?

Short answer  - no.

Long answer will come later tonight, as I am now out of time for the work day.

[youenn fablet]

(In reply to Brady Eidson from comment #18)
> (In reply to youenn fablet from comment #15)
> > > For testing, sync terminate is much more reliable. Also, it's what the Blink
> > > one does, so this allows us to import theirs. Doing a promise one (which is
> > > actually what I originally started to do!) would make our tests incompatible.
> > 
> > I see, I'll comment on the GitHub issue.
> > It seems more webby to have it promise based, even if it is just a testing
> > thing.
> 
> I agree, but Internals methods usually don't have the constraint of being
> web.
> 
> In the meantime, I don't think that should hold this up. (In fact I'm sure
> it shouldn't!)

Sure

> > > > m_workersByID.add is probably sufficient.
> > > > Or you could add and verify that the result is a new entry.
> > > 
> > > Set is more efficient than add, though?
> > 
> > I recently checked that add is more efficient.
> 
> How is that possible? Seems to go against conventional wisdom, based on the
> overhead of the return value alone.

Agreed that it is counter intuitive.

[EWS Watchlist]

Comment on attachment 328038 [details]
Patch

Attachment 328038 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5427205

Number of test failures exceeded the failure limit.

[EWS Watchlist]

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

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

[youenn fablet]

I also wonder whether that is something that web developers would like to test again.
Say a user clears all its history and so on, thus potentially terminating all service workers. Maybe a web dev might want to simulate service worker termination through web driver.

[EWS Watchlist]

Comment on attachment 328038 [details]
Patch

Attachment 328038 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5427738

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 328056 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

According https://github.com/w3c/web-platform-tests/issues/6866, Chrome implementation is apparently not waiting for the worker to be stopped and tests do not seem to rely on that. We should probably go with the promise-based version.

[Brady Eidson]

(In reply to youenn fablet from comment #27)
> According https://github.com/w3c/web-platform-tests/issues/6866, Chrome
> implementation is apparently not waiting for the worker to be stopped and
> tests do not seem to rely on that. We should probably go with the
> promise-based version.

Sounds like they intended it for it to be synchronous but failed, and there's agreement for things to be promise based.

That's great, but for reasons of making progress on the feature part of the work in this patch that has been problematic for multiple days, I don't intend to make that change now.

I'll do it in a followup.

[Brady Eidson]

Created attachment 328073 [details]
Patch

[Brady Eidson]

So the answer for "Why not use CompletionHandler?" is this:

CompletionHandler assumes that:
1 - There is *ALWAYS* a callback provided
2 - It is called exactly once

My use of WTF::Function combined with null checking it was predicated on expecting that sometimes a callback is *not* provided.

When I switched to CompletionHandler here I still also had to null check it before calling which means I was using a heavier weight class than Function for no reason.

[Brady Eidson]

Had some tweaks to the patch discussed in person with Chris, incoming.

[Brady Eidson]

Created attachment 328122 [details]
Patch

[Brady Eidson]

Created attachment 328123 [details]
Patch

[Chris Dumez]

Comment on attachment 328123 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 328123 [details]
Patch

Clearing flags on attachment: 328123

Committed r225403: <https://trac.webkit.org/changeset/225403>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35801766>