Ben Kelly [:bkelly, not reviewing] Assignee
	Description • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 1 • 8 years ago

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?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 2 • 8 years ago

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

	Jim Mathies [:jimm]
	Comment 3 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 4 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 5 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 6 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 7 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 8 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 10 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 11 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 12 • 7 years ago

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

	(no longer active)
	Comment 13 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 14 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 15 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 16 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 17 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 18 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 19 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 20 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 21 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 22 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 24 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 25 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 26 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 27 • 7 years ago

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]

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 28 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 29 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 30 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 31 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 32 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 33 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 34 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 35 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 36 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 37 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 38 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 39 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 40 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 42 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 43 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 44 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 45 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 46 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 47 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 48 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 49 • 7 years ago

Actually include the important ClientThing.h header.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 50 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 51 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 53 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 54 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 55 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 56 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 57 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 58 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 59 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 60 • 7 years ago

This gets clients-matchall.https.html passing.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 61 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 62 • 7 years ago

Make claim-fetch.https.html pass.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 63 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 64 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 65 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 66 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 67 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 68 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 69 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 70 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 71 • 7 years ago

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.