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

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.

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

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

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

Pushed by bkelly@mozilla.com: 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

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