See this issue and the tests in this comment: https://github.com/w3c/ServiceWorker/issues/922#issuecomment-239036842

The web platform tests for this have finally landed in https://github.com/w3c/web-platform-tests/commit/688f5f85cbfa5c56326551753ba86735dcf60393. I also filed a Chrome issue: https://bugs.chromium.org/p/chromium/issues/detail?id=691008

Chrome has made this change in 58 which is currently their beta. So in 6 weeks we will have a compat problem. We should fix this ASAP. I think we mainly need to get these tests passing: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/service-workers/service-worker/multi-globals/url-parsing.https.html.ini

Ben, are you going to work on this? Or we should get someone else to help?

I don't have time to work on this. So, yes it would be great to have someone take this bug.

HoPang, we need your help by taking this bug. Thank you!

Sure thing

After digging into the related threads and the implementation, I think the right way to fix this is to get the correct document (relevant.https.html in the testcase) here[0]. I am currently trying to obtain `this` passed into the `call()` and then use it to find the desired document if there is any. [0] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerContainer.cpp#147-160

MAIN --- Incumbent --- Current +- Relevant There are several global objects in the this testcase as shown by the graph above. I think the main problem is to get the correct global object when executing "register" in the following snippet[0]. current.navigator.serviceWorker.register.call(relevant.navigator.serviceWorker, 'test-sw.js', options); Though I've tried using existing utilities such as those in ScriptSettings[1] and JSAPI[2], I ended up with making a JSAPI for WebAPI implementation to get the passed "this". [0] http://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/multi-globals/incumbent/incumbent.https.html#14 [1] http://searchfox.org/mozilla-central/source/dom/base/ScriptSettings.cpp#32 [2] http://searchfox.org/mozilla-central/source/js/src/jsapi.h

Hi Bobby, To solve this issue, I ended up with adding a new JSAPI after wrestling with SpiderMonkey a bit, since I failed to find an existing applicable JSAPI. The key point here is to use the correct global object during the execution of ServiceWorkerContainer.Register(...), so the prototype saves the "this" to the JSContext before executing the native implementation of an WebAPI and cleans the "this" after the execution. Then, the newly created JSAPI, "RelevantGlobalOrNull()" tries to get the saved "this" from the JSContext passed in, and return its UncheckedUnwrap'ed global. However, if the JSAPI fails to get an applicable "this", it will fall to "CurrentGlobalOrNull". So, do you mind telling me whether there already an JSAPI can do this job, or how do you think about this JSAPI if there is no existing JSAPI can do thit work?

I've kinda paged this stuff out for the time being, deferring to bz.

This shouldn't need new API. And the proposed API implementation is definitely not OK, because it really doesn't handle reentry. Anyway, the relevant global of any DOM object is simply the global of its JS reflector. You can get this in various ways. In the special case of a DOMEventTargetHelper subclass, as here, just call GetOwnerGlobal(). Note that this can return null if things are torn-down enough; specifically if DOMEventTargetHelper::DisconnectFromOwner got called. It can also return null if the global has gone away, but that can't really happen while the DOMEventTargetHelper's reflector is alive and you're accessible from JS. But you _do_ have to worry about the torn-down state; I hope the spec defines what happens in that situation. If not, file spec bugs.

And if you really want the window, there's GetOwner() and GetWindowIfCurrent(), depending on the semantics you want. Again, both can return null, whether because you're in a non-window context, or because the window is torn down, or because it's not current in the GetWindowIfCurrent() case. Again, ideally the spec describes how to handle all that.

Comment on attachment 8865341 [details] [diff] [review] P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers A few comments: 1) Does the spec restrict to "active document" here in some way? That is, if you call register() on a ServiceWorkerContainer of an inactive document and pass a relative URI, does the spec say to throw a TypeError? Because that's what your code ends up doing. If the spec _does_ say to do this, this needs a web platform test. If it doesn't, you should probably not implement non-spec-compliant behavior. 2) The comment in the "else" clause is referring to entry documents; it should probably be modified.

Hi Boris, and thanks for caring, Since I have some doubt in some questions such as whether we should only use the active InnerWindow and the ones you mentioned, I am still writing a comment for NI other people for more advice due to lacking knowledge of what is actually desired in the spec or even future spec :P

Setting dev-doc-needed so I can investigate whether this requires any docs changes once it is further along.

Thank Boris's comments again, I looked more for this issue. Now the window is always acquired from `DOMEventTargetHelper::GetOwner()`. Via doing this, some of the testcases are solved as well. Because the iframes loaded in those testcases doesn't have a valid document, so they fail registering serviceworkers. However, I do have a question about the P1 patch. Since we're trying to get the URI via `window->GetDocBaseURI()`[0], which returns the the BaseURI of the document or the cached BasedURI when the document is removed from the window. It's quite strange to me that we can still get the GetDocBaseURI when there is no valid document. I think it might be caused by something like that a document is created first and removed later when the parser find out there is document at all. At this moment, I am still finding some evidences supporting this. [0] http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4137-4142 [1] http://searchfox.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#639

> Now the window is always acquired from `DOMEventTargetHelper::GetOwner()` On which window? > Because the iframes loaded in those testcases doesn't have a valid document Um... iframes always have a valid document, as long as they're in the DOM tree. > It's quite strange to me that we can still get the GetDocBaseURI when there is no valid document. No mDoc means the window is torn down (e.g. navigated away from and not bfcached, had its containing iframe removed from the DOM, that sort of thing).

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #26) > On which window? The relevant window of the ServiceWorkerContainer. > Um... iframes always have a valid document, as long as they're in the DOM > tree. > No mDoc means the window is torn down (e.g. navigated away from and not > bfcached, had its containing iframe removed from the DOM, that sort of > thing). Thanks, apparently, I've got these wrong. I was misguided by the `blank.html`[0], which doesn't have a document tag, in those testcases. The root cause of the failure is using the wrong base URI to form the script spec. [0] http://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/blank.html

> The relevant window of the ServiceWorkerContainer. OK. _Does_ the spec define the navigated-away-from behavior (when GetOwner will randomly be null in Gecko), like I asked for above? > I was misguided by the `blank.html`[0], which doesn't have a document tag, in those testcases. I have no idea what this means. While the blank.html is loaded, it has a document. When it's no longer loaded, the document can obviously go away...

Hi Ben, Since the implementation is done and the reason of those testcasess fail are checked, I think maybe we can start reviewing the patches. However, I do want to address Boris's comment, but I am not so sure about what the status of the spec is and don't what to do to deal with this kind of spec issue. Can you give me some hint here? Do please feel free to cancel those review if you consider it inappropriate.

Comment on attachment 8878396 [details] [diff] [review] P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers Review of attachment 8878396 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerContainer.cpp @@ +144,5 @@ > > nsCOMPtr<nsIURI> baseURI; > + nsCOMPtr<nsPIDOMWindowInner> window = GetOwner(); > + if (window) { > + baseURI = window->GetDocBaseURI(); Can we just throw an NS_ERROR_DOM_INVALID_STATE_ERROR here for now? Does that devtools test still cause us problems? I feel like throwing an error would be the safest thing for now until the spec is clarified.

Comment on attachment 8882193 [details] [diff] [review] (V2) P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers Sure, this patch throws NS_ERROR_DOM_INVALID_STATE_ERR when we cannot get the window. On the other hand, I am not so worried about the devtools tests, because now the baseURI is always computed as what we do for the devtools tests, and thus we won't break those tests theoretically. To make sure of that, I've pushed a try yesterday, and the result looks fine. https://treeherder.mozilla.org/#/jobs?repo=try&revision=549c0a531e10db7b47a4b21501b529a86dd49e3f

Comment on attachment 8882193 [details] [diff] [review] (V2) P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers Review of attachment 8882193 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks! Please just make sure there is a spec issue where we ask for clarification on this and note there that we are throwing InvalidStateErr for now. ::: dom/workers/ServiceWorkerContainer.cpp @@ +149,3 @@ > } else { > + aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); > + return nullptr; Can you just change this to "short circuit style"? Something like: if (!window) { aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); return nullptr; } baseURI = window->GetDocBaseURI();

Sorry I conflicted with your title change. r+, though. Thanks!

NI? myself as a reminder of filing a spec issue mentioned in comment 17.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e017c81d94de P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/c926529797f2 P2: Enable related testcases. r=bkelly

https://hg.mozilla.org/mozilla-central/rev/e017c81d94de https://hg.mozilla.org/mozilla-central/rev/c926529797f2

Thanks Ho-Pang!

It doesn't look to me like this requires docs update, so I'm removing the ddn. Let me know if I'm wrong.