1293277 - service worker Client interface and APIs won't work with multiple content processes

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Service workers includes an API like this:

  clients.matchAll({ includeUncontrolled }).then(list => {
    // list is ordered such the most recently focused window is first
    list.forEach(client => {
      // snapshot information about each client
      console.log('Found client - url:' + client.url +
                  ' visible:' + client.visibilityState +
                  ' focused:' + client.focused);

      // can also operate on the client
      if (url === myWindowURL) {
        client.navigtate(newURL);
      } else if (url === interestingURL) {
        client.focus();
      } else if (url === messageBuddy) {
        client.postMessage(msg);
      }
    });
  });

We implement this today, but our current solution will only work in non-e10s or e10s with a single child process.

To make this work for multiple content processes we need:

1) A way to find all windows (top level and nested) for a particular origin across the entire browser.
2) A way to navigate/focus each window
3) A way to postMessage to each window

In the future we will need to support Workers as clients as well, but today we only support windows.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Jim, this is one where we could probably use some help.  We're not sure whats available currently for locating and operating on windows across different child processes.  What do you think?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Ben Kelly [:bkelly] from comment #0)
> To make this work for multiple content processes we need:
> 
> 1) A way to find all windows (top level and nested) for a particular origin
> across the entire browser.
> 2) A way to navigate/focus each window
> 3) A way to postMessage to each window

As far as I know, we don't have anything like this right now. It would have to be implemented fresh.

Comment 3

[Jim Mathies [:jimm]]

I don't know of anything. The multi e10s team might be able to help, adding to their triage list.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I have some ideas about how I want to implement this.  I'm going to take it for now.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Snapshot of my work-in-progress.  Nothing functional yet.  Its mainly just IPC boilerplate scaffolding so far.

Basic structure:

* There is a thread-local-singleton called ClientManager.
* The ClientManager creates a PClientManager actor using PBackground to reach the real service singleton.
* The ClientManager can be used by windows/workers to create a ClientSource.
* The ClientSource is a ref-counted object representing the client entity.  When it is destroyed the client is considered removed from the system.
* The ClientManager can be used to query what clients are available.  These are returned as ClientHandle objects.
* The ClientHandle can be used to operate on the client.  It will be notified when the underlying client is destroyed.
* Since these are PBackground actors they are locked to their creation threads.
* ClientManager will support serializing ClientHandles to ClientToken objects.
* A ClientToken can be passed cross thread and process boundaries and use to re-create a ClientHandle on the other side.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Updated work-in-progress that creates PClientSource actors for windows and workers.  I verified via debug that the actors are correctly created and destroyed as you navigate around.  I also verified that they are properly destroyed and re-created when going into and out of bfcache.

This still uses the "document is a Client" definition for windows, but I plan to fix that to conform to the spec.  I need to check the spec a bit more closely.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Note, the current WIP changes the plan in comment 5 such that ClientSource is now a UniquePtr<> instead of ref-counted.  This helps avoid accidentally keeping the RAII style object alive too long.

Also, a design note:

The ClientSource could be separated from ClientManager.  The actors really could live directly under PBackground right now.  I am keeping the hierarchy with ClientManager, though, so that in the future we can more easily support an optimization where a ClientHandle directly references a ClientSource in the same process and thread.  This is only possibly if we have a child-side place to do maintain a cache of sources, though.  That place is the ClientManager object.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Some notes so I might remember what I am doing next week:

In order to do the reserved Client stuff correctly we'll need to create the Client earlier than we do today.  Right now we don't make a document "visible" as a Client until nsDocument::SetScriptGlobalObject().  We need to create the reserved Client, however, *before* the nsIChannel is created in nsDocShell.  We then need to propagate the Client through until we have an active window/document.  So this means somewhere in nsDocShell::InternalLoad() or nsDocShell::DoURILoad().

For workers we probably will need to create the Client in or before ScriptLoader.  Workers have an added problem, however, in that they do not have their principal or URL fully set at that point.  We may need to initialize a request principal/URL and then updated them after we complete the script nsIChannel.  This has a spec issue too:

https://github.com/w3c/ServiceWorker/issues/1034

When a window and its workers are frozen to be put into the bfcache we will need to "hide" the associated Client.  We should not destroy the Client, however, as we will want to re-use the same Client and its ID if the window/workers are thawed.

Finally, there are some problems with the spec around reserved clients during redirects:

https://github.com/w3c/ServiceWorker/issues/1031

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Updated work-in-progress that works with the patches in bug 1333573.  We can now reserve the Client before the initial worker script network request.

Next I'll make sure we can do the same for window Client objects as well.  This will mean disentangling where in the nsGlobalWindow/nsDocShell/nsDocument web the Client needs to live.  Right now we perform Client creation/destruction in nsDocument, but thats probably not right.

I think we will probably want to move it to nsGlobalWindow.  For example, if a page is put into the bfcache and then comes back out it would be nice to maintain the exact same Client object and ID.  This suggests we want it to live on nsGlobalWindow.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Note to self, this is the best comment describing how the various Clients work during a network request:

https://github.com/w3c/ServiceWorker/issues/870#issuecomment-245559217

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Boris suggests that we get the ClientSource from the NS_NewChannel() location to the creation of the inner window via the nsILoadInfo.

Comment 13

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #12)
> Boris suggests that we get the ClientSource from the NS_NewChannel()
> location to the creation of the inner window via the nsILoadInfo.

Is ClientSource IPC serializable?  IINM this would require serializing it and sending it to the parent as part of the load info serialization.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #13)
> Is ClientSource IPC serializable?  IINM this would require serializing it
> and sending it to the parent as part of the load info serialization.

We will send an nsID representing the Client.  ClientSource itself cannot be passed because PClientSource is a PBackground actor and the channel is a PContent actor.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Updated work-in-progress that also creates the ClientSource before the docshell load and passes the reserved client on to the resulting inner window.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Update work-in-progress with code to set the window Client ExecutionReady.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Update work-in-progress to reflect how mutable client state will be propagated.

My original idea was to push this information to the parent as it happen.  Given this information includes visibility data, though, I think this might be quite excessive.  Scrolling through twitter where there are tons of iframes would create a lot of IPC traffic.

So for now, lets plan to ping each ClientSource for its state before we return it to the Clients API.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Other thoughts recently:

I think we should expose a snapshot ClientInfo structure.  This can be attached to nsILoadInfo or nsIChannel to reflect Client related information like principal, creation URL, etc.  These could be produced equally from window and worker instead of our current nsIDocument vs WorkerPrivate nightmare.  Since its a snapshot and not a live actor it can be safely passed across threads.

Over time we could add things like referrer-policy and CSP to the ClientInfo structure.  The network stack could also get its triggering principal from the ClientInfo, etc.

Also, since the Clients API only exposes snapshots of Clients it would be adequate for implementing that API.

In order to replace nsIDocument in ServiceWorkerManager (bug 1231218), however, we will need a live actor and not just a snapshot.  The ServiceWorkerManager will need the actor destruction message to know when Clients go away.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Incorporate focus time now that bug 1266747 added it to the document.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Update to set the Client as controlled.  So far this only handles window clients.  I need to figure out a way to mark the Worker Client object as controlled as well.  Its tricky because the network request and SWM all happen on a different thread.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #20)
> Update to set the Client as controlled.  So far this only handles window
> clients.  I need to figure out a way to mark the Worker Client object as
> controlled as well.  Its tricky because the network request and SWM all
> happen on a different thread.

So, I was thinking I could make a WorkerClientSourceMainThreadProxy object that gets attached to the LoadInfo, but that will stop working once we move interception to the parent process.

Instead I think I should make the interception code use ClientManager its IPC actors to mark the Client controlled.  This could be a method on PClientManager or a method on a separate PClientHandle actor.  The separate actor may be overkill here, but we will eventually need it for other things in order to complete the multi-e10s refactor.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Starting the ClientHandle here.  I am duplicating a lot of code with the ClientSource.  I'll try to de-duplicate that once things are stable.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Ok, this handles marking Client objects controlled from the ServiceWorkerManager using a ClientHandle actor.  This means it works for Window clients and the cases where we currently support controlling Worker clients.

I say "where we currently support", because our coverage for controlling worker clients sucks.  For example, clients.claim() and skipWaiting() will not start controlling a worker today.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Updated to actually attached the ClientHandle to the ClientSource in the parent service backend.  The "controlled" message is now completely routed from the handle back to the source.

Note, we probably still need to mark nsIChannels as controlled when we implement more of the e10s refactor.  These changes are not enough because the CLientHandle-to-ClientSource path can be too slow in order to know we need to intercept an immediate <script> or importScripts() request.  With debugging today I can see we mark workers execution ready before we get the controlled message to worker ClientSource (perhaps because of the loading sync loop).

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This updates the patch to set mark the nsIChannel's load info with the controlling service workers scope and ID.  The window and worker then get this info out and update the ClientSource immediately.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Makes navigator.serviceWorker.controller use the window's ClientSource->GetControlled() data instead of the document-to-registration map.

This almost passes all WPT tests, but fails:

/service-workers/service-worker/skip-waiting-installed.https.html
-----------------------------------------------------------------
FAIL Test skipWaiting when a installed worker is waiting
/service-workers/service-worker/skip-waiting-using-registration.https.html
--------------------------------------------------------------------------
TIMEOUT [Parent]

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #27)
> /service-workers/service-worker/skip-waiting-installed.https.html
> -----------------------------------------------------------------
> FAIL Test skipWaiting when a installed worker is waiting
> /service-workers/service-worker/skip-waiting-using-registration.https.html
> --------------------------------------------------------------------------
> TIMEOUT [Parent]

These are likely due to ServiceWorkerClients caching the controller object and we don't clear it when ClientSource::Controlled() is called.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This triggers the controller change logic in ServiceWorkerContainer when the ClientSource::Controlled() method is called.  It also avoids the old paths that trigger the controller change directly from SWM.

This lets the patch pass both WPT and mochitests locally.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Note to self about work remaining:

1) Replace ClientControlledArgs with a ServiceWorkerDescriptor IPC type.  (ServiceWorkerInfo is already taken....)
2) Change the signals from Controlled(ClientControlledArgs) to SetController(ServiceWorkerDescriptor).
3) Create an actor under PClientManager to represent querying for a list of ClientInfo objects.
  a) Query params like matchAll() options in actor constructore
  b) Find list of matching ClientSourceParent objects
  c) SendRequestState() to each ClientSourceParent
  d) Wait for RecvUpdateState() or actor destruction for each ClientSourceParent
  e) Sort list of ClientInfo+ClientState objects
  f) Send list back in actor __delete__ message
4) Finish implementing Freeze/Thaw of Clients
  a) Make sure we call Freeze/Thaw in appropriate places
  b) Hide frozen actors from ClientHandle and matchAll query
  c) Don't think we need to do anything about client controller here
5) Implement infrastructure for postMessage()
6) Implement infrastructure for focus()
7) Implement infrastructure for navigate()
8) Implement infrastructure for openWindow()
9) Start implementing Clients API on top of the infrastructure
10) Clean up patches for review

Steps (1), (2), (3), (4), and (9) could probably be done within the week.

But I can't skipp steps (5), (6), (7), and (8).  Those may take some thinking to figure out.  Difficult to say how close we are.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

More notes to self:

Things like focus(), navigate(), openWindow(), and matchAll() all require resolving a promise after the operation actually completes.  My initial thought was just to various async messages for this, but since we need to match things up at the end we should probably us actors to represent the operations.

So I need to build an "operation" actor system similar to Cache API's system:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/PCacheOp.ipdl

I will need a PClientHandleOp that goes from child-to-parent and a PCacheSourceOp that goes from parent-to-child.

Another unrelated thought:

Currently the wip patch maintains the WorkerHolder at the ClientManager level and depends on the PClientManager destroying all its child actors before giving up the WorkerHolder.  Because of this we may need to check if the PClientManagerParent is tearing down before creating a PClientSourceOp.  This may just be handled correctly by the teardown semantics, though.  I'll need to check.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rough-in all the boilerplate needed to add two more actors.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This mostly replaces the "control a Client" path with the new operation actor system.  It uses MozPromise to handle the result resolve/reject of the operation.  This is nice because it lets me fix the problem with the last patch where we did not wait for the Clients to actually be controlled before resolving the Clients.claim() promise.

The current patch has a bug, though, where it does not keep the ClientHandle alive if there is an operation in progress.  This means that when code creates a handle, starts an op, and then drops the handle ref it ends up canceling the operation.  To solve this I need to delay ClientHandle::StartTeardown() if any child operation has a populated mPromiseHolder.

I'll fix this tomorrow and then do some cleanup before moving to the other operations.  The good news is that once this is proved out the other operations should be significantly easier.  I just have to define args in, do the operation in the ClientSource, and define result out.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This patch keeps the ClientHandle object alive until the pending remote op is complete.  It once again passes service worker tests locally.

Note, the dom/workers/tests/serviceworkers/test_claim.html test is a bit broken and had to be fixed here.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Add focus operation to the infrastructure.  Can't test yet since I don't have the DOM API implemented yet.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Start implementing Navigate.  I had to refactor a bit more, though, since I realized I needed async operation all the way into ClientSource.

Also, this revealed a problem with MozPromise in worker thread.  I need to fix bug 1345251.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This implements the Client.navigate() op infrastructure.  I have low confidence that the nsIWebProgressListener I copied from the existing code is adequate here.  We need wait to resolve the promise until the new Client is ExecutionReady.

I'll most likely have some rework to do once I implement the DOM layer and can test this.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

In order to get Client.postMessage() right I need to do bug 1311324 first.  Right now we are lacking some WPT test coverage because we are not to spec any more.  Also, some of our postMessage() code looks wrong to me at the moment.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Partial implementation of postMessage().  Mostly just the receive-side done so far.  I still need to do the structured clone on the send-side.

Also, as mentioned I need to sort out bug 1311324 to make sure I'm implementing this right.  I think our current implementation is both of an old spec and bugged in a few ways.  For example, we set the evt.source to the controller of the client, but thats pretty clearly wrong.  I want to get the WPT tests green so I'm more confident I'm not regressing something.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Update the patch based on the two MessageEvent dependent bugs.  Also, implement the ServiceWorkerDescriptor type and use it.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Last patch mostly finishes infrastructure for postMessage().  I took a bit of a short-cut on the sending side by assuming the actor is already setup.  That will be the case for the ServiceWorker caller, but ultimately might need to get fixed if we expose this on main thread.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Add boilerplate for the Clients.matchAll() query.  This is a separate actor on the ClientManager since PClientOp is specific to individual Client(Handle|Source) objects.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Continue to flesh out the query path.  Still needs some work on the requesting side and in the service.  I need to convert ClientManager to use some of the convenience stuff I wrote for ClientHandle/ClientSource.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

I need the changes in bug 1340163 in order to get the origin out of a PrincipalInfo() OMT.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Implemented the service side of the matchAll() query.  Still needs some cleanup, but might work.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Start factoring out the async task execution code so it can be shared among ClientSource/ClientHandle/ClientManager via a base class.  Still some work to do, but I finally figured out the right sacrifices to appease the template gods.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This refactors things further so I can use the MaybeExecute() helpers in ClientManager.  It also implements the ClientManager::Query() logic.

Next step is to implement the DOM layer of the Clients API.  It should be a fairly thin wrapper around the infrastructure elements.  Then test, fix bugs, cleanup.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Actually include the important ClientThing.h header.

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Stub out the Clients and Client DOM classes.  This was a bit harder than I expected due to having to unlink all uses of the old classes.

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Started filling in the guts of the DOM classes and realized I need some more infrastructure for:

1) Clients.get()
2) Clients.claim()
3) Clients.openWindow()

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This implements the DOM layer for Client and part of Clients.  Unfortunately there some things on Clients I still need infrastructure for.  The first two should not be too bad:

1) Clients.claim() could probably be implemented on top of ClientManager::Query and ClientHandle::Control().  I need to add some code to notify ServiceWorkerManager of the change in control, though.
2) Clients.get() should just be a variant of the Query code I added for Clients.MatchAll().

The third though:

3) Clients.openWindow() could be very very hard.  Its unclear to me yet if we have infrastructure to do what I need here.

Clients.openWindow() needs:

1) Open a window in another child process since we don't want windows in the worker process.
2) Detect when window load is complete
3) Communicate the Client UUID back to the remote process that called openWindow() so we can construct a Client DOM object

I may need to build a new set of actors to do this.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Blake tells me there is a C++ API that I can use from the parent process that will help with openWindow();  ContentParent::GetNewOrUsedBrowserProcess():

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#796

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This converts MatchAll to use a more generic Op mechanism.  It also implements Clients.claim() on top of that mechanism.

Left to do:

* Clients.get()
* Clients.openWindow()
* Lots of cleanup work

The openWindow() changes might take a couple days.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This adds most of the guts for Clients.get().  Still a bit more to do on the initiation side to get the principal and ID correctly.

I'll probably try to get this stuff passing tests before I tackle openWindow().

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Updated to implement the rest of clients.get().

I also started trying to get this test to pass:

  service-workers/service-worker/clients-get.https.html

I found and fixed a few bugs from running this.  I also realized I'm missing a change.

The new design depends on a ClientInfo being stored in the nsIChannel's nsILoadInfo.  I have code that sets the reserved client's ClientInfo, but not the initiating ClientInfo.  So I need to modify NS_NewChannel() to do that.

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

I think for loads with an associated nsINode I can automatically set the ClientInfo here:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#880

I'm not sure how to set the ClientInfo for network requests on Workers, though.  Workers are not well integrated into NS_NewChannel().  I will probably have to do a bunch of lame post-creation modifications to the nsILoadInfo for all the different places we initiated network from workers.  This will be error prone, unfortunately.

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This gets the clients-get.https.html test to pass.  It did reveal some potential issues in the spec, though:

https://github.com/whatwg/html/issues/2456
https://github.com/w3c/ServiceWorker/issues/1091

I implemented what makes sense given the current spec, but I imagine it might change in ways that could be hard to implement.

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This gets clients-matchall.https.html passing.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This gets postmessage-to-client.https.html to pass.  Turns out I forgot to implement Client::PostMessage(), although most of the infrastructure was there.

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Make claim-fetch.https.html pass.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

These are the WPT tests still failing:

Unexpected Results
==================

/service-workers/service-worker/claim-using-registration.https.html
-------------------------------------------------------------------
FAIL Test for the waiting worker claims a client which is using the the same registration
/service-workers/service-worker/client-navigate.https.html
----------------------------------------------------------
TIMEOUT Frame location should update on successful navigation
NOTRUN Frame location should not be accessible after redirect
NOTRUN Frame location should not update on failed navigation
TIMEOUT [Parent]
/service-workers/service-worker/clients-matchall-client-types.https.html
------------------------------------------------------------------------
TIMEOUT expected FAIL Verify matchAll() with window and sharedworker client types
ERROR [Parent]
/service-workers/service-worker/clients-matchall-order.https.html
-----------------------------------------------------------------
FAIL Clients.matchAll() returns controlled windows in focus order.  Case 1.
FAIL Clients.matchAll() returns controlled windows in focus order.  Case 2.
FAIL Clients.matchAll() returns non-focused uncontrolled windows in creation order.
FAIL Clients.matchAll() returns uncontrolled windows in focus order.  Case 1.
FAIL Clients.matchAll() returns uncontrolled windows in focus order.  Case 2.
FAIL Clients.matchAll() returns controlled windows and frames in focus order.
/service-workers/service-worker/fetch-event.https.html
------------------------------------------------------
FAIL Service Worker responds to fetch event with an existing client id
/service-workers/service-worker/navigate-window.https.html
----------------------------------------------------------
FAIL Clients.matchAll() should not show an old window as controlled after it navigates.
FAIL Clients.matchAll() should not show an old window after it navigates.

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This gets navigate-window.https.html test to pass.  I had forgotten to complete the Freeze/Thaw implementation and bfcache Clients were showing up in the list.

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This gets clients-matchall-order.https.html to pass.  The sorting code was wrong.

This also gets fetch-event.https.html.  The test incorrectly checks to make sure a non-subresource fetch event has a null client ID.  This is not correct according to the current spec.  I will probably break this out into a separate bug.

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This gets clients-matchall-client-types.https.html passing.  There were a few issues here:

1) The test timed out when it had a test assertion failure.  I fixed that to propagate the error.
2) We were failing because the shared worker Client.url was wrong.  It was not resolved to the full path.  I fixed Client.url to use the location.href for workers.
3) We then passed the test for the first time, so I removed the failure expectation.

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

The clients-navigate.https.html failures are due to a bigger problem.  Currently navigate is designed to work like this:

1) Client::Navigate() effectively creates a PClientHandleOp that is owned by a PClientHandle
2) The PClientHandleOp actor is then connected to a PClientSource and creates a PClientSourceOp
3) The PClientSourceOp then calls into the ClientSource representing the target window to navigate
4) ClientSource::Navigate() returns a MozPromise to the PClientSourceOp.  When the MozPromise resolves the op actors will be destroyed returning the op result.
5) ClientSource::Navigate() sets up a nsIWebProgressListener to wait for document load and then starts the load.  When the progress listener detect the load is complete it resolves the MozPromise.

The problem is that in step 5 we navigate the window.  This *destroys* the ClientSource that is in use.  This automatically aborts all operations in progress for the ClientSource.  This causes the Client.navigate() operation to fail.

I need to create some kind of operation actor associated with the PClientManager or PBackground itself.  I then need to get this new actor to work with the ClientSource in question to start the navigate, wait for the new window to load, and return info about the new ClientSource.

There is some overlap here with openWindow() in that its not an operation on the PClientSource actor.  I'm not sure I can really get any code re-use out of the two, though.  The openWindow functionality needs to work when there are no windows yet which means it must work without a PClientManager already existing.  I think openWindow will require a PContent actor for this.

So what I will probably do here is create a PClientManager navigation operation actor.  The target PClientSource actor can be passed as an argument to this new operation actor.

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

I did fix some other stupid bugs in the navigate path, though.

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

This error:

/service-workers/service-worker/claim-using-registration.https.html
-------------------------------------------------------------------
FAIL Test for the waiting worker claims a client which is using the the same registration

Is due to not rejecting the clients.claim() promise if the function is called on an inactive service worker.

Once the SWM is fully migrated to the Client and multi-e10s architecture I think this might be easier to solve.  At the moment, though, I will need to write some kludgy compat code to pull it out of the ServiceWorkerManager on the current main thread.

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #69)
> Once the SWM is fully migrated to the Client and multi-e10s architecture I
> think this might be easier to solve.  At the moment, though, I will need to
> write some kludgy compat code to pull it out of the ServiceWorkerManager on
> the current main thread.

Actually, we will eventually need this information synchronously in the service worker context anyway.  We will need it to implement self.registration.active.state which is a sync getter.  So perhaps I should just push this information to the WorkerPrivate now.

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

Current failures with the latest patch:

/service-workers/service-worker/claim-using-registration.https.html
-------------------------------------------------------------------
FAIL Test for the waiting worker claims a client which is using the the same registration
/service-workers/service-worker/client-navigate.https.html
----------------------------------------------------------
FAIL Frame location should update on successful navigation
FAIL Return value should be instance of WindowClient
FAIL Redirecting to another origin should resolve with null
FAIL Navigating to about:blank should reject with TypeError

And of course openWindow() is still completely unimplemented.

Comment 72

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Updated patch includes code to store a ServiceWorkerDescriptor on the WorkerPrivate.  This is only partly done.  I still need to propagate state changes from the ServiceWorkerInfo through the ServiceWorkerPrivate and up to the WorkerPrivate.

Once this is done I can check the state when skipWaiting() is called immediately to reject if necessary.

Comment 73

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This gets claim-using-registration.https.html to pass.  The only service worker WPT test failing now is:

/service-workers/service-worker/client-navigate.https.html
----------------------------------------------------------
FAIL Frame location should update on successful navigation
FAIL Return value should be instance of WindowClient
FAIL Redirecting to another origin should resolve with null
FAIL Navigating to about:blank should reject with TypeError

I need to do the complication navigation re-design to fix that, though.  I also need to implement openWindow().

I will be on travel for the next couple weeks, unfortunately.  I will try to work on them as I can, but may not make much progress until I return.  That might make fitting this in FF55 difficult since the merge is only another week after that.  I'll do my best, though.

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

We also fail at least this mochitest:

dom/workers/test/serviceworkers/test_match_all_client_properties.html

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This fixes client.navigate().  All WPT service worker tests now pass.

I still need to implement openWindow(), do clean up, and then flag for review.

Comment 76

[Ben Kelly [:bkelly, not reviewing]]

Blake pointed me to this in order to determine if Clients.openWindow() should potentially create a new child process:

http://searchfox.org/mozilla-central/source/xpcom/system/nsIXULRuntime.idl#104

Comment 77

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This fixes test_match_all_client_properties.html.  The test verifies we strip the {} characters off the UUID.  I fixed that in this update.

I also fixed a bug in focus(), but we still fail this test:

dom/workers/test/serviceworkers/test_notificationclick_focus.html

Its failing because focus() should fail if there hasn't been a notification click within a given time frame.  I need to add code to make that check.

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This fixes test_notificationclick_focus.html.

The next failing test is test_postmessage_advanced.html.  This one crashes.  I think the problem is that it tries to serialize a PBlob actor managed by PContent across the PClient's PBackground actor.

I'm not really sure how to handle that, unfortunately.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This fixes test_postmessage_advanced.html.  The problem was actually with how the clients code bridges the postMessage across the parent process.  When going from one PBackground manager to another we need rebuild the StructuredCloneData in the IPC args.

With this change the only remaining mochitest failure is in test_openwindow.html.  So implementing clients.openWindow() will be next.

Comment 80

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebase and update for corrected client-navigate.https.html test case in bug 1351935.

Comment 81

[Ben Kelly [:bkelly, not reviewing]]

I have some more work in progress for the openWindow() case, but nothing working yet.  I think its safe to say this will not be done in time for the merge next week.  Sorry.

Comment 82

[Ben Kelly [:bkelly, not reviewing]]

Attachment: openwindow-wip

Work-in-progress on the openWindow() code.  This pipes from the Clients.openWindow() call to the main thread on the parent process.  Next step is to create a PContent op to bridge to the child process.

Comment 83

[Ben Kelly [:bkelly, not reviewing]]

Attachment: openwindow-wip

This adds the PContent actor to bridge to the child process.  It also handles the "are we on e10s" check and possibly creating the child process if its not there yet.

Next I need to:

1) Copy the existing window opening code into a helper that can be called either from the parent or the child process.
2) Figure out how to gracefully handle browser shutdown with this setup.

Comment 84

[Ben Kelly [:bkelly, not reviewing]]

Attachment: openwindow-wip

This is a first cut at moving the openWindow() code over to the new model.  Currently crashes on tests.  Also I haven't even tried to support the android case yet.

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebase.

Comment 86

[Ben Kelly [:bkelly, not reviewing]]

Attachment: openwindow-wip

This gets about half of test_openWindow.html passing.  I had to fix the crashes, an infinite loop in the parent process, and a few issues in the test.  I think the remaining issues are related to not handling edge cases like about:blank correctly.

Note, a significant blocker to landing this work will be getting a Clients.openWindow() test on fennec.  I'm not familiar with the android test infrastructure at all.  It would be very helpful if we could get someone familiar with that side of things to write the test.  Its somewhat bad that we've been shipping this feature without a test on android.

Comment 87

[Ben Kelly [:bkelly, not reviewing]]

Attachment: openwindow-wip

This gets test_openWindow.html to pass in non-e10s.

It also passes all of the test cases in e10s, but gets confused about its final condition.  Its using global state which is broken by multi-e10s and our current setup where we spawn different service workers.  I still need to adjust the test to work around this.

Comment 88

[Ben Kelly [:bkelly, not reviewing]]

Attachment: openwindow-wip

This fixes test_openWindow.html in e10s mode.  I ended up adding a preference that makes openWindow() favor the current process.  This allows the tests to work correctly with its global state.  I did this since the same compat problem would probably break real sites as well.  In the future we can remove this code and use the normal remote process code instead.

Comment 89

[Ben Kelly [:bkelly, not reviewing]]

Attachment: openwindow-wip

Slightly better code for the "same process" case.

Next I will try to figure out android.

Comment 90

[Ben Kelly [:bkelly, not reviewing]]

I resurrected one of my fxos devices enough to run test_openWindow.html on an android device.  The good news is that it passes.  The bad news is it looks like we won't have any automation for background or not-running openWindow cases.  I'll have to manually validate those.

I'm still working on getting a good developer setup for android.

Comment 91

[Ben Kelly [:bkelly, not reviewing]]

Attachment: android.patch

Start of the android open window patch.

Comment 92

[Ben Kelly [:bkelly, not reviewing]]

Attachment: android.patch

This gets the last part of openWindow working on android.

Comment 93

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=974b0d43314f7b13c89c2d9d6ce15e60e8998597

Comment 94

[Ben Kelly [:bkelly, not reviewing]]

I have to do a bunch of rework because we apparently have all kinds of restrictions on where IPC types can be used.  I didn't hit this while developing because we only trigger these failures on windows platform.

Comment 95

[Ben Kelly [:bkelly, not reviewing]]

Attachment: grr

This fixes many of the windows.h issues, but there are still more.  I am going to see if I can fix it in the IPC layer instead in bug 1359230.

Comment 96

[Ben Kelly [:bkelly, not reviewing]]

This set of patches builds locally.  Lets see what try says:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81fbf2ca8bd6b7e12167cb7a5f315c9ef6335983

Comment 97

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c21dedc299345860fab1e3ef3ce4cb86b6d3b62

Comment 98

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 99

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 100

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=707e022dbe96ed3355d3cb7b8e068a5bae01df04

Comment 101

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8face6733eadc61dfe51368c90fae68ba31dcf7c

Comment 102

[Ben Kelly [:bkelly, not reviewing]]

Note to self:

We are getting leaks in:

devtools/client/projecteditor/test/browser_projecteditor_rename_file_01.js

Because this:

devtools/client/projecteditor/lib/stores/local.js

Creates chrome workers that it abandons, but does not terminate.  This means its depending on CC to clean up these workers in a timely manner.

With the clients patches, though, we have a WorkerHolder alive for the PClientManager actor.  This increases the busy count preventing the cycle collector from terminating the worker.

So the workers stay alive keeping the window alive until shutdown.

Comment 103

[Ben Kelly [:bkelly, not reviewing]]

Thing to try:  Call ModifyBusyCountFromWorker() in ClientManager code to force the busy count back down.

Comment 104

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6445c5e744eaa764ddb3056487fff7d88bba4b7a

Comment 105

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c6124893c069eb2dd7dccbbf5f69fbc8eec7754

Comment 106

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45f910689fe81b0bd57fec8385073c34cfaea495

Comment 107

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 108

[Ben Kelly [:bkelly, not reviewing]]

Last linux-only try was green.  Let's see what we get on all platforms.  Also, lets run some talos tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4a5e109034cb0f660d756db5e5c42ac7623246

Baseline talos push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33907ae5917b7c7a2b482ca7b6111ff63c306cc0

Comment 109

[Ben Kelly [:bkelly, not reviewing]]

There are some windows specific failures.  Fixes for that, but no talos in this build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f9985dd2cabf76be9b9341452a0ff45f93e8133

Comment 110

[Ben Kelly [:bkelly, not reviewing]]

Try to fix some of the initial about:blank replacement navigation case:

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

Documented my plan in this spec issue:

https://github.com/w3c/ServiceWorker/issues/1091#issuecomment-300817825

Comment 111

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 112

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 113

[Ben Kelly [:bkelly, not reviewing]]

So, my changes to handle initial about:blank iframe document replacement correctly break test_openWindow.html.

In particular, the redirect cases within that test depend on either one of the following happening:

1. The controlled status of the channel being propagated across a redirect.  For a navigation this shouldn't happen, but we do today AFAICT.  So if URL1 is intercepted, but then redirects to URL2 which is not, the final document is controlled.  That is wrong.
2. Or, the tests would work if we re-intercepted the redirected requests.  So URL1 is intercepted and URL2 is intercepted.  This is the correct thing to do, but we don't do it.  Bug 1363848 is filed to fix this.

Since my patches here fix (1), I need to fix (2) so that the tests will continue to pass with these patches.  Its also a pretty important compat bug, so its worthy fixing anyway.

Comment 114

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebased and folded patch queue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e1db0f46dc1837f86ad6e49adf3afb34673e58

Comment 115

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Fix a bug in the ClientChannelHelper code where we incorrectly always assumed we could use the initial ClientInfo.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=324501e4ba67f94216636b24a811509e04f69307

I still need to write a test for various about:blank initial document cases.  It seems we don't actually share the Client with the initial about:blank in many cases because the initial about:blank is created after we start the main document load.

Comment 116

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebase around some MozPromise changes.  Try with bug 1372962:

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

Comment 117

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This adds a WPT test for the client handling of about:blank replacement.  I posted it to WPT repo as well:

https://github.com/w3c/web-platform-tests/pull/6304

Let's see if I broke anything on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f4cc3bce5aed094364e73fe48d1556c9d7905db

Comment 118

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39ba1f5961757653e447865a3ec14412bbf80ccd

Comment 119

[Ben Kelly [:bkelly, not reviewing]]

Next task here is to make sure we are doing the right thing with bfcache.  We need to evict the window from bfcache if any operations are performed on its Client.  For example, if you try to postMessage() it, clients.matchAll() would have matched it, etc.  We should also evict if its controlled by a service worker that becomes redundant.

Comment 120

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Store the browser-wide Client data in a hashtable.  I was using a relatively stupid nsTArray as a placeholder before.

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

Comment 121

[Ben Kelly [:bkelly, not reviewing]]

Trying to get a build working without AbstractThread and proper runnable labeling:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=953d07c894e777755f343daef9a7392f7bcf66a5

Comment 122

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6765541479cf39889218a906339b76188cfa0042

Comment 123

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44359d72b00618116f4240f47d427d26c2710d42

Comment 124

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 125

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

This patch removes AbstractThread() in favor of nsIGlobalObject::EventTargetFor().  This should ensure runnables are labeled correctly.  Depends on bug 1379243.

Comment 126

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e3ad41906a3f083e362f1f660be6fb8614f524e

Comment 127

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ed7ece08529b9d8a450177fad960557b37a47a5

Comment 128

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebase

Comment 129

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebased.  It was surprisingly not bad to rebase and compiled relatively easily.

Lets see how green try still is:

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

Given I have been working on this for 11 months, I think I need to land what I have if try is green.  There will be TODO comments and things to polish, but this bug is blocking other e10s service worker fixes.

I hope to grace :baku with the joy of reviewing it.

Comment 130

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Fix some orange:

1. Follow new clients.openWindow() code to get proper container id.
2. Avoid referencing SWM off the main thread in a case a new WPT triggered.
3. Fix nsIDocShell.idl and nsILoadInfo.idl to use [noscript, nostdcall, notxpcom] instead of c++ virtual methods.  This was confusing the docshell vtbl.

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

Comment 131

[Ben Kelly [:bkelly, not reviewing]]

I will work on adding comments and patch splitting monday.

Comment 132

[Ben Kelly [:bkelly, not reviewing]]

Looks like one orange still left to fix:

/service-workers/service-worker/client-navigate.https.html | Frame location should update on successful navigation
assert_unreached: unexpected rejection: assert_equals: expected "https://web-platform.test:8443/service-workers/service-worker/resources/client-navigated-frame.html" but got "" Reached unreachable code 

Only on non-e10s.

Comment 133

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

The LoadInfo::mClientInfo was being lost across the internal redirect introduced by InterceptedHttpChannel.  We should copy this value in the LoadInfo copy constructor.

Comment 134

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Spent most of today fixing a non-e10s failure.  There was some subtle stuff broken by the extra InterceptedHttpChannel internal redirect.  The code now uses the ClientChannelHelper for both window and worker nsIChannels to handle redirects properly.

I would do another try build, but the tree has been closed most of today.

Comment 135

[Ben Kelly [:bkelly, not reviewing]]

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

One thing I'm curious about is how we are passing this assertion when InternalHttpChannel does its internal redirect:

https://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#687

I'll investigate that tomorrow.

Comment 136

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

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

Comment 137

[Ben Kelly [:bkelly, not reviewing]]

Try is green.  Going to start patch splitting for review.

Comment 138

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a ref-counted WorkerHolderToken() class. r=baku

Comment 139

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 140

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Expose a "parsed" ServiceWorkerState value. r=baku

Comment 141

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a ServiceWorkerDescriptor type to represent a thread-safe snapshot of a ServiceWorkerInfo. r=baku

Comment 142

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 143

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Use the ServiceWorkerDescriptor in existing WorkerPrivate and ServiceWorkerInfo code. r=baku

Comment 144

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 145

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ClientInfo and ClientState types. r=baku

Comment 146

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 147

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a ref-counted WorkerHolderToken() class. r=baku

Comment 148

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8920695 [details] [diff] [review]
P1 Add a ref-counted WorkerHolderToken() class. r=baku

Andrea, this patch adds a convenience class for using WorkerHolder objects.  It creates a ref-counted token that functions as a holder.  I find this easier to reason about and is a pattern I've used in dom/cache, etc.  Rather than implement this over and over I'd like to incorporate it in dom/workers.

Comment 149

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8920284 [details] [diff] [review]
P2 Expose a "parsed" ServiceWorkerState value. r=baku

This patch exposes a "parsed" ServiceWorkerState.  This is not strictly in the spec yet, but I've raised an issue here:

https://github.com/w3c/ServiceWorker/issues/1162

And it (or something like it) will be necessary if we ever do:

https://github.com/w3c/ServiceWorker/issues/1077

Note, this should be somewhat transparent to existing code since its not currently possible to get a ServiceWorker object that is in this state.  I need some initial value, though, since this bug is going to make us track the state from the beginning of the creation of the WorkerPrivate.

Comment 150

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a ServiceWorkerDescriptor type to represent a thread-safe snapshot of a ServiceWorkerInfo. r=baku

Comment 151

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a ServiceWorkerDescriptor type to represent a thread-safe snapshot of a ServiceWorkerInfo. r=baku

Andrea, this patch adds a new ServiceWorkerDescriptor type.  This is a simple structure that contains enough information to construct a ServiceWorker DOM object.  Its threadsafe and can be passed across process boundaries.

Note, this is not a replacement for ServiceWorkerInfo, but merely a way for us to reference a ServiceWorkerInfo in a separate process without a strict actor involved.  We can create the ServiceWorker DOM object, then build the actor to the real ServiceWorkerInfo, and update the state value if necessary.

The intent here is that nsGlobalWindow and WorkerPrivate will have a Maybe<ServiceWorkerDescriptor> to represent their controller.  This will then be set on the nsILoadInfo to indicate that the channel is controlled by a service worker.

Also note, I originally intended this to be a straight ipdl generated struct.  I had to ultimately build a wrapper class around the IPC type, though, because of windows.h pollution.  The ipdl generated code includes windows.h which makes the bindings code refuse to compile.  This was a major pain and this is the best solution I came up with after struggling with it for a couple weeks.

Comment 152

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8920340 [details] [diff] [review]
P4 Use the ServiceWorkerDescriptor in existing WorkerPrivate and ServiceWorkerInfo code. r=baku

This patch replaces various id and scope values in existing code in favor of the new ServiceWorkerDescriptor type.  This is partly to reduce duplication, but will also make it easier when we want to just copy "the service worker" around in later patches.  We can just grab the descriptor and go.

Comment 153

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ClientInfo and ClientState types. r=baku

Comment 154

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8920704 [details] [diff] [review]
P5 Add ClientInfo and ClientState types. r=baku

Andrea, this patch adds a few more structure types similar to ServiceWorkerDescriptor.

The ClientInfo is used to reference a global somewhere in the browser.  It uses a UUID to uniquely reference the global, but does not have a live actor associated with it.  The ClientInfo is designed to be a lightweight token that can be passed around.  Later patches will introduce an actor system for using the ClientInfo to attach to the global to control it (navigate, etc).

Note, the comments talk about some things happening at "execution ready" time.  This refers to this part of the spec:

https://html.spec.whatwg.org/#concept-environment-execution-ready-flag

Later patches will make this more clear where these values are actually set.

The ClientState, ClientWindowState, and ClientWorkerState are also lightweight structures.  The intent is to pass around a ClientState which can then be unpacked itno either a ClientWindowState or ClientWorkerState.

The ClientWindowState basically represents the readable attributes on WindowClient:

https://w3c.github.io/ServiceWorker/#client-interface

The ClientWorkerState does not really contain anything right now, but I'd like to keep it as a placeholder for completeness.  Its a bit easier to reason about various things in common code if we always have a state available.

In later patches ClientInfo will be used in a number of ways:

1. It will be available on nsGlobalWindow inner windows and WorkerPrivate.
2. It will be set on nsILoadInfo to represent the global triggering the network request.  This will be used to set FetchEvent.clientId for now, but later can be used to replace a lot of things we currently require nsIDocument to passed to NS_NewChannel() for.  For example, we could place CSP, referrer policy, etc on the ClientInfo.  It could also replace triggering principal as a separate argument.
3. The ClientInfo and a ClientState combined will be used to create a the dom Client or WindowClient object.
4. An internal manager API will be provided to query ClientInfo objects for a given principal anywhere in the system.  This is PBackground based and can be used anywhere that is available.

Comment 155

[Ben Kelly [:bkelly, not reviewing]]

Oh, I should also explain the directory structure I intend to use here:

 dom / client / api     - The DOM Clients API bits.
              / manager - An internal manager interface for tracking globals cross-process

Comment 156

[Andrea Marchesini [:baku]]

Comment on attachment 8920701 [details] [diff] [review]
P3 Add a ServiceWorkerDescriptor type to represent a thread-safe snapshot of a ServiceWorkerInfo. r=baku

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

::: dom/workers/ServiceWorkerDescriptor.cpp
@@ +65,5 @@
> +ServiceWorkerDescriptor::operator==(const ServiceWorkerDescriptor& aRight) const
> +{
> +  return mData->id() == aRight.mData->id() &&
> +         mData->scope() == aRight.mData->scope() &&
> +         mData->principalInfo() == aRight.mData->principalInfo();

no mData->state()? This could be confusing.

Comment 157

[Andrea Marchesini [:baku]]

Comment on attachment 8920340 [details] [diff] [review]
P4 Use the ServiceWorkerDescriptor in existing WorkerPrivate and ServiceWorkerInfo code. r=baku

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

good cleanup of the code.

Comment 158

[Andrea Marchesini [:baku]]

Comment on attachment 8920704 [details] [diff] [review]
P5 Add ClientInfo and ClientState types. r=baku

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

::: dom/clients/manager/ClientInfo.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

a new line here?

::: dom/clients/manager/ClientState.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

empty line here?

@@ +38,5 @@
> +  operator=(ClientWindowState&& aRight);
> +
> +  ~ClientWindowState();
> +
> +  explicit ClientWindowState(const IPCClientWindowState& aData);

can you move this near the other CTORs?

@@ +57,5 @@
> +// This class defines the mutable worker state we support querying
> +// through the ClientManagerService.  It is a snapshot of the state and
> +// is not live updated.  Right now, we don't actually providate any
> +// worker specific state values, but we may in the future.  This
> +// class also servces as a placeholder that the state is referring

services

@@ +73,5 @@
> +
> +  ClientWorkerState&
> +  operator=(const ClientWorkerState& aRight);
> +
> +  ClientWorkerState(ClientWorkerState&& aRight);

here as well?

@@ +102,5 @@
> +
> +  ClientState&
> +  operator=(const ClientState& aRight) = default;
> +
> +  ClientState(ClientState&& aRight);

here too.

@@ +109,5 @@
> +  operator=(ClientState&& aRight);
> +
> +  ~ClientState();
> +
> +  explicit ClientState(const IPCClientWindowState& aData);

I don't understand the order of the CTORs.

Comment 159

[Andrea Marchesini [:baku]]

I want to see how you use WorkerHolderToken before reviewing patch 1. I keep the r?.

Comment 160

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #156)
> ::: dom/workers/ServiceWorkerDescriptor.cpp
> @@ +65,5 @@
> > +ServiceWorkerDescriptor::operator==(const ServiceWorkerDescriptor& aRight) const
> > +{
> > +  return mData->id() == aRight.mData->id() &&
> > +         mData->scope() == aRight.mData->scope() &&
> > +         mData->principalInfo() == aRight.mData->principalInfo();
> 
> no mData->state()? This could be confusing.

I'll add a comment.  I don't include state() here since its not part of the identity of the SW.

Comment 161

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #160)
> (In reply to Andrea Marchesini [:baku] from comment #156)
> > ::: dom/workers/ServiceWorkerDescriptor.cpp
> > @@ +65,5 @@
> > > +ServiceWorkerDescriptor::operator==(const ServiceWorkerDescriptor& aRight) const
> > > +{
> > > +  return mData->id() == aRight.mData->id() &&
> > > +         mData->scope() == aRight.mData->scope() &&
> > > +         mData->principalInfo() == aRight.mData->principalInfo();
> > 
> > no mData->state()? This could be confusing.
> 
> I'll add a comment.  I don't include state() here since its not part of the
> identity of the SW.

Actually, this operation isn't really used, so I might as well make it a full comparison.

Comment 162

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a ServiceWorkerDescriptor type to represent a thread-safe snapshot of a ServiceWorkerInfo. r=baku

Comment 163

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ClientInfo and ClientState types. r=baku

Comment 164

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Remove nsIIPCBackgroundChildCreateCallback usage since its going away.

Comment 165

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Try to remove some more dead code after the PBackground changes.

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

Comment 166

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Use the ServiceWorkerDescriptor in existing WorkerPrivate and ServiceWorkerInfo code. r=baku

Fix a mis-typed logic condition.  I swapped (oldState == newState) for (oldState != newState).

Comment 167

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

The claim() code was using ServiceWorkerDescriptor::operator==().  I decided to just change this comparison to explicitly use Id() instead of burying it in the operator==() impl like before.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0db89b21e36dcea41f99ab2f9d1ed5dadd67741a

Comment 168

[Ben Kelly [:bkelly, not reviewing]]

I'm going to land some patches as I go to avoid bitrot and to allow more parallel work to happen.  So as things are r+'d here I may move them to a separate bug to land.  To start I'll file a bug to land ServiceWorkerDescriptor and a bug to land ClientInfo/ClientState.

Comment 169

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8920284 [details] [diff] [review]
P2 Expose a "parsed" ServiceWorkerState value. r=baku

Moved to bug 1412858.

Comment 170

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8921618 [details] [diff] [review]
P5 Add ClientInfo and ClientState types. r=baku

Moved to bug 1412864.

Comment 171

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ClientThing base class. r=baku

Comment 172

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 173

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add IPC actor structure and boilerplate. r=baku

Comment 174

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

I split the actor boilerplate out into P3.  It was harder than I expected.  Lets see what I broke:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30a8c54a65109bc66d2242de4d1a841a79a94fab

Comment 175

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ClientThing base class. r=baku

Comment 176

[Ben Kelly [:bkelly, not reviewing]]

Some new orange, but it doesn't look like its from my code splitting.  New test here:

https://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/test_bug1408734.html

Has the same problem as bug 1411746.

Comment 177

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add IPC actor structure and boilerplate. r=baku

Comment 178

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 179

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923581 [details] [diff] [review]
P2 Add ClientThing base class. r=baku

Andrea, the code in later patches is designed around the idea of an underlying IPC actor and an outer owning object.  This patch introduces a templated base class for the outer owning objects to extend.  It mainly manages the activation and shutdown steps between the owner and actor.  They each have a weak reference to each other that must be cleared when either one goes away.

I understand that you prefer to implement the owner and the actor in one class.  Personally I prefer to separate them because I find it easier to think about their life cycles separately.

In addition to preference, though, one of the Client owner objects cannot be unified easily with the actor.  The ClientSource is designed to be owned as a UniquePtr<> and is not reference counted.  This means I cannot use the trick of having the IPC code add a ref, etc.

In regards to naming, I could change this to something more verbose like ClientActorOwner if you want.  I kind of like ClientThing, though.

As a side note, this class used to also handle pending lambda operations for cases where the actor was not created yet.  That is no longer necessary since the PBackground actor is now synchronously available.  Thanks!

Comment 180

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923854 [details] [diff] [review]
P3 Add IPC actor structure and boilerplate. r=baku

This sizeable patch stubs out the various IPC actors.  There are some long living actors that form this structure:

* PBackground
  * PClientManager
    * PClientSource
    * PClientHandle

Each of these corresponds to an outer owning object.

* ClientManager is going to be a ref-counted per-thread singleton.  It will be a factory for creating ClientSource and ClientHandle objects.  It will also provide methods for performing operations to query Clients, navigate Clients, etc.
* ClientSource will be an RAII style object that each window/worker owns.  While it exists the global is considered alive.  It should be destroyed when the global is destroyed.  This is how we map globals to the Client concept.
* ClientHandle is a way to attach to an existing Client.  It is a ref-counted object.  When creating a ClientHandle the ClientManager will match up the provided ClientInfo to a corresponding ClientSource.  Messages like postMessage(), focus(), etc will then be forwarded to the correct ClientSource.  If the ClientSource goes away, then these operations will simply reject.  The ClientHandle can out live the ClientSource.

In addition to this basic structure, there are a number of "operation" type actors here.  I wrote all of this before the async RPC style ipdl support was added.  I considered going back and rewriting this stuff, but I opted not to for now.  The async RPC support requires slightly different MozPromise usage than I am using here.  It also has some subtle differences on life cycle management that I would have to hunt down.  Given that this bug is way behind I decided not to tackle that for now.

The operations are:

* PBackground
  * PClientManager
    * PClientManagerOp: various child-to-parent operations
    * PClientNavigateOp: A parent-to-child operation.
  * PClientSource
    * PClientSourceOp: various parent-to-child operations
  * PClientHandle
    * PClientHandleOp: various child-to-parent operations

* PContent
  * PClientOpenWindowOp: A parent-to-child operation for opening a new window cross-process with an async return that provides corresponding ClientInfo.

For the most part this patch is just boilerplate.  The actors and types don't have any useful information in them.  I thought getting this out of the way in a separate patch would make it easier to read later patches where I add functionality.

Comment 181

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add the ClientManager class. r=baku

Comment 182

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add the ClientManager class. r=baku

Comment 183

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

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

Comment 184

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a84ae890fbf4eb9f2b1c062c1a4808ce9723d9a

Comment 185

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ClientManagerService and reference it from parent actors. r=baku

Comment 186

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ClientManagerService and reference it from parent actors. r=baku

Comment 187

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 188

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923896 [details] [diff] [review]
P4 Add the ClientManager class. r=baku

Andrea, this patch adds the ClientManager class.  Its a per-thread singleton that acts as a factory for ClientSource and ClientHandle.  It also provides methods for querying the list of clients, etc.

There is a Startup() method that must be called early during process creation to setup the TLS constants.

There is a StartOp() utility method that creates a PClientManagerOp and ties it back to a MozPromise.  Future patches will make use of this method.

Again, this is just getting the structure in place.  Future patches will add the factory methods, etc.

Comment 189

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923962 [details] [diff] [review]
P5 Add ClientManagerService and reference it from parent actors. r=baku

This patch adds the parent-side service object that the majority of the actors connect to.  This will be the object that maintains the list of active globals/clients in the system.  When a ClientHandle is created it will be matched to a ClientSource in the service's list.

For now this patch just adds the service and adds a reference to the relevant actors.

This class lives on PBackground thread.

Comment 190

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Add the ClientSource class and hook it into ClientManager/ClientManagerService. r=baku

Comment 191

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

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

Comment 192

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923984 [details] [diff] [review]
P6 Add the ClientSource class and hook it into ClientManager/ClientManagerService. r=baku

Andrea, this patch adds the ClientSource object.  Its an RAII style object designed to be held via a UniquePtr<>.  I say "RAII" since creating it will register the existence of the Client in the ClientManagerService and destruction will remove the entry from ClientManagerService.

I went ahead an included the bits here that integrate ClientSource with ClientManager and ClientManagerService.

Creation starts when nsGlobalWindow, nsDocShell, or WorkerPrivate call one of the ClientManager::CreateSource() methods.  (These call sites will be added in a later patch.). The ClientManager then allocates the ClientSource which triggers the creation of the PClientSource actor.  The parent-side of the actor then calls AddSource on the service.

The service stores its list of Clients in a hash table keyed on the Client's UUID.  (The UUID is originally created in the ClientManager::CreateSourceInternal() method).  I chose this data structure because I wanted to make things as fast as possible for adding and removing Clients to minimize overhead from pages with many iframes, etc.  Looking up by UUID is also important so that ClientHandle can relatively quickly attach to the right source.  The main operation that will still be O(n) is the matchAll(), but that is ok because its O(n) today.

Note, I use the ContentPrincipalInfo.originNoSuffix() value to match principals off the main thread.  This is very handy as I was doing some evil things with the spec in previous versions.  Thanks for adding it!

Comment 193

[Andrea Marchesini [:baku]]

Comment on attachment 8923854 [details] [diff] [review]
P3 Add IPC actor structure and boilerplate. r=baku

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

::: dom/clients/manager/ClientManagerChild.cpp
@@ +76,5 @@
> +}
> +
> +mozilla::ipc::IPCResult
> +ClientManagerChild::RecvPClientNavigateOpConstructor(PClientNavigateOpChild* aActor,
> +                                   const ClientNavigateOpConstructorArgs& aArgs)

indentation here

Comment 194

[Andrea Marchesini [:baku]]

Comment on attachment 8920695 [details] [diff] [review]
P1 Add a ref-counted WorkerHolderToken() class. r=baku

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

::: dom/workers/WorkerHolderToken.cpp
@@ +14,5 @@
> +already_AddRefed<WorkerHolderToken>
> +WorkerHolderToken::Create(WorkerPrivate* aWorkerPrivate, Status aShutdownStatus,
> +                          Behavior aBehavior)
> +{
> +  MOZ_ASSERT(aWorkerPrivate);

Can you assert in which thread this code runs?

Comment 195

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a ref-counted WorkerHolderToken() class. r=baku

Comment 196

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add IPC actor structure and boilerplate. r=baku

Comment 197

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add ClientHandle class and make it attach to the correct ClientSource. r=baku

Comment 198

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 199

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 200

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8924306 [details] [diff] [review]
P7 Add ClientHandle class and make it attach to the correct ClientSource. r=baku

This adds the ClientHandle owner object.  It also adds the code that makes it attach to the ClientSource via the ClientManagerService.

Comment 201

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

While trying to split the next patch I realized I never implemented the principal/url validation code.  This patch adds it using MozURL for OMT support.  This should help harden the clients PBackground ipdl protocol against spoofed values (a bit).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2130ecd4975c30658a23d44ee4781d7318f486d2

Comment 202

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

Lots of orange.  Looks like file:/// urls were not passing validation.  Fix that and see if anything else is broken:

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

Comment 203

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

Allow javascript: URLs.

Also, add some validation that we don't overwrite an existing ClientSourceParent.  There were assertions for this, but we should use a runtime check since we are dealing with potentially spoofed IPC traffic.

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

Comment 204

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

Allow about:srcdoc.

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

Comment 205

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

Handle jar, moz-icon, and wyciwyg URLs.

Also, fix a bug in the WorkerPrivate integration where I was marking a worker execution ready even though the load failed due to CSP violation.

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

Comment 206

[Ben Kelly [:bkelly, not reviewing]]

I like this validate patch.  Its a bit brittle using MozURL right now, but I like that it caught this subtle bug in the WorkerPrivate code.  Also gives me more confidence things are working correctly across the system.

Comment 207

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

More file URL fixes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=686bb90a38ae381ecb972e0d9c5eab97a9251547

Comment 208

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ecee03bf2fc1f0da5b16e92fc23414afaf41be2

Comment 209

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

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

Comment 210

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0765c1d5603e86e1a7a9823bb96d026ab6a8858d

Comment 211

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=367cd20b80be07bd70d47c820865713c8bbc8199

Comment 212

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

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

Note, there are a couple weird failures I need to investigate that could indicate bugs.  For example:

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_redirects.js#158

And:

https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js

Comment 213

[Andrea Marchesini [:baku]]

Comment on attachment 8923896 [details] [diff] [review]
P4 Add the ClientManager class. r=baku

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

::: dom/clients/manager/ClientManager.cpp
@@ +93,5 @@
> +  return ref.forget();
> +}
> +
> +void
> +ClientManager::PBackgroundActorCreated(PBackgroundChild* aActor)

Can you consider moving this code in the CTOR directly?

@@ +99,5 @@
> +  NS_ASSERT_OWNINGTHREAD(ClientManager);
> +  MOZ_ASSERT(aActor);
> +
> +  if (IsShutdown()) {
> +    return;

This cannot happen. We are in CTOR.

Comment 214

[Andrea Marchesini [:baku]]

Comment on attachment 8923984 [details] [diff] [review]
P6 Add the ClientSource class and hook it into ClientManager/ClientManagerService. r=baku

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

::: dom/clients/manager/ClientManager.cpp
@@ +76,5 @@
> +
> +  if (IsShutdown()) {
> +    return nullptr;
> +  }
> +

nsID id;
rv = nsContentUtils::GenerateUUIDInPlace(id);
...

::: dom/clients/manager/ClientSource.h
@@ +9,5 @@
> +#include "mozilla/dom/ClientInfo.h"
> +#include "mozilla/dom/ClientThing.h"
> +
> +#ifdef XP_WIN
> +#undef PostMessage

No PostMessage in this method.

Comment 215

[Andrea Marchesini [:baku]]

Comment on attachment 8924306 [details] [diff] [review]
P7 Add ClientHandle class and make it attach to the correct ClientSource. r=baku

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

::: dom/clients/manager/ClientManager.cpp
@@ +105,5 @@
> +{
> +  NS_ASSERT_OWNINGTHREAD(ClientManager);
> +  MOZ_DIAGNOSTIC_ASSERT(aSerialEventTarget);
> +
> +  if (IsShutdown()) {

Can it be that we are shutting down?

::: dom/clients/manager/ClientSourceParent.cpp
@@ +31,5 @@
>    mService->RemoveSource(this);
> +
> +  nsTArray<ClientHandleParent*> handleList;
> +  mHandleList.SwapElements(handleList);
> +  for (ClientHandleParent* handle : handleList) {

Does this support the removing of elements when looping?
Can it happen that Send__delete__ triggers ClientHandleParent::ActorDestroy directly?

@@ +79,5 @@
> +
> +void
> +ClientSourceParent::DetachHandle(ClientHandleParent* aClientHandle)
> +{
> +  MOZ_DIAGNOSTIC_ASSERT(aClientHandle);

Do you want to assert mHandleList.Containers(aClientHandle) ? If not, why can DetachHandle called more than once?

Comment 216

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #215)
> ::: dom/clients/manager/ClientManager.cpp
> @@ +105,5 @@
> > +{
> > +  NS_ASSERT_OWNINGTHREAD(ClientManager);
> > +  MOZ_DIAGNOSTIC_ASSERT(aSerialEventTarget);
> > +
> > +  if (IsShutdown()) {
> 
> Can it be that we are shutting down?

Yes, if some code tries to use the ClientManager to create a handle at the same time we are shutting things down.  In that case the ClientManager actor may have been destroyed already.  (Although I see perhaps we should be setting the shutdown flag there and we are not.)

Comment 217

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #215)
> ::: dom/clients/manager/ClientSourceParent.cpp
> @@ +31,5 @@
> >    mService->RemoveSource(this);
> > +
> > +  nsTArray<ClientHandleParent*> handleList;
> > +  mHandleList.SwapElements(handleList);
> > +  for (ClientHandleParent* handle : handleList) {
> 
> Does this support the removing of elements when looping?
> Can it happen that Send__delete__ triggers ClientHandleParent::ActorDestroy
> directly?
> 
> @@ +79,5 @@
> > +
> > +void
> > +ClientSourceParent::DetachHandle(ClientHandleParent* aClientHandle)
> > +{
> > +  MOZ_DIAGNOSTIC_ASSERT(aClientHandle);
> 
> Do you want to assert mHandleList.Containers(aClientHandle) ? If not, why
> can DetachHandle called more than once?

I think I can fix both of these issues by looping on a copy instead of using SwapElements().  We can only detach once, but SwapElements() is removing the handle before detach is called.

Comment 218

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add the ClientManager class. r=baku

Comment 219

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ClientManagerService and reference it from parent actors. r=baku

Comment 220

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Add the ClientSource class and hook it into ClientManager/ClientManagerService. r=baku

Comment 221

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add ClientHandle class and make it attach to the correct ClientSource. r=baku

Comment 222

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

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

Comment 223

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

I had to fix a couple things in the wip patch, but the reviewed patches are good.  I'm going to land in a separate bug.

Comment 224

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

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

Comment 225

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 226

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c0660d6d2468dffa24d2e7b9ae5afa7eda7f4bc

Comment 227

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=179e8efb8dd352f0e04834d9d7a5e750e5a767ab

Comment 228

[Ben Kelly [:bkelly, not reviewing]]

Attachment: validate.patch

Lets simplify all the special cases and just test the main path.   As MozURL improves this will begin covering more cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29cc3887b37ee689416b182d7ee06c8d00b0cc04

Comment 229

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Validate ClientSource creation path across IPC boundary. r=baku

Comment 230

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b806f003d6a63104f639b31a2e6efe5453691eb

Comment 231

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a way for the window/worker/docshell to mark a ClientSource execution ready. r=baku

Comment 232

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89f5f35184efc6dcff9481ba3b6f88957473776c

Comment 233

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8927539 [details] [diff] [review]
P1 Validate ClientSource creation path across IPC boundary. r=baku

Andrea, this patch adds some validation to the ClientSource creation path.  Specifically it:

1) Validates the PrincipalInfo used to create the source.  I want to avoid main thread bounces, so I am using MozURL here.  This doesn't handle every possible internal URL (like nested view-source:, etc) but it does allow us to validate all the http:/https:/blob: URLs content might see in the Clients API.

Ultimately improvements in MozURL and OMT principal support should let us handle all principals.  This will be desirable so that we can use this infrastructure for other features besides Clients API.

This validation is done on both the child side and the parent side.  If a PrincipalInfo fails on the child side we just avoid creating the IPC actor and leave the ClientSource in its shutdown space.  Basically the Client just won't show up in the shared list.  If validation fails on the parent side we assume someone is spoofing the IPC connection and MOZ_CRASH.

2) This patch also validates that we don't try to add more than one ClientSource with the same UUID.  If this happens we destroy the actor for the second duplicate UUID and leave the second duplicate ClientSource in its shutdown disconnected state.

Comment 234

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8927634 [details] [diff] [review]
P2 Add a way for the window/worker/docshell to mark a ClientSource execution ready. r=baku

This page adds the concept of setting a Client to the "Execution Ready" state.

For windows this corresponds to step 4 here:

https://html.spec.whatwg.org/#set-the-active-document

And for workers this corresponds to step 22 here:

https://html.spec.whatwg.org/#worker-processing-model

Basically the document or worker script is loaded and valid.  We now have a creation URL:

https://html.spec.whatwg.org/#concept-environment-creation-url

Up until this point a Client's URL is the empty string.  We are also able to set the frame type of the Client at this stage.

This patch adds three different ways to mark a ClientSource execution ready:

1. A WorkerPrivate can mark its ClientSource execution ready.
2. An inner window can mark its ClientSource execution ready.
3. A docshell also own and mark a ClientSource execution ready.  This represents the initial about:blank window for cases where we have optimized away the early creation of the about:blank document.  In this case we need to still treat the initial about:blank as existing even if we haven't actually created the nsDocument and inner window yet.  For this reason we allow the docshell to handle this "initial" client.

Ultimately each path will result in a creation URL and frame type being set on the ClientSource's ClientInfo.  A message is then propagated to the parent side and any attached Handle objects.

We validate the creation URL against the ClientSource PrincipalInfo.  Just like with the principal validation in the last patch we validate both on the child and parent sides.  If the child side fails we just teardown the IPC actor leaving it disconnected.  If the parent side fails we treat it as a fatal error and MOZ_CRASH.

Nothing calls the methods to set execution ready yet.  I need to add a few more bits in other places before I can integrate ClientSource and this method into nsGlobalWindow/nsDocShell/WorkerPrivate.

Comment 235

[Andrea Marchesini [:baku]]

Comment on attachment 8927539 [details] [diff] [review]
P1 Validate ClientSource creation path across IPC boundary. r=baku

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

::: dom/clients/manager/ClientManagerParent.cpp
@@ +105,5 @@
> +ClientManagerParent::RecvPClientSourceConstructor(PClientSourceParent* aActor,
> +                                                  const ClientSourceConstructorArgs& aArgs)
> +{
> +  ClientSourceParent* actor = static_cast<ClientSourceParent*>(aActor);
> +  actor->Init();

return actor->Init() ? IPC_OK() : IPC_FAILURE(..)

::: dom/clients/manager/ClientSourceParent.cpp
@@ +72,5 @@
> +  // Ensure the principal is reasonable before adding ourself to the service.
> +  // Since we validate the principal on the child side as well, any failure
> +  // here is treated as fatal.
> +  if (NS_WARN_IF(!ClientIsValidPrincipalInfo(mClientInfo.PrincipalInfo()))) {
> +    MOZ_CRASH("ClientSourceParent::Init() got invalid principal");

This is wrong. we don't want the parent to crash. We should maybe return a boolean and kill the content process.

@@ +79,5 @@
> +
> +  // Its possible for AddSource() to fail if there is already an entry for
> +  // our UUID.  This should not normally happen, but could if someone is
> +  // spoofing IPC messages.
> +  if (NS_WARN_IF(!mService->AddSource(this))) {

same here.

::: dom/clients/manager/ClientValidation.h
@@ +8,5 @@
> +
> +namespace mozilla {
> +
> +namespace ipc {
> +  class PrincipalInfo;

why this indentation? Usually we don't do it.

Comment 236

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Validate ClientSource creation path across IPC boundary. r=baku

Updated patch that uses ContentParent->KillHard() as we discussed in IRC.  I hacked a failure in the parent manually so I could test this locally.

Note, I also decided to perform the validation in ClientSourceParent even when e10s is disabled.  I'd rather keep the e10s vs non-e10s paths as similar as possible instead of optimizing for a mode that will eventually go away.

Comment 237

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a way for the window/worker/docshell to mark a ClientSource execution ready. r=baku

Update the execution ready patch to use the validation failure mechanism introduced in the new P1 patch.

Comment 238

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a2e0411bd8679287e1db7a01b9768e156d94b4

Comment 239

[Andrea Marchesini [:baku]]

Comment on attachment 8927896 [details] [diff] [review]
P1 Validate ClientSource creation path across IPC boundary. r=baku

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

::: dom/clients/manager/ClientManagerService.cpp
@@ +91,5 @@
>    auto entry = mSourceTable.LookupForAdd(aSource->Info().Id());
> +  // Do not permit overwriting an existing ClientSource with the same
> +  // UUID.  This would allow a spoofed ClientParentSource actor to
> +  // intercept postMessage() intended for the real actor.
> +  if (entry) {

I would use WARN_IF

@@ +104,5 @@
>  {
>    AssertIsOnBackgroundThread();
>    MOZ_ASSERT(aSource);
>    auto entry = mSourceTable.Lookup(aSource->Info().Id());
> +  if (!entry) {

here as well

Comment 240

[Andrea Marchesini [:baku]]

Comment on attachment 8927897 [details] [diff] [review]
P2 Add a way for the window/worker/docshell to mark a ClientSource execution ready. r=baku

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

::: dom/clients/manager/ClientSource.cpp
@@ +159,5 @@
> +  if (NS_WARN_IF(!doc)) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  nsCString spec;

nsAutoCString?

Comment 241

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #240)
> ::: dom/clients/manager/ClientSource.cpp
> @@ +159,5 @@
> > +  if (NS_WARN_IF(!doc)) {
> > +    return NS_ERROR_UNEXPECTED;
> > +  }
> > +
> > +  nsCString spec;
> 
> nsAutoCString?

In this case the spec string is going to be passed over IPC which required nsCString.  Using nsAutoCString would require an extra copy in this case.  I can add a comment.

Comment 242

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Validate ClientSource creation path across IPC boundary. r=baku

Note, I had to do `NS_WARN_IF(!!entry)` to get it to coerce correctly.

Comment 243

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a way for the window/worker/docshell to mark a ClientSource execution ready. r=baku

Comment 244

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 245

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Remove some dead code.

Comment 246

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow the reserved/initial/source client data and service worker to be marked on LoadInfo. r=baku r=valentin

Comment 247

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

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

Comment 248

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928309 [details] [diff] [review]
P1 Allow the reserved/initial/source client data and service worker to be marked on LoadInfo. r=baku r=valentin

Valentin, Andrea, this patch adds some additional fields to the LoadInfo object.  These are mainly things that are being "passed through" the channel to communicate state about:

1. The global that created the channel
2. The global that will be created as a result of a non-subresource channel
3. Any service worker controlling the channel via FetchEvent interception

The globals here are represented by either a simple ClientInfo snapshot structure:

https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientInfo.h

Or possibly a live ClientSource object backed by an IPC actor:

https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientSource.h

In the future I would like to eventually migrate some other data that is on LoadInfo into the ClientInfo.  For example, its possible we can replace triggering principal with the ClientInfo principal.  Things like CSP and referral policy could also move to the Client in the future.

The advantage of doing this is that we can produce a ClientInfo equally well from both windows and workers.  So it would break the weird thing where most stuff works with nsIDocument available, but probably breaks on worker threads without an nsIDocument.

I tried to write good comments, but to summarize some of the terms used:

- "reserved client" represents a window or worker global which is not active yet, but will be created as a result of the non-subresource nsIChannel.
- "initial client" represents a window global that will be used after the nsIChannel completes, but its already active because its an initial about:blank document.  This is a legacy feature of the web that we unfortunately have to deal with.
- The simple load info "client" refers to the window or worker global that initiated nsIChannel.  I suppose we could rename this to (Get|Set)TriggeringClientInfo() if you think that's clearer.  The service worker spec just calls it FetchEvent.clientId, which is why I used the simpler name here.

Note, there is no code landed yet that will populate or consume these values.  That will come in my next few patches:

a. A ClientChannelHelper class that will handle the special redirect logic needed for non-subresource loads.
b. WorkerPrivate and ScriptLoader code to set the reserved ClientInfo and to add the ClientChannelHelper for its main script load.
c. nsDocShell code that will set its various types of clients and add the ClientChannelHelper for document loads.

Comment 249

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Track the client window or worker's event target on ClientSource. r=baku

Comment 250

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a helper to handle Client objects properly on nsIChannels. r=baku

Comment 251

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 252

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a helper to handle Client objects properly on nsIChannels. r=baku

Comment 253

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928615 [details] [diff] [review]
P2 Track the client window or worker's event target on ClientSource. r=baku

This patch captures the window or worker's nsIEventTarget and stores it on the ClientSource.  We will need this to do execute async operations on the ClientSource correctly.

Comment 254

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928647 [details] [diff] [review]
P3 Add a helper to handle Client objects properly on nsIChannels. r=baku

This provides a helper for setting up client information on a non-subresource nsIChannel.  We have to be very careful that client UUIDs are not allowed to cross origins since ultimately they will be used to allow postMessage() later.  This helper checks for cross-origin redirects and creates a new reserved ClientSource if necessary.

Note, this code only supports creating Window ClientSource objects.  This is for a couple reasons:

1. Worker ClientSource objects are owned by the worker thread and cannot be created on the main thread.
2. Worker scripts are required to be same-origin and other security checks block cross-origin redirects for worker non-subresource nsIChannels.

Comment 255

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Create a ClientSource for the WorkerPrivate and mark it execution ready after the script loads. r=baku

Comment 256

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a31b36937268f822e4247f1b18cdb997766e0b4

Comment 257

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928778 [details] [diff] [review]
P4 Create a ClientSource for the WorkerPrivate and mark it execution ready after the script loads. r=baku

Andrea, this patch does the following:

1. Makes WorkerPrivate create a ClientSource before performing its script load.
2. Passes the ClientInfo for that ClientSource on its main script load as the reserved client.
3. Adds the ClientChannelHelper to the script load to maintain client information properly on same-origin redirects.
4. Sets the WorkerPrivate ClientSource execution ready after the script loads successfully.

Note, this is the first patch that will cause ClientSource objects to be actively created and registered in the service.  I considered waiting to do this at the end, but I have a couple reasons to do it now:

1. Landing this integration lets me unblock your worker refactor patches sitting in my review queue.
2. It gets some of this code covered by our test suite.  I have already had one bug filed about no code coverage in dom/clients in the tree.
3. Its better to slowly introduce the platform changes over time to catch any perf issues early.

Comment 258

[Ben Kelly [:bkelly, not reviewing]]

Oh, I forgot to mention, the patch in comment 257 also adds a new ClientType for service worker globals.  I included the spec issue.  I haven't heard any major objections from the other spec participants.  I want to add this so we are consistent in creating ClientSource for all globals and labeling them with an appropriate type.

Comment 259

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8928309 [details] [diff] [review]
P1 Allow the reserved/initial/source client data and service worker to be marked on LoadInfo. r=baku r=valentin

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

Looks good.

::: netwerk/base/nsILoadInfo.idl
@@ +851,5 @@
> +   * Note which client (i.e. global) initiated this network request.  All
> +   * nsGlobalWindow and WorkerPrivate can be converted to a ClientInfo to
> +   * be set here.  While this is being added to support service worker
> +   * FetchEvent, it can also be used to communicate other information about
> +   * the source global context in the future.

The comment is OK, but I suspect it will become obsolete if we start adding other uses; and will probably forget to update it :)

Comment 260

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Valentin Gosu [:valentin] from comment #259)
> The comment is OK, but I suspect it will become obsolete if we start adding
> other uses; and will probably forget to update it :)

I'd like to leave it for now.  I think I will be involved with any future build out of this feature and will make a note to remove it if necessary.

Comment 261

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Track the client window or worker's event target on ClientSource. r=baku

Rebase.

Comment 262

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebase around nsGlobalWindowInner changes.

Comment 263

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Handle creating and activating the ClientSource in nsDocShell and nsGlobalWindow. r=baku r=freesamael

Comment 264

[Ben Kelly [:bkelly, not reviewing]]

Try just up to P5, but without any of the remaining patches.  I want to double check this is safe by itself.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=339c19d829f74244a838b90224da4119514ec3f1

Comment 265

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Handle creating and activating the ClientSource in nsDocShell and nsGlobalWindow. r=baku r=freesamael

https://treeherder.mozilla.org/#/jobs?repo=try&revision=804a94e34120644b0fff10c72f3ec3104d1b88b6

Comment 266

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

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

Comment 267

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8929199 [details] [diff] [review]
P5 Handle creating and activating the ClientSource in nsDocShell and nsGlobalWindow. r=baku r=freesamael

Andrea, Samael,

This patch integrated ClientSource and ClientInfo into the main thread window globals.  Samael, Olli suggested you would be a good person to double-check the docshell parts of this.

Samael, let me try to provide some context for your benefit since this bug is very long.  The overall goal here is to track all the globals, both workers and windows, across all the processes in the browsers.  This is to both accurately implement the Clients DOM API, but also to provide a basis for other internal features that might need this information.

To that end I've introduced a ClientSource object that the window and worker global own.  This is a live actor object that auto-registers itself in a parent side service table.  When the ClientSource is destroyed it removes itself from this table.  Due to being an IPC actor a ClientSource is limited to the thread it was created on.  To support cross-thread operations there is also an inert structure called ClientInfo that can be used to track back to the ClientSource it represents.

With that context out of the way, let me try to describe what this patch does.

The overall goal is to create the ClientSource *before* the nsIChannel load for the document and then pass ownership to the final nsGlobalWindowInner that results from that nsIChannel.  We need the ClientSource before the nsIChannel load in order to properly populate things spec'd on the service worker FetchEvent.

There are a few ways this will happen depending on the type of load.  Lets consider a normal top level window load:

1. docshell creates the navigation nsIChannel.
2. docshell creates a "initial" ClientSource to represent the initial about:blank document in the window.  The spec requires this to be created even if we optimize away the creation of the about:blank document/window.
3. docshell adds the ClientChannelHelper to the nsIChannel.  This will set the initial ClientInfo on the nsIChannel LoadInfo, transfer it during same-origin redirects, and create a new ClientSource for cross-origin redirects.
4. After the nsIChannel completes the nsGlobalWindowInner will try to take any ClientSource created on the LoadInfo or the initial ClientSource from the docshell.
5. The nsGlobalWindowOuter will then mark the inner window and its ClientSource "execution ready" with the final load URL.

(I feel like there may be some cases where we don't want to call MaybeCreateInitialClientSource(), but right not it seems like we should always call it per the spec.  I filed a spec issue to clarify this:

https://github.com/w3c/ServiceWorker/issues/1228

For now I'd like to proceed with this patch, though.)

An alternative case to the steps above is when we actually create the inital about:blank document/window because something called GetDocument().  Then we do this:

1. Create the initial ClientSource in CreateAboutBlankContentViewer.
2. When CreateAboutBlankContentViewer create the nsGlobalWindowInner for the about:blank document it steals the initial ClientSource.
3. Eventually the docshell creates the navigation nsIChannel
4. docshell adds ClientChannelHelper
5. Resulting nsGlobalWindoInner either has the existing initial about:blank or the reserved ClientSource from LoadInfo in a cross-origin redirect occurred.  (It should be a different nsGlobalWindowInner as well.)
6. nsGLobalWindowOuter marks the final inner window and its ClientSource "execution ready".

There are some cases where I punted on creating the initial about:blank ClientSource.  Mainly these were cases where docshell does not easily know the principal before the load.  These are corner cases and can be added later.  Its safer not to get the initial about:blank source created then it would be to use the wrong principal.

Comment 268

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #267)
> Comment on attachment 8929199 [details] [diff] [review]
> P5 Handle creating and activating the ClientSource in nsDocShell and
> nsGlobalWindow. r=baku r=freesamael
> (I feel like there may be some cases where we don't want to call
> MaybeCreateInitialClientSource(), but right not it seems like we should
> always call it per the spec.  I filed a spec issue to clarify this:
> 
> https://github.com/w3c/ServiceWorker/issues/1228
> 
> For now I'd like to proceed with this patch, though.)

I got feedback on my spec issue already.  In theory the initial about:blank ClientSource should only inherit the principal from the parent or opener.  For top level navigations it should get an opaque origin which makes it non-observable.

This is actually what the code does here because in DoChannelLoad() I don't pass an explicit principal to MaybeCreateInitialClientSource().  So we try to inherit the principal and simply avoid creating the ClientSource if we can't find one.

So I believe the code in the patch up for review is correct.  Sorry for my confusion.  I find all the cases here kind of complex and hard to keep straight at times.

I'll take an action to add a comment describing all of this in DoChannelLoad().  Clearly I'm going to forget how all this works again in a couple weeks...

Comment 269

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Allow ClientSource objects to be frozen while in bfcache. r=baku

Comment 270

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8929636 [details] [diff] [review]
P6 Allow ClientSource objects to be frozen while in bfcache. r=baku

Andrea, this patch lets us mark ClientSource objects as frozen while their owning window/worker is frozen in the bfcache.  We keep the ClientSource in place while in the bfcache so things like the Client ID don't change, etc.

Note, there is an open action item for me to evict window/worker objects from the bfcache if their absence is observed via the clients API.  I had originally planned to do that here, but I punted it to bug 1418489.

Comment 271

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 272

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 273

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Set ClientInfo on the LoadInfo for document owned network requests. r=baku

Andrea, this patch makes us set the source or triggering ClientInfo on the LoadInfo for all network requests owned by a document.  Doing this properly for workers will come later.  We don't currently report FetchEvent.clientId in today's code either.

Comment 274

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 275

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Allow a caller to set a ClientSource controlled by a service worker using a ClientHandle. r=baku

Comment 276

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8929199 [details] [diff] [review]
P5 Handle creating and activating the ClientSource in nsDocShell and nsGlobalWindow. r=baku r=freesamael

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

Thanks for the very detailed explanation, Ben! Good for me to understand a bit about how service worker works in e10s-multi. The patch looks good and well-documented. Thanks!

Comment 277

[Andrea Marchesini [:baku]]

Comment on attachment 8928647 [details] [diff] [review]
P3 Add a helper to handle Client objects properly on nsIChannels. r=baku

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

::: dom/clients/manager/ClientChannelHelper.cpp
@@ +31,5 @@
> +{
> +  nsCOMPtr<nsIInterfaceRequestor> mOuter;
> +  nsCOMPtr<nsISerialEventTarget> mEventTarget;
> +
> +  ~ClientChannelHelper()

= default;

@@ +36,5 @@
> +  {
> +  }
> +
> +  NS_IMETHOD
> +  GetInterface(const nsIID & aIID, void **aResultOut) override

& and ** should be near the type.

@@ +52,5 @@
> +    return NS_ERROR_NO_INTERFACE;
> +  }
> +
> +  NS_IMETHOD
> +  AsyncOnChannelRedirect(nsIChannel *aOldChannel,

* next to the type. here and everywhere in this file.

@@ +90,5 @@
> +          oldLoadInfo->GetReservedClientInfo();
> +        if (reservedClientInfo.isSome()) {
> +          newLoadInfo->SetReservedClientInfo(reservedClientInfo.ref());
> +        } else {
> +          const Maybe<ClientInfo>& initialClientInfo =

why this if/else? It should work also if you do:

const Maybe<ClientInfo>& reservedClientInfo = oldLoadInfo->GetReservedClientInfo();
if (reservedClientInfo.isSome()) {
  newLoadInfo->SetReservedClientInfo(reservedClientInfo.ref());
}

const Maybe<ClientInfo>& initialClientInfo = ...
if (initialClientInfo.isSome()) {
  newLoadInfo->Set...
}

@@ +109,5 @@
> +        }
> +      }
> +    }
> +
> +    // If its a cross-origin redirect then we discard the old reserved client

it's ?

@@ +175,5 @@
> +  nsresult rv = ssm->GetChannelResultPrincipal(aChannel, getter_AddRefs(channelPrincipal));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  Maybe<ClientInfo> initialClientInfo(Move(aInitialClientInfo));
> +  Maybe<ClientInfo> reservedClientInfo(Move(aReservedClientInfo));

I would move these at line 167 before any other operation, but I don't think we leak data if we keep them here.

Comment 278

[Andrea Marchesini [:baku]]

Comment on attachment 8928778 [details] [diff] [review]
P4 Create a ClientSource for the WorkerPrivate and mark it execution ready after the script loads. r=baku

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

::: dom/workers/ScriptLoader.cpp
@@ +1979,5 @@
>        }
>        return true;
>      }
>  
> +    // If this is a top level scropt that succeeded, then mark the

script

Comment 279

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 280

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Allow a caller to set a ClientSource controlled by a service worker using a ClientHandle. r=baku

Comment 281

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 282

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Allow a caller to set a ClientSource controlled by a service worker using a ClientHandle. r=baku

Andrea, this patch adds the ability to call ClientHandle::Control(swd) to mark a ClientSource object controlled by a service worker.  This also marks the controller in the ClientSourceParent which will enable the service to distinguish controlled vs non-controlled clients.

The patch also lets code local to the ClientSource directly mark it controlled via a SetController() method.  This is an optimization and timing fix to handle some cases where we know at ClientSource process that we're controlled before the async ClientHandle operation completes.

Nothing calls this yet, but it will be used in this way:

1. The main use will be for ServiceWorkerManager to call ClientHandle::Control() from the parent process.  For example, both from when the SWM intercepts the initial non-subresource network load and for things like Clients.claim().

2. In the case of the initial non-subresource network load the ClientSource::SetController() will also be called directly after the channel completes.  This is necessary to properly support synchronous access to navigator.serviceWorker.controller during first script evaluation.  If the async ClientHandle::Control() operation finishes first, then this is a no-op.

Future patches in this bug will make ClientSource::SetController() fire a controllerchange event.

Comment 283

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 284

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a helper to handle Client objects properly on nsIChannels. r=baku

Comment 285

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Create a ClientSource for the WorkerPrivate and mark it execution ready after the script loads. r=baku

Comment 286

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku

Comment 287

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 288

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku

Comment 289

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8930774 [details] [diff] [review]
P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku

Andrea, this patch makes the code actually use the ClientHandle::Control() and ClientSource::SetController() methods to mark the client as controlled.

There are a few cases that are handled here:

1. The docshell handles setting the initial about:blank client as controlled if its parent is controlled.  This is something we have agreed on in the spec, but we have not implemented before.

2. The SWM marks the reserved/initial client and load info as controlled in DispatchFetchEvent.

3. The SWM also marks the client as controlled in StartControllingADocument().  This method will disappear in the future when we refactor SWM to live in the parent, but I do it here for now for parity with legacy code.

4. The inner window marks its own client as controlled if the load info was marked controlled by the SWM.

5. The worker ScriptLoader and WorkerPrivate mark its ClientSource controlled if the load info is marked controlled by the SWM.

Note, this patch does not yet handle the Clients.claim() case.  That requires some additional plumbing for now since we have to notify the child-side SWM instances in the process with the client.  I will break that out as a separate patch.

Also note, there is a sync IPC call here.  This is needed for worker startup.  Consider that we basically need this ordering:

1. WorkerPrivate creates a ClientSource on worker thread
2. ScriptLoader calls ClientHandle::Control on main thread
3. The ClientSource from (1) is created in the parent and attaches to ClientManagerService.
4. The control message from (2) reaches the parent and finds the ClientSourceParent from (3).

Of course, this ordering is not enforced in any way by IPC.  We can totally get (4) happening before (3).

As a simple solution I introduced a "sync ping" message that is only used for dedicated and shared workers.  This is called between (1) and (2) above.  It ensures that (3) always takes place before (4).  It is a small amount of overhead, but it does not touch any main thread so should be relatively cheap.

There are of course other solutions, but they all seemed more complex to me.  We can investigate replacing this in a follow-up if you wish.

Comment 290

[Ben Kelly [:bkelly, not reviewing]]

Lets checkpoint the wip patch again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e578b156cfe246d1b93e45c8798b0267e46b97e

Also, the wip patch is less than 200kb now.  Hooray!

Comment 291

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97d994c940a4235156c547f1dfc6c009f28cddfa

Comment 292

[Ben Kelly [:bkelly, not reviewing]]

Try for just P8 and P9:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fbbf540760b69b53841229a9eb9e43b51c3b87a

Comment 293

[Ben Kelly [:bkelly, not reviewing]]

Tweak P8 to fix a warning failure.  Just keeping this locally for now to avoid churning the review flags.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=621ba239fd26c6388c8fd08067bc5f30e20754d3

Comment 294

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

I update my initial about:blank WPT test case based on review comments from JakeA.  I also added a test case to verify we do something sane for bug 1404468.

I think I'm going to close bug 1404468 and make the change to not reuse the inner window here.  I'm not sure its really something that can wait for a later bug.

The P9 patch should be safe to land, though, since nothing in the system is consuming the controller state from ClientSource yet.  Those changes are still in this mega-patch.

Comment 295

[Ben Kelly [:bkelly, not reviewing]]

Err, I meant bug 1419620 in comment 294.

Comment 297

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Rebase.

Also, I merged the about-blank-replacement.https.html WPT test in this patch upstream.  I'll remove from the wip patch when it next gets sync'd to m-c.

Comment 298

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf904809dffbce57604bad09b426c36261776ec

Comment 299

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

The WPT test merged from upstream so the wip patch shrinks ~10kb.

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

Comment 300

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Copy the service worker controller across redirects by default and clear it explicitly for non-subresource redirects. r=baku

Comment 301

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Handle the case where an iframe starts with an inherited controller and then ends up uncontrolled. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82dcbe8c7a6cfad38800a01eaf73a4c40350fe34

Comment 302

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8933789 [details] [diff] [review]
P10 Copy the service worker controller across redirects by default and clear it explicitly for non-subresource redirects. r=baku

Andrea, this patch reverses some of the LoadInfo controller logic.  Before we required the controller to be explicitly copied across redirects.  Now with this patch we will default to copying the controller and non-subresource channels will clear the controller if necessary in ClientChannelHelper.

I realized the old logic was flawed because we will need to keep the controller in place once we start using it on subresources channels.  These channels won't have ClientChannelHelper to explicitly move the controller around.

Comment 303

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Add a ClientManager::GetInfoAndState() that retrieves the ClientInfo and ClientState for a given ID. r=baku

Comment 304

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 305

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8933790 [details] [diff] [review]
P11 Handle the case where an iframe starts with an inherited controller and then ends up uncontrolled. r=baku

Andrea, this implements a short term fix for the case where:

1. An initial about:blank iframe inherits a service worker controller from its parent.
2. And its final document load is not actually covered by any service worker scope.

In this case we need to have a controlled initial about:blank and an uncontrolled final window.

How we make that happen is a bit up in the air right now.  There is a spec issue here:

https://github.com/w3c/ServiceWorker/issues/1232

For now I have done the smallest change possible to get the basic sanity requirements above.  I simply mint a new ClientSource if we hit this situation.  In the long term we probably want to clear the controller on the ClientSource or force docshell to create a separate inner window (like if it was cross-origin load, etc).  Those are more work though and its unclear which way the spec issue will go.

Comment 306

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8933849 [details] [diff] [review]
P12 Add a ClientManager::GetInfoAndState() that retrieves the ClientInfo and ClientState for a given ID. r=baku

This patch adds a ClientManager::GetInfoAndState() static method that will query the service for a given ID, snapshot the found client's state, and return the values via promise.

This is basically what the `Clients.get(id)` DOM API does.

We don't need or want a full handle here.  Just the static information necessary to return the Client object.  If any methods like `Client.focus()`, etc, are called then we will mint a ClientHandle at that point.

This patch is similar to the previous ClientHandle::Control() patch.  It pipes the operation through to the source.  In this case we also pass back the resulting ClientInfoAndState object.

Comment 307

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 308

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Fix race in about-blank-replacement.https.html popup test case. r=baku

When I went through review on the about-blank-replacement.https.html test case upstream I added some extra cleanup to call popup.close().  This was a bit of a mistake, though, since that cleanup code races with the code that actually tries to inspect the state of the popup window.

To fix this lets wait to cleanup until we unload the popup window's opener frame.

This fixes the test on non-e10s with this patch queue applied.

Comment 309

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Remove some stale code.

Comment 310

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P14 Implement ClientManager::MatchAll() to support clients.matchAll() WebAPI. r=baku

Comment 311

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 312

[Ben Kelly [:bkelly, not reviewing]]

Try based on bug 1422983:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b7383c9149217f1b2bec599cf2e9c2fcd0f93d8

Comment 313

[Andrea Marchesini [:baku]]

Comment on attachment 8930560 [details] [diff] [review]
P8 Allow a caller to set a ClientSource controlled by a service worker using a ClientHandle. r=baku

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

::: dom/clients/manager/ClientSourceOpChild.cpp
@@ +43,5 @@
> +    // This may cause the ClientSource object to be destroyed.  Do not
> +    // use the source variable after this call.
> +    promise = (source->*aMethod)(aArgs);
> +  }
> +

promise could be null at this point.

Comment 314

[Andrea Marchesini [:baku]]

Comment on attachment 8930774 [details] [diff] [review]
P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku

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

::: ipc/ipdl/sync-messages.ini
@@ +1059,5 @@
>  description =
>  [PLayerTransaction::ShutdownSync]
>  description = bug 1363126
> +[PClientSource::WorkerSyncPing]
> +description = Synchronous ping allowing worker thread to confirm actor is created. Necessary to avoid racing with ClientHandle actors on main thread.

you need a r+ from a IPC peer.

Comment 315

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8930774 [details] [diff] [review]
P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku

Jed, can you review this sync IPC message I'm adding?

The idea here is basically to guarantee that the content Worker thread actor is created on the parent side before we do some work on the main thread that will expect it to be there.  The sync message is a ping that just enforces that the actor is alive before we proceed to the main thread work.

The sync message is only used on PBackground off the main thread.  There are assertions to enforce this.

Comment 316

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #315)
> Jed, can you review this sync IPC message I'm adding?

Note, I think I'm also an IPC peer now, but I'm not in the hg commit hook yet.  Filing a separate bug to fix that.

Comment 317

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #313)
> ::: dom/clients/manager/ClientSourceOpChild.cpp
> @@ +43,5 @@
> > +    // This may cause the ClientSource object to be destroyed.  Do not
> > +    // use the source variable after this call.
> > +    promise = (source->*aMethod)(aArgs);
> > +  }
> > +
> 
> promise could be null at this point.

I don't think it can, actually.  The source object may be gone, but we hold a strong ref to the promise.

Also, all of the methods we call are required to return a promise.  If they hit an error they will reject the promise instead of returning nullptr.

I can add a MOZ_DIAGNOSTIC_ERROR and a comment to this point.

Comment 318

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Allow a caller to set a ClientSource controlled by a service worker using a ClientHandle. r=baku

Comment 319

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8934286 [details] [diff] [review]
P14 Implement ClientManager::MatchAll() to support clients.matchAll() WebAPI. r=baku

Andrea, this patch implements a query method to support the Clients.matchAll() WebAPI.  The query can be initiated using the ClientManager::MatchAll() method.

Note, I have a goal to support the Clients API in windows and other workers besides service workers.  To that end I have designed this to work with either a ClientInfo source or a ServiceWorkerDescriptor source.  I wrap this up into a ClientEndPoint union type.  We only support matching controlled clients if the endpoint is a service worker.

I could in theory remove the ClientEndPoint stuff for now, but I would like to keep it if you don't mind.  It doesn't complicate much and helps prove out that we're not introducing worker-specific design elements.

In terms of exposing Clients API in the future on non-service-worker globals, I think there are two ways we may do this:

1. I have an open spec issue to add it to all window/worker globals.
2. We may expose it to chrome script so that they can use the clients API.  This may be simpler for some code than using our xpcom interfaces.

Comment 320

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Add ClientManager::Claim() message to matching ClientSource objects. r=baku

Comment 321

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=480d460952da8f82a5e2ccdbb28a943a4de51a2f

Comment 322

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8934713 [details] [diff] [review]
P15 Add ClientManager::Claim() message to matching ClientSource objects. r=baku

Andrea, this patch adds a ClientManager::Claim() method which sends a message to all matching ClientSource objects.  Currently all this then does is call SetController on the ClientSource, but in a future patch it will also update the ServiceWorkerManager.

Long term we are going to move the ServiceWorkerManager to the parent process, so we will eventually remove this ClientClaimArgs message to the ClientSource.  Instead we will have the ServiceWorkerManager just use the ClientHandle::Control() method from the parent.

For now, though, we have to propagate all the way through to the correct child process.  We must update the ServiceWorkerManager in each child process as appropriate.

I'm leaving the ServiceWorkerManager integration for a later patch here to avoid breaking any existing behavior.  I want to land the rest of the dom/clients/manager code first.

Comment 323

[Ben Kelly [:bkelly, not reviewing]]

Looks like the work-in-progress patch is down to 150kb.  At its peak it was at 390kb.  More than half way...

Comment 324

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8930774 [details] [diff] [review]
P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku

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

r=me for the sync IPC.  If it's off-main-thread and the sending thread would have to block waiting for the response anyway, sync IPC seems harmless.

Comment 325

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8930774 [details] [diff] [review]
P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku

Moved to bug 1423412 for landing.

Comment 326

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Add ClientManager::Navigate() method. r=baku

Comment 327

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76fb4fd209198ba9c859e140fbba5587952cfea7

Comment 328

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Add ClientManager::Navigate() method. r=baku

Comment 329

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 330

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Implement ClientManager::OpenWindow(). r=baku

Comment 331

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a2d37cb5c1acf791cf4a63ce631518a65e2a359

Comment 332

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Remove some stale changes that we don't need any more.

Comment 333

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P18 Implement ClientHandle::Focus(). r=baku

Comment 334

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 335

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Add ClientManager::Navigate() method. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a2cfc0d0d07c4f473826e0ee445aaf3ecac6a89

Comment 336

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935138 [details] [diff] [review]
P16 Add ClientManager::Navigate() method. r=baku

Andrea, this patch adds the ClientManager::Navigate() method to support the Client.navigate(url) WebAPI.

My initial naive implementation put Navigate on ClientHandle since we already know what Client we want to navigate.  This was problematic, though, since navigating a window will destroy the current ClientSource/ClientHandle actors.

To deal with the destruction and re-creation of actors I had to put Navigate() on the ClientManager instead.  The message goes to the ClientManagerService which looks up the right target ClientSource actor.  A separate ClientNavigateOp actor is then used to initiate the navigation using the target client in its process.

For the most part the navigation bits are lifted straight from the existing ServiceWorkerWindowClient navigation bits:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerWindowClient.cpp#386

The only significant change I made was removing this code which focuses the window after navigating it:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerWindowClient.cpp#268

AFAICT the spec does not do this and there is an interaction requirement in order to focus a window.  This focus was bypassing the interaction requirement so I just removed it.

A later patch will remove the existing code in ServiceWorkerWindowClient once I switch the bindings over to my new code.

Comment 337

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935114 [details] [diff] [review]
P17 Implement ClientManager::OpenWindow(). r=baku

Andrea, this is another relatively large patch.  It adds the ClientManager::OpenWindow() method to support the Clients.openWindow() WebAPI.

When I started looking at making Clients.openWindow() work in multi-e10s I asked the e10s team the best way to open a window from any process.  In addition, I needed a way that would let me know when the window was done opening.  I was somewhat surprised we did not have any existing infrastructure that provided this (seemingly fundamental browser) feature.  So... this patch adds a bunch of infrastructure to do this.

The first step is a ClientManagerOp type to get us from the child thread/process to the parent process background thread.  There we call ClientManagerService::OpenWindow().

From the ClientManagerService we need to do some conditional things depending on if we are in e10s or non-e10s mode.

If we're in non-e10s we simply call a new utility method ClientOpenWindowInCurrentProcess().  This returns a promise which we tie back to the ClientManagerOp actor.

If we;re in e10s mode, though, we first need to hop to the appropriate content process.  This may involve creating a process if necessary.  To do this we call ContentParent::GetNewOrUsedBrowserProcess().

We also have a preference which lets us select our source process if available.  This is a quirk to make things work better before we complete the multi-e10s code.  Some service worker tests expect to see the FetchEvent for the window they open.  Currently this means we need to open in the same process if possible.  Once multi-e10s refactoring is complete we can remove this preference.

Once we have our target content process we create a new PContent ClientOpenWindowOp actor.  The resulting child actor then calls the same ClientOpenWindowInCurrentProcess() utility method.

The code in ClientOpenWindowInCurrentProcess() is basically directly lifted from the existing clients code:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerClients.cpp#510

Its quite verbose and a bit gnarly, though, so I isolated it off in a separate ClientOpenWindowUtils.(h|cpp) module.

Comment 338

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935135 [details] [diff] [review]
P18 Implement ClientHandle::Focus(). r=baku

Andrea, this implements the ClientHandle::Focus() method to support the Client.focus() WebAPI.  It basically just pipes the operation to the ClientSource, fires the chrome focus event, and then takes a snapshot of the ClientState to return.

The spec requires interaction to use this, but I will enforce that in the API layer when I implement the DOM object.

Comment 339

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 340

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #319)
> Comment on attachment 8934286 [details] [diff] [review]
> P14 Implement ClientManager::MatchAll() to support clients.matchAll()
> WebAPI. r=baku

> Note, I have a goal to support the Clients API in windows and other workers
> besides service workers.  To that end I have designed this to work with
> either a ClientInfo source or a ServiceWorkerDescriptor source.  I wrap this
> up into a ClientEndPoint union type.  We only support matching controlled
> clients if the endpoint is a service worker.
> 
> I could in theory remove the ClientEndPoint stuff for now, but I would like
> to keep it if you don't mind.  It doesn't complicate much and helps prove
> out that we're not introducing worker-specific design elements.

After further consideration, I think I'm going to remove ClientEndPoint now.  I'm going to do this in a follow-on patch, though, to avoid forcing a rebase of all these patches.

Comment 341

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P19 Implement ClientHandle::PostMessage() r=baku

Comment 342

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 343

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P19 Implement ClientHandle::PostMessage() r=baku

Comment 344

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 345

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935526 [details] [diff] [review]
P19 Implement ClientHandle::PostMessage() r=baku

Andrea, this patch adds the ClientHandle::PostMessage() method to support the Client.postMessage() WebAPI.

Currently the spec only defines Client.postMessage() as being called from a ServiceWorker thread.  The resulting MessageEvent is dispatched at the client's self.navigator.serviceWorker event target.  The MessageEvent.source is a ServiceWorker object.

In the future I plan to add support for the MessageEvent.source to be a Client and for Client.postMessage() to be called from any client thread.  When that happens we would fire the MessageEvent at the self.clients event target.  For now, though, this is not implemented yet.

Comment 346

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P20 Remove the ClientEndPoint union type for now. r=baku

Comment 347

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935539 [details] [diff] [review]
P20 Remove the ClientEndPoint union type for now. r=baku

This removes the ClientEndPoint union type I introduced in P14.  I still want to support the ClientHandle and ClientManager operations on all window/worker threads, but lets do that in a follow-up bug.

Comment 348

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=450743ac676acaa37aa6f05d0306143e08e7e6be

Comment 349

[Andrea Marchesini [:baku]]

Comment on attachment 8935114 [details] [diff] [review]
P17 Implement ClientManager::OpenWindow(). r=baku

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

::: dom/clients/manager/ClientManagerService.cpp
@@ +584,5 @@
> +
> +    // Possibly try to open the window in the same process that called
> +    // openWindow().  This is a temporary compat setting until the
> +    // multi-e10s service worker refactor is complete.
> +    if (Preferences::GetBool("dom.clients.openwindow_favors_same_process",

would be nice to cache this.

Comment 350

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #349)
> ::: dom/clients/manager/ClientManagerService.cpp
> @@ +584,5 @@
> > +
> > +    // Possibly try to open the window in the same process that called
> > +    // openWindow().  This is a temporary compat setting until the
> > +    // multi-e10s service worker refactor is complete.
> > +    if (Preferences::GetBool("dom.clients.openwindow_favors_same_process",
> 
> would be nice to cache this.

The Clients.openWindow() call is not very hot.  It requires user interaction to trigger.  Also, I hope to remove this relatively soon.  So I'd prefer not to add extra bits to cache for now if thats ok.

Comment 351

[Ben Kelly [:bkelly, not reviewing]]

Attachment: storage-denied.patch

Thanks to a test that Tom added I realized my wip patch regressed some StorageAccess checks I added for Clients.get() and Clients.matchAll().  This patch adds those back.

Let's see if it fixes the test failures:

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

Comment 352

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Capture StorageAccess when snapshoting ClientSource state. r=baku

Comment 353

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 354

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935982 [details] [diff] [review]
P1 Capture StorageAccess when snapshoting ClientSource state. r=baku

A month ago or so I added some checking in Clients.get() and Clients.matchAll() to avoid exposing Client objects where the target global is not allowed to access storage.  The goal is to prevent communication between these restricted globals and the service worker which always has access to storage.

Tom then added some console reporting to make this block easier to understand.  He also added a test for the console reporting.  This test shows that my patches here regress the storage access blocking in Clients.get() and Clients.matchAll().

To fix the regression this patch captures the StorageAccess enumeration value when we snapshot the ClientState.

Arguably I could have put this on ClientInfo, but I chose not to for two reasons:

1. In theory the pref can change, so the StorageAccess state can change while a window is still open.
2. An initial about:blank window may have StorageAccess, but then lose it after the real URL loads.

Since the value seems like it can sometimes change I chose to put it in ClientState.  This also keeps ClientInfo smaller for now.

If necessary we can move it to ClientInfo in the future.

Note, I want to implement the actual block in the Clients API DOM layer and not in the infrastructure here.  In theory I want other parts of the system to be able to use this code, so I don't want to bake this restriction into the infrastructure bits.

Comment 355

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Cleanup test_openWindow.html mochitest service worker a bit. r=baku

Comment 356

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove the dom.serviceWorkers.openWindow.enabled pref. r=baku

Comment 357

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 358

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935986 [details] [diff] [review]
P2 Cleanup test_openWindow.html mochitest service worker a bit. r=baku

While investigating other failures I noticed this test doesn't use event.waitUntil() like it should.  This patch fixes that and some whitespace issues.

Comment 359

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8935987 [details] [diff] [review]
P3 Remove the dom.serviceWorkers.openWindow.enabled pref. r=baku

This patch removes the pref controlling if Clients.openWindow() is enabled.  We shipped this a long time ago and don't need the ability to turn it off individually any more.  Rather than make the new DOM object work with this I'd rather just remove the pref.

Comment 360

[Ben Kelly [:bkelly, not reviewing]]

Next step is to clean up the new Client/Clients DOM classes and then land them.  Cleanup mainly consists of removing my stale code that tried to support windows.  The code can be simpler for now since we are only supporting these on service worker globals at first.

After that, I will like have a series of patches that must land in a single push that swaps the implementations.

And finally, some patches to remove stale code.

Assuming reviews are possible during Austin I hope to have this bug resolved next week.  (I don't think the reviews will be that complicated or hard FWIW.)

Comment 361

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 362

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku

Comment 363

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 364

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936169 [details] [diff] [review]
P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku

Andrea, this adds the DOM Client and Clients objects.  It does not hook them into the bindings yet.

Comment 365

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d9de760619fe03ababa9450c7d4bc2b91633c20

Comment 366

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Switch bindings over to new Client and Clients classes. r=baku

Comment 367

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 368

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29e57934b344a44dff995d4b475497db1b3dcdb7

Comment 369

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936173 [details] [diff] [review]
P5 Switch bindings over to new Client and Clients classes. r=baku

Andrea, this patch switches the bindings over to the new Client and Clients classes.

NOTE, I will not land this until the rest of the wip patch is split up and reviewed.  This patch needs to land in the same push as later code that will:

1. Fix the FetchEvent.clientId to use the new ClientInfo.Id() value.
2. Fire the ControllerChange event from ClientSource::SetController().
3. Call into ServiceWorkerManager from ClientSource::Claim().
4. Test expectations are updated for new things passing.

This patch, however, updates the bindings, webidl files, and removes references to the old classes.

Comment 370

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 371

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Switch bindings over to new Client and Clients classes. r=baku

Sorry, the last patch had some build issues that didn't show locally for some reason.  Dropping the flag for now till I can get a green try.  I can't land this without splitting more patches anyway.

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

Comment 372

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Switch bindings over to new Client and Clients classes. r=baku

Comment 373

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Use the ClientInfo.Id() value for FetchEvent.clientId. r=baku

Comment 374

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 375

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make ServiceWorkerManager use ClientHandle::Control() for claiming and ClientSource::SetController() fire the controller change event. r=baku

Comment 376

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Update WPT test expectations for re-written Clients API

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16c4be4cdb78a1c3cffb01d9ef5d9630b65eaa6b

Comment 377

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Switch bindings over to new Client and Clients classes. r=baku

Comment 378

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Use the ClientInfo.Id() value for FetchEvent.clientId. r=baku

Comment 379

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make ServiceWorkerManager use ClientHandle::Control() for claiming and ClientSource::SetController() fire the controller change event. r=baku

Comment 380

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6252b2608c68853cbe83e93eff7775545ee58ace

Comment 381

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Update WPT test expectations for re-written Clients API

Comment 382

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936304 [details] [diff] [review]
P5 Switch bindings over to new Client and Clients classes. r=baku

Andrea, this patch updates the bindings and webidl files to use the new Client and Clients classes.  It also removes the old class files and orphaned code in ServiceWorkerManager.

Comment 383

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936305 [details] [diff] [review]
P6 Use the ClientInfo.Id() value for FetchEvent.clientId. r=baku

Andrea, this patch makes us you the LoadInfo's ClientInfo ID for FetchEvent.clientId.  This is in place of the old nsIDocument's ID value.

Note, we only expose FetchEvent.clientId here for subresource requests.  Some sites are using it to infer if a request is subresource or non-subresource.  Until we fully ship the "resultingClientId" stuff we need to keep it null for non-subresource.

This patch also removes all the machinery for the nsIDocument ID, etc.  Notably, we no longer need to register every nsIDocument in the observer service.

I also have one small extra change in here to stop passing the scope for the ServiceWorker's WorkerPrivate name.  Technically I should have included that with the patches in bug 1412858.  I'm including it here since it was left over in this file in my wip patch.

Comment 384

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936306 [details] [diff] [review]
P7 Make ServiceWorkerManager use ClientHandle::Control() for claiming and ClientSource::SetController() fire the controller change event. r=baku

Andrea, this patch integrates the ServiceWorkerManager "claim" support with ClientHandle::Control().  It also moves the dispatch of the controllerchange event to ClientSource::SetController().

Eventually we won't have to call into ServiceWorkerManager from ClientSource at all once its been migrated to the parent process.  For now, though, we must keep calling into it from ClientSource::Claim().  Once SWM is moved to the parent we can replace the ClientSource::Claim() stuff with a direct Control() message from the ClientManagerService::Claim() method.

Comment 385

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936307 [details] [diff] [review]
P8 Update WPT test expectations for re-written Clients API

Update WPT test expectations.  We now handle about:blank replacement correctly and also can observe SharedWorker/Worker clients via matchAll().

Comment 386

[Andrea Marchesini [:baku]]

Comment on attachment 8936169 [details] [diff] [review]
P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku

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

::: dom/clients/api/Client.cpp
@@ +33,5 @@
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +Client::~Client()

= default

::: dom/clients/api/Clients.cpp
@@ +34,5 @@
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +Clients::~Clients()

= default? But I guess it's going to change in the next patches.

Comment 387

[Andrea Marchesini [:baku]]

Comment on attachment 8936304 [details] [diff] [review]
P5 Switch bindings over to new Client and Clients classes. r=baku

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

cool!

::: dom/bindings/Bindings.conf
@@ +149,5 @@
>      'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate',
>  },
>  
>  'Client': {
> +    'nativeType': 'mozilla::dom::Client',

why do you need this? Get rid of the full block. mozilla::dom::Client is by default the nativetype.

@@ +154,4 @@
>  },
>  
>  'Clients': {
> +    'nativeType': 'mozilla::dom::Clients',

same here.

Comment 388

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the reviews!

From my last try push I have one new persistent test failure:

devtools/client/aboutdebugging/test/browser_service_workers_push_service.js

It only fails on non-e10s.  I'm investigating.  Hopefully its just something I accidentally dropped when shuffling patches.

Comment 389

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Disable browser_service_workers_push_service.js on non-e10s due to races. r=jryans

I spoke with Ryan on IRC and he gave me r+ to disable this test for now.  It only fails on non-e10s.  AFAIK we don't ship about:debugging in non-e10s any more.  Also, it seems to be some timing dependent problem in the devtools related to push.  I don't think my patches here should impact this code outside of timing.

Comment 390

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku

Comment 391

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Switch bindings over to new Client and Clients classes. r=baku

Comment 392

[Ben Kelly [:bkelly, not reviewing]]

One last try push...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=269210e32237135f33e8c840fa799e3c6d54fbb6

Comment 393

[Ben Kelly [:bkelly, not reviewing]]

Last try had a single orange in a new devtools test.  I couldn't reproduce locally or in retriggers.  I filed bug 1424914 for now.

Comment 394

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3fb8bd8b10d
P1 Capture StorageAccess when snapshoting ClientSource state. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f23acc15006
P2 Cleanup test_openWindow.html mochitest service worker a bit. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d09b4467c62
P3 Remove the dom.serviceWorkers.openWindow.enabled pref. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b0cd514d46
P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/63938d2d5492
P5 Switch bindings over to new Client and Clients classes. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa02d97591e
P6 Use the ClientInfo.Id() value for FetchEvent.clientId. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2388a414a4
P7 Make ServiceWorkerManager use ClientHandle::Control() for claiming and ClientSource::SetController() fire the controller change event. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/93b27fc24c01
P8 Update WPT test expectations for re-written Clients API.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec647e220016
P9 Disable browser_service_workers_push_service.js on non-e10s due to races. r=jryans

Comment 395

[Ben Kelly [:bkelly, not reviewing]]

I see an orange in the inbound push on Linux64 opt tc-M-e10s(4):

  https://treeherder.mozilla.org/logviewer.html#?job_id=151339987&repo=mozilla-inbound&lineNumber=4289

I'm doing some re-triggers to see how intermittent this is.  I didn't see it in my try pushes at all.

Comment 396

[Ben Kelly [:bkelly, not reviewing]]

The intermittent in comment 395 has about a 20% retrigger rate it seems.  I'll investigate first thing tomorrow in bug 1424943.

Comment 397

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e3fb8bd8b10d
https://hg.mozilla.org/mozilla-central/rev/3f23acc15006
https://hg.mozilla.org/mozilla-central/rev/6d09b4467c62
https://hg.mozilla.org/mozilla-central/rev/f7b0cd514d46
https://hg.mozilla.org/mozilla-central/rev/63938d2d5492
https://hg.mozilla.org/mozilla-central/rev/baa02d97591e
https://hg.mozilla.org/mozilla-central/rev/3a2388a414a4
https://hg.mozilla.org/mozilla-central/rev/93b27fc24c01
https://hg.mozilla.org/mozilla-central/rev/ec647e220016

Comment 398

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding a release note for 59 beta/dev ed on Ben Kelly's suggestion:
"Service worker Clients API can now find and communicate with windows in a separate browser process"

Comment 399

[Liz Henry (:lizzard) (relman/hg->git project)]

On further discussion, sounds like we should add this to MDN instead, to https://developer.mozilla.org/en-US/Firefox/Releases/59

Comment 400

[Liz Henry (:lizzard) (relman/hg->git project)]

I added a note for this and two other issues under the "Service workers" heading on the MDN page mentioned in comment 399.