1425316 - move ShouldReportToWindow() off ServiceWorkerManager and into the outer window

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently the devtools console API in the child process calls:

  nsIServiceWorkerManager.shouldReportToWindow(winProxy, scope)

The SWM then tries to determine if the given window cares about the given SW scope.

This is problematic given we want to remove SWM from the child process.  Also, the SWM relies on some nsIDocument pointers and other local state to perform this check.

Normally I would try to pipe this stuff through ClientHandle, but we cannot here.  The shouldReportToWindow() method is synchronous which means we can't do any IPC.

Therefore we need to replace this code with child-process state maintained within the window DOM tree.  This is mostly there with the ClientSource and LoadInfo references to ServiceWorkerDescriptor.  The one thing we need to add is a list of scopes which the document has registered for.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose a chrome-only Window.shouldReportForServiceWorkerScope() method that initially forwards to ServiceWorkerManager. r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Improve nsGlobalWindowInner::CallOnChildren() to take variable arguments and allow aborting iteration if false is returned. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose a chrome-only Window.shouldReportForServiceWorkerScope() method that initially forwards to ServiceWorkerManager. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Improve nsGlobalWindowInner::CallOnChildren() to take variable arguments and allow aborting iteration if false is returned. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make nsGlobalWindowInner::ShouldReportForServiceWorkerScope() handle checks for controlled windows. r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Improve nsGlobalWindowInner::CallOnChildren() to take variable arguments and allow iteration to be aborted. r=asuth

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make nsGlobalWindowInner::ShouldReportForServiceWorkerScope() handle checks for controlled windows. r=asuth

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Note that a ClientSource has called register() for a SW scope and use it to match window console reports. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=874271ea5d20794b7979c4f3c36c1101b9585a92

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Note that a ClientSource has called register() for a SW scope and use it to match window console reports. r=asuth

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Match service worker console reports against currently loading navigation channel. r=asuth

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Remove ServiceWorkerManager::ShouldReportToWindow(). r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5312bc0c8afe1cf7d62d340398118b44b3a09f18

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937151 [details] [diff] [review]
P1 Expose a chrome-only Window.shouldReportForServiceWorkerScope() method that initially forwards to ServiceWorkerManager. r=asuth

As a first step to removing nsIDocument and other direct child-side references from ServiceWorkerManager I want to remove ServiceWorkerManager::ShouldReportToWindow().  This is called by the console service when it receives a console message.  It asks the SWM if the given scope is associated with the given window.  To remove this code from SWM we need to make it ask the window directly instead.

So the first step here is to add a chrome-only method that console service can use instead of the old nsIServiceWorkerManager interface.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937156 [details] [diff] [review]
P2 Improve nsGlobalWindowInner::CallOnChildren() to take variable arguments and allow iteration to be aborted. r=asuth

The window-based ShouldReportForServiceWorkerScope() will need to determine if it cares about the given scope.  This means not only checking its own information, but also iterating over all child frames.

To make this easier in later patches this patch improves CallOnChildren().  It makes it:

1. Support variadic arguments
2. Support short-circuiting iteration if a method returns a certain value

I didn't want to make things like Suspend() and Resume() return the CallState type, so CallOnChildren does some template type inference to only do the short-circuit checking if CallState is returned.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937163 [details] [diff] [review]
P3 Make nsGlobalWindowInner::ShouldReportForServiceWorkerScope() handle checks for controlled windows. r=asuth

Next we call a ShouldReportForServiceWorkerScopeInternal() method which uses CallOnChildren() to apply it to all child frames as well.

For now ShouldReportForServiceWorkerScopeInternal() only checks if the window is controlled by a service worker of the given scope.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937547 [details] [diff] [review]
P4 Note that a ClientSource has called register() for a SW scope and use it to match window console reports. r=asuth

ServiceWorkerManager::ShouldReportToWindow() also reports for windows that have called register() for the scope, but are not controlled.  Unfortunately we don't have any child-side data for this currently.

This patch adds a NoteCalledRegisterForServiceWorkerScope() method on the window which in turn tracks registering scope on the ClientSource.  It then uses these values to check if scope matches in ShouldReportForServiceWorkerScopeInternal().

I put the list on ClientSource since I expect to track this for workers in the future as well.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937548 [details] [diff] [review]
P5 Match service worker console reports against currently loading navigation channel. r=asuth

The final check in ServiceWorkerManager::ShouldReportToWindow() that we need to duplicate is matching windows that are currently navigating and in the middle of a FetchEvent on the given service worker.

This is a bit tricky since right now the interception is done on the child, but in the future it won't be.  To handle this I chose to simply find in-progress navigation channels and match their URL against the scope.  Its not exactly the same as tracking channels that are actually intercepted, but its close enough for console reporting.

(Note, this part of our console reporting is not covered by a test AFAICT.)

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937549 [details] [diff] [review]
P6 Remove ServiceWorkerManager::ShouldReportToWindow(). r=asuth

Finally, remove the remains of the ServiceWorkerManager::ShouldReportToWindow() method.  Sadly we can't remove any data members yet as they are all still used by the ReportToAllClients() infrastructure right now.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Of course, now I'm wondering if this code will be used in the long run.  Once we move to the an out-of-process service worker we probably will need to change this console.log() code:

https://searchfox.org/mozilla-central/source/dom/console/Console.cpp#587

To instead send a service worker manager actor message.  But I'm not 100% sure I understand the console code.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

But its worth doing this for now until we are closer to moving SWM to a separate process.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8937151 [details] [diff] [review]
P1 Expose a chrome-only Window.shouldReportForServiceWorkerScope() method that initially forwards to ServiceWorkerManager. r=asuth

Review of attachment 8937151 [details] [diff] [review]:
-----------------------------------------------------------------

(webidl changes are okay because you're a DOM peer and the hook is cool with that.)

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8937156 [details] [diff] [review]
P2 Improve nsGlobalWindowInner::CallOnChildren() to take variable arguments and allow iteration to be aborted. r=asuth

Review of attachment 8937156 [details] [diff] [review]:
-----------------------------------------------------------------

restating the template-fu as I understand it (and being aware that nfroyd already weighed in on IRC last week):
- The decltype at the call-site in CallOnChildren extracts the return value of the given method, and provides this as the first argument to the CallChild function template instantiation, which is the only thing that can't otherwise being inferred from the call invocation.
- Only one of the CallChild templates matches based on the return-type, thanks to std::enable_if and SFINAE (http://en.cppreference.com/w/cpp/language/sfinae) mooting the one where enable_if fails to match on the return type.

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8937547 [details] [diff] [review]
P4 Note that a ClientSource has called register() for a SW scope and use it to match window console reports. r=asuth

Review of attachment 8937547 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/clients/manager/ClientSource.h
@@ +62,5 @@
>  
>    ClientInfo mClientInfo;
>    Maybe<ServiceWorkerDescriptor> mController;
>  
> +  AutoTArray<nsCString, 1> mRegisteringScopeList;

Maybe add a comment like:
List of de-duplicated scopes that have ever begun registration on this client.  Scopes are not removed, so this could grow without bound in the face of malicious content code, but there are easier ways content could intentionally DoS the browser.

::: dom/workers/ServiceWorkerManager.cpp
@@ -3651,5 @@
>    uint64_t winId = targetWin->WindowID();
>  
> -  // Check our weak registering document references first.  This way we clear
> -  // out as many dead weak references as possible when this method is called.
> -  WeakDocumentList* list = mRegisteringDocuments.Get(aScope);

(Just affirming that the ServiceWorkerManager::ReportToAllClients logic contains the weak reference cleanup logic as well, so there's no cleanup issue.)

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #19)
> But its worth doing this for now until we are closer to moving SWM to a
> separate process.

Agreed.  All direct docshell/window-to-SWM direct interaction/coupling is helpful to minimize or remove.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Note that a ClientSource has called register() for a SW scope and use it to match window console reports. r=asuth

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56b3aab1d763
P1 Expose a chrome-only Window.shouldReportForServiceWorkerScope() method that initially forwards to ServiceWorkerManager. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/81fe3f741dc3
P2 Improve nsGlobalWindowInner::CallOnChildren() to take variable arguments and allow iteration to be aborted. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca80e585ab6d
P3 Make nsGlobalWindowInner::ShouldReportForServiceWorkerScope() handle checks for controlled windows. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c08edd442321
P4 Note that a ClientSource has called register() for a SW scope and use it to match window console reports. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2c417bbe76
P5 Match service worker console reports against currently loading navigation channel. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2457625325
P6 Remove ServiceWorkerManager::ShouldReportToWindow(). r=asuth

Comment 26

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/56b3aab1d763
https://hg.mozilla.org/mozilla-central/rev/81fe3f741dc3
https://hg.mozilla.org/mozilla-central/rev/ca80e585ab6d
https://hg.mozilla.org/mozilla-central/rev/c08edd442321
https://hg.mozilla.org/mozilla-central/rev/8c2c417bbe76
https://hg.mozilla.org/mozilla-central/rev/1e2457625325