Closed Bug 1425316 Opened 2 years ago Closed 2 years ago

move ShouldReportToWindow() off ServiceWorkerManager and into the outer window

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(6 files, 6 obsolete files)

6.40 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.42 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.71 KB, patch
asuth
: review+
Details | Diff | Splinter Review
5.04 KB, patch
asuth
: review+
Details | Diff | Splinter Review
3.04 KB, patch
asuth
: review+
Details | Diff | Splinter Review
8.70 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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.
Priority: -- → P2
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.
Attachment #8937151 - Flags: review?(bugmail)
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.
Attachment #8937156 - Flags: review?(bugmail)
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.
Attachment #8937163 - Flags: review?(bugmail)
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.
Attachment #8937547 - Flags: review?(bugmail)
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.)
Attachment #8937548 - Flags: review?(bugmail)
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.
Attachment #8937549 - Flags: review?(bugmail)
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.)
Attachment #8937151 - Flags: review?(bugmail) → review+
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.
Attachment #8937156 - Flags: review?(bugmail) → review+
Attachment #8937163 - Flags: review?(bugmail) → review+
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.)
Attachment #8937547 - Flags: review?(bugmail) → review+
Attachment #8937548 - Flags: review?(bugmail) → review+
Attachment #8937549 - Flags: review?(bugmail) → review+
(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
You need to log in before you can comment on or make changes to this bug.