1298819 - Update to latest SW spec settings object usage

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

See this issue and the tests in this comment:

https://github.com/w3c/ServiceWorker/issues/922#issuecomment-239036842

Comment 1

[Domenic Denicola]

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 6

[Ben Hsu [:HoPang]]

Sure thing

Comment 7

[Ben Hsu [:HoPang]]

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

Comment 8

[Ben Hsu [:HoPang]]

Attachment: P1: Introduce a JSAPI to obtain the relevant global object from a JSContext

Comment 9

[Ben Hsu [:HoPang]]

Attachment: P2: Use relevant global objects instead of entry global objects when registering service workers

Comment 10

[Ben Hsu [:HoPang]]

Attachment: P3: Enable related testcase

Comment 11

[Ben Hsu [:HoPang]]

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

Comment 12

[Ben Hsu [:HoPang]]

Attachment: P1: Introduce a JSAPI to obtain the relevant global object from a JSContext

Comment 13

[Ben Hsu [:HoPang]]

Attachment: P2: Use relevant global objects instead of entry global objects when registering service workers

Comment 14

[Ben Hsu [:HoPang]]

Attachment: P3: Enable related testcases

Comment 15

[Ben Hsu [:HoPang]]

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?

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 19

[Ben Hsu [:HoPang]]

Attachment: P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers

Comment 20

[Ben Hsu [:HoPang]]

Attachment: P2: Enable related testcases

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Ben Hsu [:HoPang]]

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

Comment 23

[Chris Mills [:cmills]]

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

Comment 24

[Ben Hsu [:HoPang]]

Attachment: P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers

Comment 25

[Ben Hsu [:HoPang]]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[Ben Hsu [:HoPang]]

(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

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

Comment 29

[Ben Hsu [:HoPang]]

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 30

[Ben Kelly [:bkelly, not reviewing]]

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 31

[Ben Hsu [:HoPang]]

Attachment: (V2) P1: Use the owner global object of a ServiceWorkerContainer instead of the entry global object when registering service workers

Comment 32

[Ben Hsu [:HoPang]]

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 33

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 35

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b6da0f32ce5cee3fc7ac2454e99ca1fad7d6766

Comment 36

[Ben Hsu [:HoPang]]

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

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Ho-Pang!

Comment 40

[Chris Mills [:cmills]]

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