Bug 180170 - Add Internals.terminateServiceWorker, and the ability to restart service workers for postMessage
Summary: Add Internals.terminateServiceWorker, and the ability to restart service work...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-29 15:23 PST by Brady Eidson
Modified: 2017-12-01 11:46 PST (History)
13 users (show)

See Also:


Attachments
Patch (46.71 KB, patch)
2017-11-30 13:37 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (45.26 KB, patch)
2017-11-30 13:41 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (45.29 KB, patch)
2017-11-30 14:00 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (46.67 KB, patch)
2017-11-30 15:03 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (47.26 KB, patch)
2017-11-30 15:23 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.45 MB, application/zip)
2017-11-30 16:19 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.80 MB, application/zip)
2017-11-30 16:52 PST, Build Bot
no flags Details
Patch (47.80 KB, patch)
2017-11-30 22:25 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (47.41 KB, patch)
2017-12-01 10:48 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (47.47 KB, patch)
2017-12-01 11:02 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-11-29 15:23:30 PST
Add Internals.terminateServiceWorker, and the ability to restart service workers for postMessage
Comment 1 Brady Eidson 2017-11-30 13:37:29 PST
Created attachment 328016 [details]
Patch
Comment 2 Brady Eidson 2017-11-30 13:41:53 PST
Created attachment 328017 [details]
Patch
Comment 3 Brady Eidson 2017-11-30 14:00:35 PST
Created attachment 328020 [details]
Patch
Comment 4 Brady Eidson 2017-11-30 14:11:42 PST
EWS on this might depend on the patch in 180216 landing first - but this is still reviewable
Comment 5 Chris Dumez 2017-11-30 14:17:59 PST
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.
Comment 6 youenn fablet 2017-11-30 14:22:48 PST
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.
Comment 7 Chris Dumez 2017-11-30 14:28:21 PST
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?
Comment 8 Brady Eidson 2017-11-30 14:30:18 PST
(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
Comment 9 Chris Dumez 2017-11-30 14:31:50 PST
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
Comment 10 Brady Eidson 2017-11-30 14:36:37 PST
(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?
Comment 11 Brady Eidson 2017-11-30 14:40:30 PST
(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.
Comment 12 Brady Eidson 2017-11-30 14:41:57 PST
(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.
Comment 13 Brady Eidson 2017-11-30 15:03:46 PST
Created attachment 328037 [details]
Patch
Comment 14 Brady Eidson 2017-11-30 15:23:41 PST
Created attachment 328038 [details]
Patch
Comment 15 youenn fablet 2017-11-30 15:33:43 PST
> 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.
Comment 16 Chris Dumez 2017-11-30 15:38:50 PST
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?
Comment 17 Chris Dumez 2017-11-30 15:40:47 PST
> > > 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.
Comment 18 Brady Eidson 2017-11-30 15:43:07 PST
(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.
Comment 19 Brady Eidson 2017-11-30 15:44:12 PST
(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.
Comment 20 Brady Eidson 2017-11-30 15:45:34 PST
(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.
Comment 21 youenn fablet 2017-11-30 16:19:07 PST
(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.
Comment 22 Build Bot 2017-11-30 16:19:10 PST
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.
Comment 23 Build Bot 2017-11-30 16:19:12 PST
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
Comment 24 youenn fablet 2017-11-30 16:22:45 PST
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.
Comment 25 Build Bot 2017-11-30 16:52:28 PST
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.
Comment 26 Build Bot 2017-11-30 16:52:30 PST
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
Comment 27 youenn fablet 2017-11-30 20:48:43 PST
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.
Comment 28 Brady Eidson 2017-11-30 21:17:08 PST
(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.
Comment 29 Brady Eidson 2017-11-30 22:25:26 PST
Created attachment 328073 [details]
Patch
Comment 30 Brady Eidson 2017-11-30 22:27:28 PST
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.
Comment 31 Brady Eidson 2017-12-01 09:13:12 PST
Had some tweaks to the patch discussed in person with Chris, incoming.
Comment 32 Brady Eidson 2017-12-01 10:48:48 PST
Created attachment 328122 [details]
Patch
Comment 33 Brady Eidson 2017-12-01 11:02:10 PST
Created attachment 328123 [details]
Patch
Comment 34 Chris Dumez 2017-12-01 11:26:54 PST
Comment on attachment 328123 [details]
Patch

r=me
Comment 35 WebKit Commit Bot 2017-12-01 11:45:46 PST
Comment on attachment 328123 [details]
Patch

Clearing flags on attachment: 328123

Committed r225403: <https://trac.webkit.org/changeset/225403>
Comment 36 WebKit Commit Bot 2017-12-01 11:45:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Radar WebKit Bug Importer 2017-12-01 11:46:24 PST
<rdar://problem/35801766>