Client objects are linked to outer windows which leads to all sorts of trouble such as: 1. service worker creates a client (using matchAll). 2. the corresponding window navigates to google.com 3. client.postMessage will now dispatch messages to the google.com window.

Comment on attachment 8599308 [details] [diff] [review] ServiceWorkerClient: use innerWindow id for referencing clients. Review of attachment 8599308 [details] [diff] [review]: ----------------------------------------------------------------- Please pass this through bz once unless it was done after talking with him.

Comment on attachment 8599308 [details] [diff] [review] ServiceWorkerClient: use innerWindow id for referencing clients. Hey bz, are you okay with this approach? A client object will hold a innerWindow id which will be used for dispatching postMessage and focus calls. There is a corner case that I should point out. When intercepting fetch events generated by navigation (i.e. <a href>.click()), the document that originated the event becomes detached and has no inner window. At this point we cannot construct a valid client object for this document, but I don't think we should.

> A client object will hold a innerWindow id which will be used for dispatching postMessage > and focus calls. Is it expected that the client object doesn't keep the window alive? If so, then this sounds like the right approach. If not, just hold a strong ref to it.... > When intercepting fetch events generated by navigation (i.e. <a href>.click()), the > document that originated the event becomes detached and has no inner window. Hmm. It should have an inner window until the new page is actually loading. At what point is the client object being constructed, exactly?

(In reply to Not doing reviews right now from comment #5) > > A client object will hold a innerWindow id which will be used for dispatching postMessage > > and focus calls. > > Is it expected that the client object doesn't keep the window alive? No, the client shouldn't not keep the window alive. The id is a 64-bit integer which can be used to get the window reference when needed using nsGlobalWindow::GetInnerWindowWithId. > > When intercepting fetch events generated by navigation (i.e. <a href>.click()), the > > document that originated the event becomes detached and has no inner window. > > Hmm. It should have an inner window until the new page is actually loading. > At what point is the client object being constructed, exactly? When calling matchAll inside the fetch handler. This is the example I encountered the issue with: window.onload = function() { document.querySelector("a").click(); }; <a ping="ping" href="fetch.txt">link</a> // worker onfetch = function(e) { matchAll(); } When intercepting the fetch for ping and calling matchAll(), the initial document is detached and doesn't have a inner window.

Hmm. That's a bit odd. Ping loads are kicked off immediately after the fetch.txt load in that case. Is there a significant delay between those loads starting and the interception code running, such that the server has time to respond for the fetch.txt? Or is it that the "server" in this case is the worker so it responds immediately, before it has looked at the pings at all?

(In reply to Not doing reviews right now from comment #7) > Hmm. That's a bit odd. Ping loads are kicked off immediately after the > fetch.txt load in that case. Is there a significant delay between those > loads starting and the interception code running, such that the server has > time to respond for the fetch.txt? > > Or is it that the "server" in this case is the worker so it responds > immediately, before it has looked at the pings at all? Yes, the service worker responds to both requests, with fetch.txt being the first one. Delaying the request for fetch.txt with waitUntil will avoid the situation with the detached document.

OK, I see. So the pings really are being processed after the document has unloaded. What behavior do we want in that case in terms of this service worker code?

There's no spec directive for this, but I'd say we want to create the client objects with invalid ids. Both .postMessage and .focus will fail, but the other client attributes might be useful for the user. nsm?

If the document has unloaded, it should have stopped being controlled and there really isn't a 'client' to speak of right? Is it still in the set of controlled documents or possibly controlled documents? This seems relevant https://github.com/slightlyoff/ServiceWorker/issues/626 But that discussion seems to have gotten way complicated. I like comment 10 here.

Landing this, we can follow up based on issue 626 as required.