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