Closed Bug 1293277 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Iteration:
54.3 - Mar 6
Tracking Status
relnote-firefox --- -
firefox59 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [e10s-multi:+])

Attachments

(9 files, 223 obsolete files)

(deleted), patch
baku
: review+
Details | Diff | Splinter Review
(deleted), patch
baku
: review+
Details | Diff | Splinter Review
(deleted), patch
baku
: review+
Details | Diff | Splinter Review
(deleted), patch
baku
: review+
Details | Diff | Splinter Review
(deleted), patch
baku
: review+
Details | Diff | Splinter Review
(deleted), patch
baku
: review+
Details | Diff | Splinter Review
(deleted), patch
bkelly
: review+
Details | Diff | Splinter Review
(deleted), patch
bkelly
: review+
Details | Diff | Splinter Review
(deleted), patch
bkelly
: review+
Details | Diff | Splinter Review
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.
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?
Flags: needinfo?(jmathies)
(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.
I don't know of anything. The multi e10s team might be able to help, adding to their triage list.
Flags: needinfo?(jmathies)
Whiteboard: e10s-multi:?
I have some ideas about how I want to implement this. I'm going to take it for now.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8812928 - Attachment is obsolete: true
Attachment #8818696 - Attachment is patch: true
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.
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
Blocks: 1032521
Blocks: 1183625
Blocks: 1231211
Blocks: 1231218
Depends on: 1333573
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8818696 - Attachment is obsolete: true
Attachment #8832648 - Attachment is patch: true
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
Boris suggests that we get the ClientSource from the NS_NewChannel() location to the creation of the inner window via the nsILoadInfo.
(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.
(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.
Depends on: 1337439
Depends on: 1337522
Whiteboard: e10s-multi:? → [e10s-multi:?]
Attached patch wip (obsolete) (deleted) — Splinter Review
Updated work-in-progress that also creates the ClientSource before the docshell load and passes the reserved client on to the resulting inner window.
Attachment #8832648 - Attachment is obsolete: true
Depends on: 1339587
No longer depends on: 1339587
Depends on: 1290936
Depends on: 1339844
No longer depends on: 1290936
Blocks: 1264177
Depends on: 1340201
Attached patch wip (obsolete) (deleted) — Splinter Review
Update work-in-progress with code to set the window Client ExecutionReady.
Attachment #8837307 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8838268 - Attachment is obsolete: true
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.
Depends on: 1266747
Attached patch wip (obsolete) (deleted) — Splinter Review
Incorporate focus time now that bug 1266747 added it to the document.
Attachment #8839708 - Attachment is obsolete: true
Depends on: 1343308
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8842042 - Attachment is obsolete: true
(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.
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8842606 - Attachment is obsolete: true
Iteration: --- → 54.3 - Mar 6
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8843084 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8843422 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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).
Attachment #8843484 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8843527 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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]
Attachment #8843594 - Attachment is obsolete: true
(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.
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8843598 - Attachment is obsolete: true
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.
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
Rough-in all the boilerplate needed to add two more actors.
Attachment #8843617 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8843745 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8844303 - Attachment is obsolete: true
Depends on: 1345132
Attached patch wip (obsolete) (deleted) — Splinter Review
Add focus operation to the infrastructure. Can't test yet since I don't have the DOM API implemented yet.
Attachment #8844482 - Attachment is obsolete: true
Depends on: 1345251
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8844567 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8844660 - Attachment is obsolete: true
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.
Depends on: 1311324
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8844726 - Attachment is obsolete: true
Depends on: 1345943
Attached patch wip (obsolete) (deleted) — Splinter Review
Update the patch based on the two MessageEvent dependent bugs. Also, implement the ServiceWorkerDescriptor type and use it.
Attachment #8845151 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8846081 - Attachment is obsolete: true
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8846204 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8846750 - Attachment is obsolete: true
I need the changes in bug 1340163 in order to get the origin out of a PrincipalInfo() OMT.
Depends on: 1340163
Whiteboard: [e10s-multi:?] → [e10s-multi:+]
Attached patch wip (obsolete) (deleted) — Splinter Review
Implemented the service side of the matchAll() query. Still needs some cleanup, but might work.
Attachment #8846823 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8847280 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8847385 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Actually include the important ClientThing.h header.
Attachment #8847788 - Attachment is obsolete: true
Blocks: 1348082
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8847790 - Attachment is obsolete: true
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()
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8848237 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8848575 - Attachment is obsolete: true
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
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8848681 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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().
Attachment #8849693 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8849729 - Attachment is obsolete: true
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8850193 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
This gets clients-matchall.https.html passing.
Attachment #8850267 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
This gets postmessage-to-client.https.html to pass. Turns out I forgot to implement Client::PostMessage(), although most of the infrastructure was there.
Attachment #8850644 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Make claim-fetch.https.html pass.
Attachment #8850676 - Attachment is obsolete: true
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8850686 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8850707 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8850779 - Attachment is obsolete: true
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
I did fix some other stupid bugs in the navigate path, though.
Attachment #8850787 - Attachment is obsolete: true
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.
(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.
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.
Depends on: 1350398
Depends on: 1350433
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8850811 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8851108 - Attachment is obsolete: true
We also fail at least this mochitest: dom/workers/test/serviceworkers/test_match_all_client_properties.html
Depends on: 1351935
Depends on: 1351959
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8851180 - Attachment is obsolete: true
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
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8852800 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8853337 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8853373 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Rebase and update for corrected client-navigate.https.html test case in bug 1351935.
Attachment #8853387 - Attachment is obsolete: true
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.
Attached patch openwindow-wip (obsolete) (deleted) — Splinter Review
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.
Attached patch openwindow-wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8857206 - Attachment is obsolete: true
Attached patch openwindow-wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8857669 - Attachment is obsolete: true
Depends on: 1313096
Attached patch wip (obsolete) (deleted) — Splinter Review
Rebase.
Attachment #8856710 - Attachment is obsolete: true
Attached patch openwindow-wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8858402 - Attachment is obsolete: true
Depends on: 1357463
Attached patch openwindow-wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8858927 - Attachment is obsolete: true
Attached patch openwindow-wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8859335 - Attachment is obsolete: true
Attached patch openwindow-wip (obsolete) (deleted) — Splinter Review
Slightly better code for the "same process" case. Next I will try to figure out android.
Attachment #8859647 - Attachment is obsolete: true
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.
Attached patch android.patch (obsolete) (deleted) — Splinter Review
Start of the android open window patch.
Depends on: 1358622
Attached patch android.patch (obsolete) (deleted) — Splinter Review
This gets the last part of openWindow working on android.
Attachment #8860161 - Attachment is obsolete: true
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.
Depends on: 1359230
Attached patch grr (obsolete) (deleted) — Splinter Review
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.
No longer depends on: 1359230
Depends on: 1361051
Depends on: 1361166
Depends on: 1361722
Depends on: 1362033
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.
Thing to try: Call ModifyBusyCountFromWorker() in ClientManager code to force the busy count back down.
Depends on: 1362444
There are some windows specific failures. Fixes for that, but no talos in this build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f9985dd2cabf76be9b9341452a0ff45f93e8133
Depends on: 1364276
Depends on: 1364277
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.
Depends on: 1363848
Depends on: 1366089
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8858925 - Attachment is obsolete: true
Attachment #8859652 - Attachment is obsolete: true
Attachment #8861032 - Attachment is obsolete: true
Attachment #8861184 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8876908 - Attachment is obsolete: true
Depends on: 1372962
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8877356 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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
Attachment #8877668 - Attachment is obsolete: true
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
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
Attachment #8880542 - Attachment is obsolete: true
Depends on: 1379243
Trying to get a build working without AbstractThread and proper runnable labeling: https://treeherder.mozilla.org/#/jobs?repo=try&revision=953d07c894e777755f343daef9a7392f7bcf66a5
Attached patch wip (obsolete) (deleted) — Splinter Review
This patch removes AbstractThread() in favor of nsIGlobalObject::EventTargetFor(). This should ensure runnables are labeled correctly. Depends on bug 1379243.
Attachment #8880849 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Rebase
Attachment #8885452 - Attachment is obsolete: true
Priority: -- → P2
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8891524 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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
Attachment #8918004 - Attachment is obsolete: true
I will work on adding comments and patch splitting monday.
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.
Attached patch wip (obsolete) (deleted) — Splinter Review
The LoadInfo::mClientInfo was being lost across the internal redirect introduced by InterceptedHttpChannel. We should copy this value in the LoadInfo copy constructor.
Attachment #8918457 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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.
Attachment #8918937 - Attachment is obsolete: true
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.
Try is green. Going to start patch splitting for review.
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8919288 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8919908 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8920286 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8920341 - Attachment is obsolete: true
Attachment #8919905 - Attachment is obsolete: true
Attachment #8920695 - Attachment description: P1 Respect LOAD_CALL_CONTENT_SNIFFERS when a channel is intercepted by a ServiceWorker. r=valentin → P1 Add a ref-counted WorkerHolderToken() class. r=baku
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.
Attachment #8920695 - Flags: review?(amarchesini)
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.
Attachment #8920284 - Flags: review?(amarchesini)
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.
Attachment #8920700 - Attachment is obsolete: true
Attachment #8920701 - Flags: review?(amarchesini)
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.
Attachment #8920340 - Flags: review?(amarchesini)
Attached patch P5 Add ClientInfo and ClientState types. r=baku (obsolete) (deleted) — Splinter Review
Attachment #8920375 - Attachment is obsolete: true
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.
Attachment #8920704 - Flags: review?(amarchesini)
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
Attachment #8920284 - Flags: review?(amarchesini) → review+
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.
Attachment #8920701 - Flags: review?(amarchesini) → review+
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.
Attachment #8920340 - Flags: review?(amarchesini) → review+
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.
Attachment #8920704 - Flags: review?(amarchesini) → review+
I want to see how you use WorkerHolderToken before reviewing patch 1. I keep the r?.
(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.
(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.
Attached patch P5 Add ClientInfo and ClientState types. r=baku (obsolete) (deleted) — Splinter Review
Attachment #8920704 - Attachment is obsolete: true
Attachment #8921618 - Flags: review+
Attached patch wip (obsolete) (deleted) — Splinter Review
Remove nsIIPCBackgroundChildCreateCallback usage since its going away.
Attachment #8920379 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Try to remove some more dead code after the PBackground changes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d7d7c9ecdf2400ebd2196b1d48f03f42dc68ef
Attachment #8921624 - Attachment is obsolete: true
Depends on: 1411746
Fix a mis-typed logic condition. I swapped (oldState == newState) for (oldState != newState).
Attachment #8920340 - Attachment is obsolete: true
Attachment #8922084 - Flags: review+
Attached patch wip (obsolete) (deleted) — Splinter Review
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
Attachment #8921644 - Attachment is obsolete: true
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.
No longer depends on: 1313096
Blocks: 1337439
No longer depends on: 1337439
Blocks: 1412856
No longer depends on: 1357463
Depends on: 1412858
Comment on attachment 8920284 [details] [diff] [review] P2 Expose a "parsed" ServiceWorkerState value. r=baku Moved to bug 1412858.
Attachment #8920284 - Attachment is obsolete: true
Attachment #8921617 - Attachment is obsolete: true
Attachment #8922084 - Attachment is obsolete: true
Depends on: 1412864
Comment on attachment 8921618 [details] [diff] [review] P5 Add ClientInfo and ClientState types. r=baku Moved to bug 1412864.
Attachment #8921618 - Attachment is obsolete: true
Attached patch P2 Add ClientThing base class. r=baku (obsolete) (deleted) — Splinter Review
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8922085 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
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
Attachment #8923528 - Attachment is obsolete: true
Attached patch P2 Add ClientThing base class. r=baku (obsolete) (deleted) — Splinter Review
Attachment #8923527 - Attachment is obsolete: true
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.
Depends on: 1413056
Attachment #8923579 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8923580 - Attachment is obsolete: true
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!
Attachment #8923581 - Flags: review?(amarchesini)
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.
Attachment #8923854 - Flags: review?(amarchesini)
Attached patch P4 Add the ClientManager class. r=baku (obsolete) (deleted) — Splinter Review
Attachment #8923581 - Flags: review?(amarchesini) → review+
Attached patch P4 Add the ClientManager class. r=baku (obsolete) (deleted) — Splinter Review
Attachment #8923891 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8923897 - Attachment is obsolete: true
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.
Attachment #8923896 - Flags: review?(amarchesini)
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.
Attachment #8923962 - Flags: review?(amarchesini)
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!
Attachment #8923984 - Flags: review?(amarchesini)
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
Attachment #8923854 - Flags: review?(amarchesini) → review+
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?
Attachment #8920695 - Flags: review?(amarchesini) → review+
Attachment #8920695 - Attachment is obsolete: true
Attachment #8924200 - Flags: review+
Attachment #8923854 - Attachment is obsolete: true
Attachment #8924201 - Flags: review+
Depends on: 1413604
Depends on: 1413606
Attachment #8924200 - Attachment is obsolete: true
Attachment #8923581 - Attachment is obsolete: true
Attachment #8924201 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8923988 - Attachment is obsolete: true
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.
Attachment #8924306 - Flags: review?(amarchesini)
Attached patch validate.patch (obsolete) (deleted) — Splinter Review
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
Attached patch validate.patch (obsolete) (deleted) — Splinter Review
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
Attachment #8924627 - Attachment is obsolete: true
Attached patch validate.patch (obsolete) (deleted) — Splinter Review
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
Attached patch validate.patch (obsolete) (deleted) — Splinter Review
Attachment #8924676 - Attachment is obsolete: true
Attachment #8924712 - Attachment is obsolete: true
Attached patch validate.patch (obsolete) (deleted) — Splinter Review
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
Attachment #8924728 - Attachment is obsolete: true
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 on attachment 8923896 [details] [diff] [review] P4 Add the ClientManager class. r=baku Review of attachment 8923896 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientManager.cpp @@ +93,5 @@ > + return ref.forget(); > +} > + > +void > +ClientManager::PBackgroundActorCreated(PBackgroundChild* aActor) Can you consider moving this code in the CTOR directly? @@ +99,5 @@ > + NS_ASSERT_OWNINGTHREAD(ClientManager); > + MOZ_ASSERT(aActor); > + > + if (IsShutdown()) { > + return; This cannot happen. We are in CTOR.
Attachment #8923896 - Flags: review?(amarchesini) → review+
Attachment #8923962 - Flags: review?(amarchesini) → review+
Comment on attachment 8923984 [details] [diff] [review] P6 Add the ClientSource class and hook it into ClientManager/ClientManagerService. r=baku Review of attachment 8923984 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientManager.cpp @@ +76,5 @@ > + > + if (IsShutdown()) { > + return nullptr; > + } > + nsID id; rv = nsContentUtils::GenerateUUIDInPlace(id); ... ::: dom/clients/manager/ClientSource.h @@ +9,5 @@ > +#include "mozilla/dom/ClientInfo.h" > +#include "mozilla/dom/ClientThing.h" > + > +#ifdef XP_WIN > +#undef PostMessage No PostMessage in this method.
Attachment #8923984 - Flags: review?(amarchesini) → review+
Comment on attachment 8924306 [details] [diff] [review] P7 Add ClientHandle class and make it attach to the correct ClientSource. r=baku Review of attachment 8924306 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientManager.cpp @@ +105,5 @@ > +{ > + NS_ASSERT_OWNINGTHREAD(ClientManager); > + MOZ_DIAGNOSTIC_ASSERT(aSerialEventTarget); > + > + if (IsShutdown()) { Can it be that we are shutting down? ::: dom/clients/manager/ClientSourceParent.cpp @@ +31,5 @@ > mService->RemoveSource(this); > + > + nsTArray<ClientHandleParent*> handleList; > + mHandleList.SwapElements(handleList); > + for (ClientHandleParent* handle : handleList) { Does this support the removing of elements when looping? Can it happen that Send__delete__ triggers ClientHandleParent::ActorDestroy directly? @@ +79,5 @@ > + > +void > +ClientSourceParent::DetachHandle(ClientHandleParent* aClientHandle) > +{ > + MOZ_DIAGNOSTIC_ASSERT(aClientHandle); Do you want to assert mHandleList.Containers(aClientHandle) ? If not, why can DetachHandle called more than once?
Attachment #8924306 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #215) > ::: dom/clients/manager/ClientManager.cpp > @@ +105,5 @@ > > +{ > > + NS_ASSERT_OWNINGTHREAD(ClientManager); > > + MOZ_DIAGNOSTIC_ASSERT(aSerialEventTarget); > > + > > + if (IsShutdown()) { > > Can it be that we are shutting down? Yes, if some code tries to use the ClientManager to create a handle at the same time we are shutting things down. In that case the ClientManager actor may have been destroyed already. (Although I see perhaps we should be setting the shutdown flag there and we are not.)
(In reply to Andrea Marchesini [:baku] from comment #215) > ::: dom/clients/manager/ClientSourceParent.cpp > @@ +31,5 @@ > > mService->RemoveSource(this); > > + > > + nsTArray<ClientHandleParent*> handleList; > > + mHandleList.SwapElements(handleList); > > + for (ClientHandleParent* handle : handleList) { > > Does this support the removing of elements when looping? > Can it happen that Send__delete__ triggers ClientHandleParent::ActorDestroy > directly? > > @@ +79,5 @@ > > + > > +void > > +ClientSourceParent::DetachHandle(ClientHandleParent* aClientHandle) > > +{ > > + MOZ_DIAGNOSTIC_ASSERT(aClientHandle); > > Do you want to assert mHandleList.Containers(aClientHandle) ? If not, why > can DetachHandle called more than once? I think I can fix both of these issues by looping on a copy instead of using SwapElements(). We can only detach once, but SwapElements() is removing the handle before detach is called.
Attached patch P4 Add the ClientManager class. r=baku (obsolete) (deleted) — Splinter Review
Attachment #8923896 - Attachment is obsolete: true
Attachment #8926243 - Flags: review+
Attachment #8923962 - Attachment is obsolete: true
Attachment #8926246 - Flags: review+
Attached patch wip (obsolete) (deleted) — Splinter Review
I had to fix a couple things in the wip patch, but the reviewed patches are good. I'm going to land in a separate bug.
Attachment #8926249 - Attachment is obsolete: true
Depends on: 1415779
Attachment #8926243 - Attachment is obsolete: true
Attachment #8926246 - Attachment is obsolete: true
Attachment #8926247 - Attachment is obsolete: true
Attachment #8926248 - Attachment is obsolete: true
Blocks: 1415829
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8926713 - Attachment is obsolete: true
Attached patch validate.patch (obsolete) (deleted) — Splinter Review
Lets simplify all the special cases and just test the main path. As MozURL improves this will begin covering more cases. https://treeherder.mozilla.org/#/jobs?repo=try&revision=29cc3887b37ee689416b182d7ee06c8d00b0cc04
Attachment #8927494 - Attachment is obsolete: true
Comment on attachment 8927539 [details] [diff] [review] P1 Validate ClientSource creation path across IPC boundary. r=baku Andrea, this patch adds some validation to the ClientSource creation path. Specifically it: 1) Validates the PrincipalInfo used to create the source. I want to avoid main thread bounces, so I am using MozURL here. This doesn't handle every possible internal URL (like nested view-source:, etc) but it does allow us to validate all the http:/https:/blob: URLs content might see in the Clients API. Ultimately improvements in MozURL and OMT principal support should let us handle all principals. This will be desirable so that we can use this infrastructure for other features besides Clients API. This validation is done on both the child side and the parent side. If a PrincipalInfo fails on the child side we just avoid creating the IPC actor and leave the ClientSource in its shutdown space. Basically the Client just won't show up in the shared list. If validation fails on the parent side we assume someone is spoofing the IPC connection and MOZ_CRASH. 2) This patch also validates that we don't try to add more than one ClientSource with the same UUID. If this happens we destroy the actor for the second duplicate UUID and leave the second duplicate ClientSource in its shutdown disconnected state.
Attachment #8927539 - Flags: review?(amarchesini)
Comment on attachment 8927634 [details] [diff] [review] P2 Add a way for the window/worker/docshell to mark a ClientSource execution ready. r=baku This page adds the concept of setting a Client to the "Execution Ready" state. For windows this corresponds to step 4 here: https://html.spec.whatwg.org/#set-the-active-document And for workers this corresponds to step 22 here: https://html.spec.whatwg.org/#worker-processing-model Basically the document or worker script is loaded and valid. We now have a creation URL: https://html.spec.whatwg.org/#concept-environment-creation-url Up until this point a Client's URL is the empty string. We are also able to set the frame type of the Client at this stage. This patch adds three different ways to mark a ClientSource execution ready: 1. A WorkerPrivate can mark its ClientSource execution ready. 2. An inner window can mark its ClientSource execution ready. 3. A docshell also own and mark a ClientSource execution ready. This represents the initial about:blank window for cases where we have optimized away the early creation of the about:blank document. In this case we need to still treat the initial about:blank as existing even if we haven't actually created the nsDocument and inner window yet. For this reason we allow the docshell to handle this "initial" client. Ultimately each path will result in a creation URL and frame type being set on the ClientSource's ClientInfo. A message is then propagated to the parent side and any attached Handle objects. We validate the creation URL against the ClientSource PrincipalInfo. Just like with the principal validation in the last patch we validate both on the child and parent sides. If the child side fails we just teardown the IPC actor leaving it disconnected. If the parent side fails we treat it as a fatal error and MOZ_CRASH. Nothing calls the methods to set execution ready yet. I need to add a few more bits in other places before I can integrate ClientSource and this method into nsGlobalWindow/nsDocShell/WorkerPrivate.
Attachment #8927634 - Flags: review?(amarchesini)
Comment on attachment 8927539 [details] [diff] [review] P1 Validate ClientSource creation path across IPC boundary. r=baku Review of attachment 8927539 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientManagerParent.cpp @@ +105,5 @@ > +ClientManagerParent::RecvPClientSourceConstructor(PClientSourceParent* aActor, > + const ClientSourceConstructorArgs& aArgs) > +{ > + ClientSourceParent* actor = static_cast<ClientSourceParent*>(aActor); > + actor->Init(); return actor->Init() ? IPC_OK() : IPC_FAILURE(..) ::: dom/clients/manager/ClientSourceParent.cpp @@ +72,5 @@ > + // Ensure the principal is reasonable before adding ourself to the service. > + // Since we validate the principal on the child side as well, any failure > + // here is treated as fatal. > + if (NS_WARN_IF(!ClientIsValidPrincipalInfo(mClientInfo.PrincipalInfo()))) { > + MOZ_CRASH("ClientSourceParent::Init() got invalid principal"); This is wrong. we don't want the parent to crash. We should maybe return a boolean and kill the content process. @@ +79,5 @@ > + > + // Its possible for AddSource() to fail if there is already an entry for > + // our UUID. This should not normally happen, but could if someone is > + // spoofing IPC messages. > + if (NS_WARN_IF(!mService->AddSource(this))) { same here. ::: dom/clients/manager/ClientValidation.h @@ +8,5 @@ > + > +namespace mozilla { > + > +namespace ipc { > + class PrincipalInfo; why this indentation? Usually we don't do it.
Attachment #8927539 - Flags: review?(amarchesini) → review-
Updated patch that uses ContentParent->KillHard() as we discussed in IRC. I hacked a failure in the parent manually so I could test this locally. Note, I also decided to perform the validation in ClientSourceParent even when e10s is disabled. I'd rather keep the e10s vs non-e10s paths as similar as possible instead of optimizing for a mode that will eventually go away.
Attachment #8927539 - Attachment is obsolete: true
Attachment #8927896 - Flags: review?(amarchesini)
Update the execution ready patch to use the validation failure mechanism introduced in the new P1 patch.
Attachment #8927634 - Attachment is obsolete: true
Attachment #8927634 - Flags: review?(amarchesini)
Attachment #8927897 - Flags: review?(amarchesini)
Comment on attachment 8927896 [details] [diff] [review] P1 Validate ClientSource creation path across IPC boundary. r=baku Review of attachment 8927896 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientManagerService.cpp @@ +91,5 @@ > auto entry = mSourceTable.LookupForAdd(aSource->Info().Id()); > + // Do not permit overwriting an existing ClientSource with the same > + // UUID. This would allow a spoofed ClientParentSource actor to > + // intercept postMessage() intended for the real actor. > + if (entry) { I would use WARN_IF @@ +104,5 @@ > { > AssertIsOnBackgroundThread(); > MOZ_ASSERT(aSource); > auto entry = mSourceTable.Lookup(aSource->Info().Id()); > + if (!entry) { here as well
Attachment #8927896 - Flags: review?(amarchesini) → review+
Comment on attachment 8927897 [details] [diff] [review] P2 Add a way for the window/worker/docshell to mark a ClientSource execution ready. r=baku Review of attachment 8927897 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientSource.cpp @@ +159,5 @@ > + if (NS_WARN_IF(!doc)) { > + return NS_ERROR_UNEXPECTED; > + } > + > + nsCString spec; nsAutoCString?
Attachment #8927897 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #240) > ::: dom/clients/manager/ClientSource.cpp > @@ +159,5 @@ > > + if (NS_WARN_IF(!doc)) { > > + return NS_ERROR_UNEXPECTED; > > + } > > + > > + nsCString spec; > > nsAutoCString? In this case the spec string is going to be passed over IPC which required nsCString. Using nsAutoCString would require an extra copy in this case. I can add a comment.
Note, I had to do `NS_WARN_IF(!!entry)` to get it to coerce correctly.
Attachment #8927896 - Attachment is obsolete: true
Attachment #8928205 - Flags: review+
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8927899 - Attachment is obsolete: true
Depends on: 1417172
Attachment #8928205 - Attachment is obsolete: true
Attachment #8928220 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Remove some dead code.
Attachment #8928251 - Attachment is obsolete: true
Comment on attachment 8928309 [details] [diff] [review] P1 Allow the reserved/initial/source client data and service worker to be marked on LoadInfo. r=baku r=valentin Valentin, Andrea, this patch adds some additional fields to the LoadInfo object. These are mainly things that are being "passed through" the channel to communicate state about: 1. The global that created the channel 2. The global that will be created as a result of a non-subresource channel 3. Any service worker controlling the channel via FetchEvent interception The globals here are represented by either a simple ClientInfo snapshot structure: https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientInfo.h Or possibly a live ClientSource object backed by an IPC actor: https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientSource.h In the future I would like to eventually migrate some other data that is on LoadInfo into the ClientInfo. For example, its possible we can replace triggering principal with the ClientInfo principal. Things like CSP and referral policy could also move to the Client in the future. The advantage of doing this is that we can produce a ClientInfo equally well from both windows and workers. So it would break the weird thing where most stuff works with nsIDocument available, but probably breaks on worker threads without an nsIDocument. I tried to write good comments, but to summarize some of the terms used: - "reserved client" represents a window or worker global which is not active yet, but will be created as a result of the non-subresource nsIChannel. - "initial client" represents a window global that will be used after the nsIChannel completes, but its already active because its an initial about:blank document. This is a legacy feature of the web that we unfortunately have to deal with. - The simple load info "client" refers to the window or worker global that initiated nsIChannel. I suppose we could rename this to (Get|Set)TriggeringClientInfo() if you think that's clearer. The service worker spec just calls it FetchEvent.clientId, which is why I used the simpler name here. Note, there is no code landed yet that will populate or consume these values. That will come in my next few patches: a. A ClientChannelHelper class that will handle the special redirect logic needed for non-subresource loads. b. WorkerPrivate and ScriptLoader code to set the reserved ClientInfo and to add the ClientChannelHelper for its main script load. c. nsDocShell code that will set its various types of clients and add the ClientChannelHelper for document loads.
Attachment #8928309 - Flags: review?(valentin.gosu)
Attachment #8928309 - Flags: review?(amarchesini)
Attachment #8928309 - Flags: review?(amarchesini) → review+
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8928354 - Attachment is obsolete: true
Comment on attachment 8928615 [details] [diff] [review] P2 Track the client window or worker's event target on ClientSource. r=baku This patch captures the window or worker's nsIEventTarget and stores it on the ClientSource. We will need this to do execute async operations on the ClientSource correctly.
Attachment #8928615 - Flags: review?(amarchesini)
Comment on attachment 8928647 [details] [diff] [review] P3 Add a helper to handle Client objects properly on nsIChannels. r=baku This provides a helper for setting up client information on a non-subresource nsIChannel. We have to be very careful that client UUIDs are not allowed to cross origins since ultimately they will be used to allow postMessage() later. This helper checks for cross-origin redirects and creates a new reserved ClientSource if necessary. Note, this code only supports creating Window ClientSource objects. This is for a couple reasons: 1. Worker ClientSource objects are owned by the worker thread and cannot be created on the main thread. 2. Worker scripts are required to be same-origin and other security checks block cross-origin redirects for worker non-subresource nsIChannels.
Attachment #8928647 - Flags: review?(amarchesini)
Comment on attachment 8928778 [details] [diff] [review] P4 Create a ClientSource for the WorkerPrivate and mark it execution ready after the script loads. r=baku Andrea, this patch does the following: 1. Makes WorkerPrivate create a ClientSource before performing its script load. 2. Passes the ClientInfo for that ClientSource on its main script load as the reserved client. 3. Adds the ClientChannelHelper to the script load to maintain client information properly on same-origin redirects. 4. Sets the WorkerPrivate ClientSource execution ready after the script loads successfully. Note, this is the first patch that will cause ClientSource objects to be actively created and registered in the service. I considered waiting to do this at the end, but I have a couple reasons to do it now: 1. Landing this integration lets me unblock your worker refactor patches sitting in my review queue. 2. It gets some of this code covered by our test suite. I have already had one bug filed about no code coverage in dom/clients in the tree. 3. Its better to slowly introduce the platform changes over time to catch any perf issues early.
Attachment #8928778 - Flags: review?(amarchesini)
Oh, I forgot to mention, the patch in comment 257 also adds a new ClientType for service worker globals. I included the spec issue. I haven't heard any major objections from the other spec participants. I want to add this so we are consistent in creating ClientSource for all globals and labeling them with an appropriate type.
Attachment #8928615 - Flags: review?(amarchesini) → review+
Comment on attachment 8928309 [details] [diff] [review] P1 Allow the reserved/initial/source client data and service worker to be marked on LoadInfo. r=baku r=valentin Review of attachment 8928309 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: netwerk/base/nsILoadInfo.idl @@ +851,5 @@ > + * Note which client (i.e. global) initiated this network request. All > + * nsGlobalWindow and WorkerPrivate can be converted to a ClientInfo to > + * be set here. While this is being added to support service worker > + * FetchEvent, it can also be used to communicate other information about > + * the source global context in the future. The comment is OK, but I suspect it will become obsolete if we start adding other uses; and will probably forget to update it :)
Attachment #8928309 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #259) > The comment is OK, but I suspect it will become obsolete if we start adding > other uses; and will probably forget to update it :) I'd like to leave it for now. I think I will be involved with any future build out of this feature and will make a note to remove it if necessary.
Rebase.
Attachment #8928615 - Attachment is obsolete: true
Attachment #8929077 - Flags: review+
Attached patch wip (obsolete) (deleted) — Splinter Review
Rebase around nsGlobalWindowInner changes.
Attachment #8928800 - Attachment is obsolete: true
Depends on: 1418007
Attachment #8928309 - Attachment is obsolete: true
Attachment #8929077 - Attachment is obsolete: true
Try just up to P5, but without any of the remaining patches. I want to double check this is safe by itself. https://treeherder.mozilla.org/#/jobs?repo=try&revision=339c19d829f74244a838b90224da4119514ec3f1
Comment on attachment 8929199 [details] [diff] [review] P5 Handle creating and activating the ClientSource in nsDocShell and nsGlobalWindow. r=baku r=freesamael Andrea, Samael, This patch integrated ClientSource and ClientInfo into the main thread window globals. Samael, Olli suggested you would be a good person to double-check the docshell parts of this. Samael, let me try to provide some context for your benefit since this bug is very long. The overall goal here is to track all the globals, both workers and windows, across all the processes in the browsers. This is to both accurately implement the Clients DOM API, but also to provide a basis for other internal features that might need this information. To that end I've introduced a ClientSource object that the window and worker global own. This is a live actor object that auto-registers itself in a parent side service table. When the ClientSource is destroyed it removes itself from this table. Due to being an IPC actor a ClientSource is limited to the thread it was created on. To support cross-thread operations there is also an inert structure called ClientInfo that can be used to track back to the ClientSource it represents. With that context out of the way, let me try to describe what this patch does. The overall goal is to create the ClientSource *before* the nsIChannel load for the document and then pass ownership to the final nsGlobalWindowInner that results from that nsIChannel. We need the ClientSource before the nsIChannel load in order to properly populate things spec'd on the service worker FetchEvent. There are a few ways this will happen depending on the type of load. Lets consider a normal top level window load: 1. docshell creates the navigation nsIChannel. 2. docshell creates a "initial" ClientSource to represent the initial about:blank document in the window. The spec requires this to be created even if we optimize away the creation of the about:blank document/window. 3. docshell adds the ClientChannelHelper to the nsIChannel. This will set the initial ClientInfo on the nsIChannel LoadInfo, transfer it during same-origin redirects, and create a new ClientSource for cross-origin redirects. 4. After the nsIChannel completes the nsGlobalWindowInner will try to take any ClientSource created on the LoadInfo or the initial ClientSource from the docshell. 5. The nsGlobalWindowOuter will then mark the inner window and its ClientSource "execution ready" with the final load URL. (I feel like there may be some cases where we don't want to call MaybeCreateInitialClientSource(), but right not it seems like we should always call it per the spec. I filed a spec issue to clarify this: https://github.com/w3c/ServiceWorker/issues/1228 For now I'd like to proceed with this patch, though.) An alternative case to the steps above is when we actually create the inital about:blank document/window because something called GetDocument(). Then we do this: 1. Create the initial ClientSource in CreateAboutBlankContentViewer. 2. When CreateAboutBlankContentViewer create the nsGlobalWindowInner for the about:blank document it steals the initial ClientSource. 3. Eventually the docshell creates the navigation nsIChannel 4. docshell adds ClientChannelHelper 5. Resulting nsGlobalWindoInner either has the existing initial about:blank or the reserved ClientSource from LoadInfo in a cross-origin redirect occurred. (It should be a different nsGlobalWindowInner as well.) 6. nsGLobalWindowOuter marks the final inner window and its ClientSource "execution ready". There are some cases where I punted on creating the initial about:blank ClientSource. Mainly these were cases where docshell does not easily know the principal before the load. These are corner cases and can be added later. Its safer not to get the initial about:blank source created then it would be to use the wrong principal.
Attachment #8929199 - Flags: review?(sawang)
Attachment #8929199 - Flags: review?(amarchesini)
(In reply to Ben Kelly [:bkelly] from comment #267) > Comment on attachment 8929199 [details] [diff] [review] > P5 Handle creating and activating the ClientSource in nsDocShell and > nsGlobalWindow. r=baku r=freesamael > (I feel like there may be some cases where we don't want to call > MaybeCreateInitialClientSource(), but right not it seems like we should > always call it per the spec. I filed a spec issue to clarify this: > > https://github.com/w3c/ServiceWorker/issues/1228 > > For now I'd like to proceed with this patch, though.) I got feedback on my spec issue already. In theory the initial about:blank ClientSource should only inherit the principal from the parent or opener. For top level navigations it should get an opaque origin which makes it non-observable. This is actually what the code does here because in DoChannelLoad() I don't pass an explicit principal to MaybeCreateInitialClientSource(). So we try to inherit the principal and simply avoid creating the ClientSource if we can't find one. So I believe the code in the patch up for review is correct. Sorry for my confusion. I find all the cases here kind of complex and hard to keep straight at times. I'll take an action to add a comment describing all of this in DoChannelLoad(). Clearly I'm going to forget how all this works again in a couple weeks...
Blocks: 1418489
Comment on attachment 8929636 [details] [diff] [review] P6 Allow ClientSource objects to be frozen while in bfcache. r=baku Andrea, this patch lets us mark ClientSource objects as frozen while their owning window/worker is frozen in the bfcache. We keep the ClientSource in place while in the bfcache so things like the Client ID don't change, etc. Note, there is an open action item for me to evict window/worker objects from the bfcache if their absence is observed via the clients API. I had originally planned to do that here, but I punted it to bug 1418489.
Attachment #8929636 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8929203 - Attachment is obsolete: true
Andrea, this patch makes us set the source or triggering ClientInfo on the LoadInfo for all network requests owned by a document. Doing this properly for workers will come later. We don't currently report FetchEvent.clientId in today's code either.
Attachment #8930212 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8929640 - Attachment is obsolete: true
Comment on attachment 8929199 [details] [diff] [review] P5 Handle creating and activating the ClientSource in nsDocShell and nsGlobalWindow. r=baku r=freesamael Review of attachment 8929199 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the very detailed explanation, Ben! Good for me to understand a bit about how service worker works in e10s-multi. The patch looks good and well-documented. Thanks!
Attachment #8929199 - Flags: review?(sawang) → review+
Comment on attachment 8928647 [details] [diff] [review] P3 Add a helper to handle Client objects properly on nsIChannels. r=baku Review of attachment 8928647 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientChannelHelper.cpp @@ +31,5 @@ > +{ > + nsCOMPtr<nsIInterfaceRequestor> mOuter; > + nsCOMPtr<nsISerialEventTarget> mEventTarget; > + > + ~ClientChannelHelper() = default; @@ +36,5 @@ > + { > + } > + > + NS_IMETHOD > + GetInterface(const nsIID & aIID, void **aResultOut) override & and ** should be near the type. @@ +52,5 @@ > + return NS_ERROR_NO_INTERFACE; > + } > + > + NS_IMETHOD > + AsyncOnChannelRedirect(nsIChannel *aOldChannel, * next to the type. here and everywhere in this file. @@ +90,5 @@ > + oldLoadInfo->GetReservedClientInfo(); > + if (reservedClientInfo.isSome()) { > + newLoadInfo->SetReservedClientInfo(reservedClientInfo.ref()); > + } else { > + const Maybe<ClientInfo>& initialClientInfo = why this if/else? It should work also if you do: const Maybe<ClientInfo>& reservedClientInfo = oldLoadInfo->GetReservedClientInfo(); if (reservedClientInfo.isSome()) { newLoadInfo->SetReservedClientInfo(reservedClientInfo.ref()); } const Maybe<ClientInfo>& initialClientInfo = ... if (initialClientInfo.isSome()) { newLoadInfo->Set... } @@ +109,5 @@ > + } > + } > + } > + > + // If its a cross-origin redirect then we discard the old reserved client it's ? @@ +175,5 @@ > + nsresult rv = ssm->GetChannelResultPrincipal(aChannel, getter_AddRefs(channelPrincipal)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + Maybe<ClientInfo> initialClientInfo(Move(aInitialClientInfo)); > + Maybe<ClientInfo> reservedClientInfo(Move(aReservedClientInfo)); I would move these at line 167 before any other operation, but I don't think we leak data if we keep them here.
Attachment #8928647 - Flags: review?(amarchesini) → review+
Comment on attachment 8928778 [details] [diff] [review] P4 Create a ClientSource for the WorkerPrivate and mark it execution ready after the script loads. r=baku Review of attachment 8928778 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ScriptLoader.cpp @@ +1979,5 @@ > } > return true; > } > > + // If this is a top level scropt that succeeded, then mark the script
Attachment #8928778 - Flags: review?(amarchesini) → review+
Attachment #8929199 - Flags: review?(amarchesini) → review+
Attachment #8929636 - Flags: review?(amarchesini) → review+
Attachment #8930212 - Flags: review?(amarchesini) → review+
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8930213 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8930538 - Attachment is obsolete: true
Andrea, this patch adds the ability to call ClientHandle::Control(swd) to mark a ClientSource object controlled by a service worker. This also marks the controller in the ClientSourceParent which will enable the service to distinguish controlled vs non-controlled clients. The patch also lets code local to the ClientSource directly mark it controlled via a SetController() method. This is an optimization and timing fix to handle some cases where we know at ClientSource process that we're controlled before the async ClientHandle operation completes. Nothing calls this yet, but it will be used in this way: 1. The main use will be for ServiceWorkerManager to call ClientHandle::Control() from the parent process. For example, both from when the SWM intercepts the initial non-subresource network load and for things like Clients.claim(). 2. In the case of the initial non-subresource network load the ClientSource::SetController() will also be called directly after the channel completes. This is necessary to properly support synchronous access to navigator.serviceWorker.controller during first script evaluation. If the async ClientHandle::Control() operation finishes first, then this is a no-op. Future patches in this bug will make ClientSource::SetController() fire a controllerchange event.
Attachment #8930554 - Attachment is obsolete: true
Attachment #8930560 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8930555 - Attachment is obsolete: true
Attachment #8930614 - Flags: review+
Depends on: 1419536
Attachment #8930614 - Attachment is obsolete: true
Attachment #8930615 - Attachment is obsolete: true
Attachment #8929199 - Attachment is obsolete: true
Attachment #8929636 - Attachment is obsolete: true
Attachment #8930212 - Attachment is obsolete: true
Blocks: 1419620
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8930561 - Attachment is obsolete: true
Comment on attachment 8930774 [details] [diff] [review] P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku Andrea, this patch makes the code actually use the ClientHandle::Control() and ClientSource::SetController() methods to mark the client as controlled. There are a few cases that are handled here: 1. The docshell handles setting the initial about:blank client as controlled if its parent is controlled. This is something we have agreed on in the spec, but we have not implemented before. 2. The SWM marks the reserved/initial client and load info as controlled in DispatchFetchEvent. 3. The SWM also marks the client as controlled in StartControllingADocument(). This method will disappear in the future when we refactor SWM to live in the parent, but I do it here for now for parity with legacy code. 4. The inner window marks its own client as controlled if the load info was marked controlled by the SWM. 5. The worker ScriptLoader and WorkerPrivate mark its ClientSource controlled if the load info is marked controlled by the SWM. Note, this patch does not yet handle the Clients.claim() case. That requires some additional plumbing for now since we have to notify the child-side SWM instances in the process with the client. I will break that out as a separate patch. Also note, there is a sync IPC call here. This is needed for worker startup. Consider that we basically need this ordering: 1. WorkerPrivate creates a ClientSource on worker thread 2. ScriptLoader calls ClientHandle::Control on main thread 3. The ClientSource from (1) is created in the parent and attaches to ClientManagerService. 4. The control message from (2) reaches the parent and finds the ClientSourceParent from (3). Of course, this ordering is not enforced in any way by IPC. We can totally get (4) happening before (3). As a simple solution I introduced a "sync ping" message that is only used for dedicated and shared workers. This is called between (1) and (2) above. It ensures that (3) always takes place before (4). It is a small amount of overhead, but it does not touch any main thread so should be relatively cheap. There are of course other solutions, but they all seemed more complex to me. We can investigate replacing this in a follow-up if you wish.
Attachment #8930774 - Flags: review?(amarchesini)
Lets checkpoint the wip patch again. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e578b156cfe246d1b93e45c8798b0267e46b97e Also, the wip patch is less than 200kb now. Hooray!
Tweak P8 to fix a warning failure. Just keeping this locally for now to avoid churning the review flags. https://treeherder.mozilla.org/#/jobs?repo=try&revision=621ba239fd26c6388c8fd08067bc5f30e20754d3
Attached patch wip (obsolete) (deleted) — Splinter Review
I update my initial about:blank WPT test case based on review comments from JakeA. I also added a test case to verify we do something sane for bug 1404468. I think I'm going to close bug 1404468 and make the change to not reuse the inner window here. I'm not sure its really something that can wait for a later bug. The P9 patch should be safe to land, though, since nothing in the system is consuming the controller state from ClientSource yet. Those changes are still in this mega-patch.
Attachment #8930988 - Attachment is obsolete: true
No longer blocks: 1419620
Attached patch wip (obsolete) (deleted) — Splinter Review
Rebase. Also, I merged the about-blank-replacement.https.html WPT test in this patch upstream. I'll remove from the wip patch when it next gets sync'd to m-c.
Attachment #8931152 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
The WPT test merged from upstream so the wip patch shrinks ~10kb. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f316261261d747317d3e21867cad2cafc8ad3360
Attachment #8932631 - Attachment is obsolete: true
Comment on attachment 8933789 [details] [diff] [review] P10 Copy the service worker controller across redirects by default and clear it explicitly for non-subresource redirects. r=baku Andrea, this patch reverses some of the LoadInfo controller logic. Before we required the controller to be explicitly copied across redirects. Now with this patch we will default to copying the controller and non-subresource channels will clear the controller if necessary in ClientChannelHelper. I realized the old logic was flawed because we will need to keep the controller in place once we start using it on subresources channels. These channels won't have ClientChannelHelper to explicitly move the controller around.
Attachment #8933789 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8933059 - Attachment is obsolete: true
Comment on attachment 8933790 [details] [diff] [review] P11 Handle the case where an iframe starts with an inherited controller and then ends up uncontrolled. r=baku Andrea, this implements a short term fix for the case where: 1. An initial about:blank iframe inherits a service worker controller from its parent. 2. And its final document load is not actually covered by any service worker scope. In this case we need to have a controlled initial about:blank and an uncontrolled final window. How we make that happen is a bit up in the air right now. There is a spec issue here: https://github.com/w3c/ServiceWorker/issues/1232 For now I have done the smallest change possible to get the basic sanity requirements above. I simply mint a new ClientSource if we hit this situation. In the long term we probably want to clear the controller on the ClientSource or force docshell to create a separate inner window (like if it was cross-origin load, etc). Those are more work though and its unclear which way the spec issue will go.
Attachment #8933790 - Flags: review?(amarchesini)
Comment on attachment 8933849 [details] [diff] [review] P12 Add a ClientManager::GetInfoAndState() that retrieves the ClientInfo and ClientState for a given ID. r=baku This patch adds a ClientManager::GetInfoAndState() static method that will query the service for a given ID, snapshot the found client's state, and return the values via promise. This is basically what the `Clients.get(id)` DOM API does. We don't need or want a full handle here. Just the static information necessary to return the Client object. If any methods like `Client.focus()`, etc, are called then we will mint a ClientHandle at that point. This patch is similar to the previous ClientHandle::Control() patch. It pipes the operation through to the source. In this case we also pass back the resulting ClientInfoAndState object.
Attachment #8933849 - Flags: review?(amarchesini)
When I went through review on the about-blank-replacement.https.html test case upstream I added some extra cleanup to call popup.close(). This was a bit of a mistake, though, since that cleanup code races with the code that actually tries to inspect the state of the popup window. To fix this lets wait to cleanup until we unload the popup window's opener frame. This fixes the test on non-e10s with this patch queue applied.
Attachment #8934195 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Remove some stale code.
Attachment #8933851 - Attachment is obsolete: true
Attachment #8934218 - Attachment is patch: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8934218 - Attachment is obsolete: true
Depends on: 1422983
Comment on attachment 8930560 [details] [diff] [review] P8 Allow a caller to set a ClientSource controlled by a service worker using a ClientHandle. r=baku Review of attachment 8930560 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientSourceOpChild.cpp @@ +43,5 @@ > + // This may cause the ClientSource object to be destroyed. Do not > + // use the source variable after this call. > + promise = (source->*aMethod)(aArgs); > + } > + promise could be null at this point.
Attachment #8930560 - Flags: review?(amarchesini) → review+
Comment on attachment 8930774 [details] [diff] [review] P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku Review of attachment 8930774 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/ipdl/sync-messages.ini @@ +1059,5 @@ > description = > [PLayerTransaction::ShutdownSync] > description = bug 1363126 > +[PClientSource::WorkerSyncPing] > +description = Synchronous ping allowing worker thread to confirm actor is created. Necessary to avoid racing with ClientHandle actors on main thread. you need a r+ from a IPC peer.
Attachment #8930774 - Flags: review?(amarchesini) → review+
Attachment #8933789 - Flags: review?(amarchesini) → review+
Attachment #8933790 - Flags: review?(amarchesini) → review+
Attachment #8934195 - Flags: review?(amarchesini) → review+
Attachment #8933849 - Flags: review?(amarchesini) → review+
Comment on attachment 8930774 [details] [diff] [review] P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku Jed, can you review this sync IPC message I'm adding? The idea here is basically to guarantee that the content Worker thread actor is created on the parent side before we do some work on the main thread that will expect it to be there. The sync message is a ping that just enforces that the actor is alive before we proceed to the main thread work. The sync message is only used on PBackground off the main thread. There are assertions to enforce this.
Attachment #8930774 - Flags: review?(jld)
(In reply to Ben Kelly [:bkelly] from comment #315) > Jed, can you review this sync IPC message I'm adding? Note, I think I'm also an IPC peer now, but I'm not in the hg commit hook yet. Filing a separate bug to fix that.
(In reply to Andrea Marchesini [:baku] from comment #313) > ::: dom/clients/manager/ClientSourceOpChild.cpp > @@ +43,5 @@ > > + // This may cause the ClientSource object to be destroyed. Do not > > + // use the source variable after this call. > > + promise = (source->*aMethod)(aArgs); > > + } > > + > > promise could be null at this point. I don't think it can, actually. The source object may be gone, but we hold a strong ref to the promise. Also, all of the methods we call are required to return a promise. If they hit an error they will reject the promise instead of returning nullptr. I can add a MOZ_DIAGNOSTIC_ERROR and a comment to this point.
Depends on: 1423328
Attachment #8934645 - Attachment is obsolete: true
Comment on attachment 8934286 [details] [diff] [review] P14 Implement ClientManager::MatchAll() to support clients.matchAll() WebAPI. r=baku Andrea, this patch implements a query method to support the Clients.matchAll() WebAPI. The query can be initiated using the ClientManager::MatchAll() method. Note, I have a goal to support the Clients API in windows and other workers besides service workers. To that end I have designed this to work with either a ClientInfo source or a ServiceWorkerDescriptor source. I wrap this up into a ClientEndPoint union type. We only support matching controlled clients if the endpoint is a service worker. I could in theory remove the ClientEndPoint stuff for now, but I would like to keep it if you don't mind. It doesn't complicate much and helps prove out that we're not introducing worker-specific design elements. In terms of exposing Clients API in the future on non-service-worker globals, I think there are two ways we may do this: 1. I have an open spec issue to add it to all window/worker globals. 2. We may expose it to chrome script so that they can use the clients API. This may be simpler for some code than using our xpcom interfaces.
Attachment #8934286 - Flags: review?(amarchesini)
Comment on attachment 8934713 [details] [diff] [review] P15 Add ClientManager::Claim() message to matching ClientSource objects. r=baku Andrea, this patch adds a ClientManager::Claim() method which sends a message to all matching ClientSource objects. Currently all this then does is call SetController on the ClientSource, but in a future patch it will also update the ServiceWorkerManager. Long term we are going to move the ServiceWorkerManager to the parent process, so we will eventually remove this ClientClaimArgs message to the ClientSource. Instead we will have the ServiceWorkerManager just use the ClientHandle::Control() method from the parent. For now, though, we have to propagate all the way through to the correct child process. We must update the ServiceWorkerManager in each child process as appropriate. I'm leaving the ServiceWorkerManager integration for a later patch here to avoid breaking any existing behavior. I want to land the rest of the dom/clients/manager code first.
Attachment #8934713 - Flags: review?(amarchesini)
Looks like the work-in-progress patch is down to 150kb. At its peak it was at 390kb. More than half way...
Comment on attachment 8930774 [details] [diff] [review] P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku Review of attachment 8930774 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the sync IPC. If it's off-main-thread and the sending thread would have to block waiting for the response anyway, sync IPC seems harmless.
Attachment #8930774 - Flags: review?(jld) → review+
Depends on: 1423412
Comment on attachment 8930774 [details] [diff] [review] P9 Actually mark window/worker ClientSource objects controlled when loaded with a controlling service worker. r=baku Moved to bug 1423412 for landing.
Attachment #8930774 - Attachment is obsolete: true
Attachment #8933789 - Attachment is obsolete: true
Attachment #8933790 - Attachment is obsolete: true
Attachment #8933849 - Attachment is obsolete: true
Attachment #8934195 - Attachment is obsolete: true
Attachment #8935071 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935072 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Remove some stale changes that we don't need any more.
Attachment #8935115 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935122 - Attachment is obsolete: true
Comment on attachment 8935138 [details] [diff] [review] P16 Add ClientManager::Navigate() method. r=baku Andrea, this patch adds the ClientManager::Navigate() method to support the Client.navigate(url) WebAPI. My initial naive implementation put Navigate on ClientHandle since we already know what Client we want to navigate. This was problematic, though, since navigating a window will destroy the current ClientSource/ClientHandle actors. To deal with the destruction and re-creation of actors I had to put Navigate() on the ClientManager instead. The message goes to the ClientManagerService which looks up the right target ClientSource actor. A separate ClientNavigateOp actor is then used to initiate the navigation using the target client in its process. For the most part the navigation bits are lifted straight from the existing ServiceWorkerWindowClient navigation bits: https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerWindowClient.cpp#386 The only significant change I made was removing this code which focuses the window after navigating it: https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerWindowClient.cpp#268 AFAICT the spec does not do this and there is an interaction requirement in order to focus a window. This focus was bypassing the interaction requirement so I just removed it. A later patch will remove the existing code in ServiceWorkerWindowClient once I switch the bindings over to my new code.
Attachment #8935138 - Flags: review?(amarchesini)
Comment on attachment 8935114 [details] [diff] [review] P17 Implement ClientManager::OpenWindow(). r=baku Andrea, this is another relatively large patch. It adds the ClientManager::OpenWindow() method to support the Clients.openWindow() WebAPI. When I started looking at making Clients.openWindow() work in multi-e10s I asked the e10s team the best way to open a window from any process. In addition, I needed a way that would let me know when the window was done opening. I was somewhat surprised we did not have any existing infrastructure that provided this (seemingly fundamental browser) feature. So... this patch adds a bunch of infrastructure to do this. The first step is a ClientManagerOp type to get us from the child thread/process to the parent process background thread. There we call ClientManagerService::OpenWindow(). From the ClientManagerService we need to do some conditional things depending on if we are in e10s or non-e10s mode. If we're in non-e10s we simply call a new utility method ClientOpenWindowInCurrentProcess(). This returns a promise which we tie back to the ClientManagerOp actor. If we;re in e10s mode, though, we first need to hop to the appropriate content process. This may involve creating a process if necessary. To do this we call ContentParent::GetNewOrUsedBrowserProcess(). We also have a preference which lets us select our source process if available. This is a quirk to make things work better before we complete the multi-e10s code. Some service worker tests expect to see the FetchEvent for the window they open. Currently this means we need to open in the same process if possible. Once multi-e10s refactoring is complete we can remove this preference. Once we have our target content process we create a new PContent ClientOpenWindowOp actor. The resulting child actor then calls the same ClientOpenWindowInCurrentProcess() utility method. The code in ClientOpenWindowInCurrentProcess() is basically directly lifted from the existing clients code: https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerClients.cpp#510 Its quite verbose and a bit gnarly, though, so I isolated it off in a separate ClientOpenWindowUtils.(h|cpp) module.
Attachment #8935114 - Flags: review?(amarchesini)
Comment on attachment 8935135 [details] [diff] [review] P18 Implement ClientHandle::Focus(). r=baku Andrea, this implements the ClientHandle::Focus() method to support the Client.focus() WebAPI. It basically just pipes the operation to the ClientSource, fires the chrome focus event, and then takes a snapshot of the ClientState to return. The spec requires interaction to use this, but I will enforce that in the API layer when I implement the DOM object.
Attachment #8935135 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935136 - Attachment is obsolete: true
(In reply to Ben Kelly [:bkelly] from comment #319) > Comment on attachment 8934286 [details] [diff] [review] > P14 Implement ClientManager::MatchAll() to support clients.matchAll() > WebAPI. r=baku > Note, I have a goal to support the Clients API in windows and other workers > besides service workers. To that end I have designed this to work with > either a ClientInfo source or a ServiceWorkerDescriptor source. I wrap this > up into a ClientEndPoint union type. We only support matching controlled > clients if the endpoint is a service worker. > > I could in theory remove the ClientEndPoint stuff for now, but I would like > to keep it if you don't mind. It doesn't complicate much and helps prove > out that we're not introducing worker-specific design elements. After further consideration, I think I'm going to remove ClientEndPoint now. I'm going to do this in a follow-on patch, though, to avoid forcing a rebase of all these patches.
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935474 - Attachment is obsolete: true
Attachment #8935495 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935496 - Attachment is obsolete: true
Comment on attachment 8935526 [details] [diff] [review] P19 Implement ClientHandle::PostMessage() r=baku Andrea, this patch adds the ClientHandle::PostMessage() method to support the Client.postMessage() WebAPI. Currently the spec only defines Client.postMessage() as being called from a ServiceWorker thread. The resulting MessageEvent is dispatched at the client's self.navigator.serviceWorker event target. The MessageEvent.source is a ServiceWorker object. In the future I plan to add support for the MessageEvent.source to be a Client and for Client.postMessage() to be called from any client thread. When that happens we would fire the MessageEvent at the self.clients event target. For now, though, this is not implemented yet.
Attachment #8935526 - Flags: review?(amarchesini)
Comment on attachment 8935539 [details] [diff] [review] P20 Remove the ClientEndPoint union type for now. r=baku This removes the ClientEndPoint union type I introduced in P14. I still want to support the ClientHandle and ClientManager operations on all window/worker threads, but lets do that in a follow-up bug.
Attachment #8935539 - Flags: review?(amarchesini)
Attachment #8934286 - Flags: review?(amarchesini) → review+
Attachment #8934713 - Flags: review?(amarchesini) → review+
Comment on attachment 8935114 [details] [diff] [review] P17 Implement ClientManager::OpenWindow(). r=baku Review of attachment 8935114 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/manager/ClientManagerService.cpp @@ +584,5 @@ > + > + // Possibly try to open the window in the same process that called > + // openWindow(). This is a temporary compat setting until the > + // multi-e10s service worker refactor is complete. > + if (Preferences::GetBool("dom.clients.openwindow_favors_same_process", would be nice to cache this.
Attachment #8935114 - Flags: review?(amarchesini) → review+
Attachment #8935135 - Flags: review?(amarchesini) → review+
Attachment #8935526 - Flags: review?(amarchesini) → review+
Attachment #8935539 - Flags: review?(amarchesini) → review+
Attachment #8935138 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #349) > ::: dom/clients/manager/ClientManagerService.cpp > @@ +584,5 @@ > > + > > + // Possibly try to open the window in the same process that called > > + // openWindow(). This is a temporary compat setting until the > > + // multi-e10s service worker refactor is complete. > > + if (Preferences::GetBool("dom.clients.openwindow_favors_same_process", > > would be nice to cache this. The Clients.openWindow() call is not very hot. It requires user interaction to trigger. Also, I hope to remove this relatively soon. So I'd prefer not to add extra bits to cache for now if thats ok.
Depends on: 1424338
Attachment #8934286 - Attachment is obsolete: true
Attachment #8934713 - Attachment is obsolete: true
Attachment #8935114 - Attachment is obsolete: true
Attachment #8935135 - Attachment is obsolete: true
Attachment #8935138 - Attachment is obsolete: true
Attachment #8935526 - Attachment is obsolete: true
Attachment #8935539 - Attachment is obsolete: true
Attached patch storage-denied.patch (obsolete) (deleted) — Splinter Review
Thanks to a test that Tom added I realized my wip patch regressed some StorageAccess checks I added for Clients.get() and Clients.matchAll(). This patch adds those back. Let's see if it fixes the test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee20b1ecc87d03fccf1e36b8dfb40fefef10fa7e
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935551 - Attachment is obsolete: true
Comment on attachment 8935982 [details] [diff] [review] P1 Capture StorageAccess when snapshoting ClientSource state. r=baku A month ago or so I added some checking in Clients.get() and Clients.matchAll() to avoid exposing Client objects where the target global is not allowed to access storage. The goal is to prevent communication between these restricted globals and the service worker which always has access to storage. Tom then added some console reporting to make this block easier to understand. He also added a test for the console reporting. This test shows that my patches here regress the storage access blocking in Clients.get() and Clients.matchAll(). To fix the regression this patch captures the StorageAccess enumeration value when we snapshot the ClientState. Arguably I could have put this on ClientInfo, but I chose not to for two reasons: 1. In theory the pref can change, so the StorageAccess state can change while a window is still open. 2. An initial about:blank window may have StorageAccess, but then lose it after the real URL loads. Since the value seems like it can sometimes change I chose to put it in ClientState. This also keeps ClientInfo smaller for now. If necessary we can move it to ClientInfo in the future. Note, I want to implement the actual block in the Clients API DOM layer and not in the infrastructure here. In theory I want other parts of the system to be able to use this code, so I don't want to bake this restriction into the infrastructure bits.
Attachment #8935982 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935983 - Attachment is obsolete: true
Comment on attachment 8935986 [details] [diff] [review] P2 Cleanup test_openWindow.html mochitest service worker a bit. r=baku While investigating other failures I noticed this test doesn't use event.waitUntil() like it should. This patch fixes that and some whitespace issues.
Attachment #8935986 - Flags: review?(amarchesini)
Comment on attachment 8935987 [details] [diff] [review] P3 Remove the dom.serviceWorkers.openWindow.enabled pref. r=baku This patch removes the pref controlling if Clients.openWindow() is enabled. We shipped this a long time ago and don't need the ability to turn it off individually any more. Rather than make the new DOM object work with this I'd rather just remove the pref.
Attachment #8935987 - Flags: review?(amarchesini)
Next step is to clean up the new Client/Clients DOM classes and then land them. Cleanup mainly consists of removing my stale code that tried to support windows. The code can be simpler for now since we are only supporting these on service worker globals at first. After that, I will like have a series of patches that must land in a single push that swaps the implementations. And finally, some patches to remove stale code. Assuming reviews are possible during Austin I hope to have this bug resolved next week. (I don't think the reviews will be that complicated or hard FWIW.)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8935988 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8936167 - Attachment is obsolete: true
Comment on attachment 8936169 [details] [diff] [review] P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku Andrea, this adds the DOM Client and Clients objects. It does not hook them into the bindings yet.
Attachment #8936169 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8936170 - Attachment is obsolete: true
Comment on attachment 8936173 [details] [diff] [review] P5 Switch bindings over to new Client and Clients classes. r=baku Andrea, this patch switches the bindings over to the new Client and Clients classes. NOTE, I will not land this until the rest of the wip patch is split up and reviewed. This patch needs to land in the same push as later code that will: 1. Fix the FetchEvent.clientId to use the new ClientInfo.Id() value. 2. Fire the ControllerChange event from ClientSource::SetController(). 3. Call into ServiceWorkerManager from ClientSource::Claim(). 4. Test expectations are updated for new things passing. This patch, however, updates the bindings, webidl files, and removes references to the old classes.
Attachment #8936173 - Flags: review?(amarchesini)
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8936174 - Attachment is obsolete: true
Sorry, the last patch had some build issues that didn't show locally for some reason. Dropping the flag for now till I can get a green try. I can't land this without splitting more patches anyway. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae712c73c63018bf0acf33f59916fe167291e9de
Attachment #8936173 - Attachment is obsolete: true
Attachment #8936173 - Flags: review?(amarchesini)
Attachment #8936177 - Attachment is obsolete: true
Attached patch wip (obsolete) (deleted) — Splinter Review
Attachment #8936175 - Attachment is obsolete: true
Attachment #8936269 - Attachment is obsolete: true
Comment on attachment 8936304 [details] [diff] [review] P5 Switch bindings over to new Client and Clients classes. r=baku Andrea, this patch updates the bindings and webidl files to use the new Client and Clients classes. It also removes the old class files and orphaned code in ServiceWorkerManager.
Attachment #8936304 - Flags: review?(amarchesini)
Comment on attachment 8936305 [details] [diff] [review] P6 Use the ClientInfo.Id() value for FetchEvent.clientId. r=baku Andrea, this patch makes us you the LoadInfo's ClientInfo ID for FetchEvent.clientId. This is in place of the old nsIDocument's ID value. Note, we only expose FetchEvent.clientId here for subresource requests. Some sites are using it to infer if a request is subresource or non-subresource. Until we fully ship the "resultingClientId" stuff we need to keep it null for non-subresource. This patch also removes all the machinery for the nsIDocument ID, etc. Notably, we no longer need to register every nsIDocument in the observer service. I also have one small extra change in here to stop passing the scope for the ServiceWorker's WorkerPrivate name. Technically I should have included that with the patches in bug 1412858. I'm including it here since it was left over in this file in my wip patch.
Attachment #8936305 - Flags: review?(amarchesini)
Comment on attachment 8936306 [details] [diff] [review] P7 Make ServiceWorkerManager use ClientHandle::Control() for claiming and ClientSource::SetController() fire the controller change event. r=baku Andrea, this patch integrates the ServiceWorkerManager "claim" support with ClientHandle::Control(). It also moves the dispatch of the controllerchange event to ClientSource::SetController(). Eventually we won't have to call into ServiceWorkerManager from ClientSource at all once its been migrated to the parent process. For now, though, we must keep calling into it from ClientSource::Claim(). Once SWM is moved to the parent we can replace the ClientSource::Claim() stuff with a direct Control() message from the ClientManagerService::Claim() method.
Attachment #8936306 - Flags: review?(amarchesini)
Comment on attachment 8936307 [details] [diff] [review] P8 Update WPT test expectations for re-written Clients API Update WPT test expectations. We now handle about:blank replacement correctly and also can observe SharedWorker/Worker clients via matchAll().
Attachment #8936307 - Flags: review?(amarchesini)
Attachment #8935982 - Flags: review?(amarchesini) → review+
Attachment #8935986 - Flags: review?(amarchesini) → review+
Attachment #8935987 - Flags: review?(amarchesini) → review+
Comment on attachment 8936169 [details] [diff] [review] P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku Review of attachment 8936169 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/clients/api/Client.cpp @@ +33,5 @@ > + NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > +Client::~Client() = default ::: dom/clients/api/Clients.cpp @@ +34,5 @@ > + NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > +Clients::~Clients() = default? But I guess it's going to change in the next patches.
Attachment #8936169 - Flags: review?(amarchesini) → review+
Comment on attachment 8936304 [details] [diff] [review] P5 Switch bindings over to new Client and Clients classes. r=baku Review of attachment 8936304 [details] [diff] [review]: ----------------------------------------------------------------- cool! ::: dom/bindings/Bindings.conf @@ +149,5 @@ > 'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate', > }, > > 'Client': { > + 'nativeType': 'mozilla::dom::Client', why do you need this? Get rid of the full block. mozilla::dom::Client is by default the nativetype. @@ +154,4 @@ > }, > > 'Clients': { > + 'nativeType': 'mozilla::dom::Clients', same here.
Attachment #8936304 - Flags: review?(amarchesini) → review+
Attachment #8936305 - Flags: review?(amarchesini) → review+
Attachment #8936306 - Flags: review?(amarchesini) → review+
Attachment #8936307 - Flags: review?(amarchesini) → review+
Thanks for the reviews! From my last try push I have one new persistent test failure: devtools/client/aboutdebugging/test/browser_service_workers_push_service.js It only fails on non-e10s. I'm investigating. Hopefully its just something I accidentally dropped when shuffling patches.
I spoke with Ryan on IRC and he gave me r+ to disable this test for now. It only fails on non-e10s. AFAIK we don't ship about:debugging in non-e10s any more. Also, it seems to be some timing dependent problem in the devtools related to push. I don't think my patches here should impact this code outside of timing.
Attachment #8936430 - Flags: review+
Last try had a single orange in a new devtools test. I couldn't reproduce locally or in retriggers. I filed bug 1424914 for now.
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3fb8bd8b10d P1 Capture StorageAccess when snapshoting ClientSource state. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/3f23acc15006 P2 Cleanup test_openWindow.html mochitest service worker a bit. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/6d09b4467c62 P3 Remove the dom.serviceWorkers.openWindow.enabled pref. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b0cd514d46 P4 Add Client and Clients DOM classes, but don't hook them into bindings yet. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/63938d2d5492 P5 Switch bindings over to new Client and Clients classes. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/baa02d97591e P6 Use the ClientInfo.Id() value for FetchEvent.clientId. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2388a414a4 P7 Make ServiceWorkerManager use ClientHandle::Control() for claiming and ClientSource::SetController() fire the controller change event. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/93b27fc24c01 P8 Update WPT test expectations for re-written Clients API. https://hg.mozilla.org/integration/mozilla-inbound/rev/ec647e220016 P9 Disable browser_service_workers_push_service.js on non-e10s due to races. r=jryans
I see an orange in the inbound push on Linux64 opt tc-M-e10s(4): https://treeherder.mozilla.org/logviewer.html#?job_id=151339987&repo=mozilla-inbound&lineNumber=4289 I'm doing some re-triggers to see how intermittent this is. I didn't see it in my try pushes at all.
Depends on: 1424943
The intermittent in comment 395 has about a 20% retrigger rate it seems. I'll investigate first thing tomorrow in bug 1424943.
Depends on: 1425614
Depends on: 1425704
Depends on: 1425175
Adding a release note for 59 beta/dev ed on Ben Kelly's suggestion: "Service worker Clients API can now find and communicate with windows in a separate browser process"
On further discussion, sounds like we should add this to MDN instead, to https://developer.mozilla.org/en-US/Firefox/Releases/59
I added a note for this and two other issues under the "Service workers" heading on the MDN page mentioned in comment 399.
Blocks: 1432640
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: