1306170 - Consider implementing .origin on Window and WorkerGlobalScope

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Need to check whether the spec makes sense.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

The spec is basically:

interface WindowOrWorkerGlobalScope {
  [Replaceable] readonly attribute USVString origin;
};

"The origin attribute's getter must return this object's relevant setting object's origin, serialized."

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah.  That doesn't seem unreasonable.  For the window case, we'd just GetUTFOrigin(GetPrincipal(), aRetVal).  

For the worker case, there's the mLocationInfo.mOrigin on the WorkerPrivate, but this does NOT seem like the right thing.  In particular, it's munged by WorkerPrivateParent<Derived>::SetBaseURI, right?  Or will that always end up with something with the right origin string?  It's not clear to me why it would, necessarily.

There's also mLoadInfo.mPrincipal, but we don't really want to do GetUTFOrigin on a worker thread, right?  Can we just grab the origin in WorkerPrivateParent<Derived>::SetPrincipal, and will that be the right origin?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> There's also mLoadInfo.mPrincipal, but we don't really want to do
> GetUTFOrigin on a worker thread, right?  Can we just grab the origin in
> WorkerPrivateParent<Derived>::SetPrincipal, and will that be the right
> origin?

I think that's the right thing to do, but I haven't looked in detail.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change the HTML general interfaces web platform test to have WindowOrWorkerGlobalScope

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Split the IDLs out of the HTML interfaces web platform test so we can run it in both windows and workers

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Fix idlharness to properly handle single-element exposure sets on interfaces and exposure sets on members

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Fix idlharness to properly handle FrozenArray return types

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Run the HTML interfaces web platform test in workers as well as windows

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Implement WindowOrWorkerGlobalScope.origin

Comment 10

[Boris Zbarsky [:bzbarsky]]

Parts 3 and 4 correspond to https://github.com/w3c/testharness.js/pull/241 and https://github.com/w3c/testharness.js/pull/242 respectively, so they're getting review upstream too.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8835607 [details] [diff] [review]
part 1.  Change the HTML general interfaces web platform test to have WindowOrWorkerGlobalScope

Review of attachment 8835607 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/dom/interfaces.html
@@ +2502,4 @@
>  
>  [NoInterfaceObject, Exposed=(Window,Worker)]
> +interface WindowOrWorkerGlobalScope {
> +  DOMString btoa(DOMString btoa);

nit:  The spec also has this comment:

  // base64 utility methods

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8835608 [details] [diff] [review]
part 2.  Split the IDLs out of the HTML interfaces web platform test so we can run it in both windows and workers

Review of attachment 8835608 [details] [diff] [review]:
-----------------------------------------------------------------

Trusting that the .idl files are right.  Splinter did not really make it easy to read the hg copy + changes here.

::: testing/web-platform/tests/html/dom/interfaces.html
@@ +217,5 @@
>  };
> +
> +function fetchData(url) {
> +  return new Promise(function(resolve, reject) {
> +    var xhr = new XMLHttpRequest();

The `fetch()` API has been shipped by chrome, firefox, safari, and edge.  Might be easier to just use that here since you want a promise anyway?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8835610 [details] [diff] [review]
part 4.  Fix idlharness to properly handle FrozenArray return types

Review of attachment 8835610 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/dom/interfaces.html
@@ +201,5 @@
>      CloseEvent: ['new CloseEvent("close")'],
>      AbstractWorker: [],
>      Worker: [],
>      SharedWorker: [],
> +    MessageEvent: ['new MessageEvent("message", { data: 5 })'],

This exercises the FrozenArray test via the ports member?

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8835609 [details] [diff] [review]
part 3.  Fix idlharness to properly handle single-element exposure sets on interfaces and exposure sets on members

Review of attachment 8835609 [details] [diff] [review]:
-----------------------------------------------------------------

Can you describe the problem that needs to be solved here?  I kind of understand what you are doing, but its unclear to me exactly what is being fixed.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8835611 [details] [diff] [review]
part 5.  Run the HTML interfaces web platform test in workers as well as windows

Review of attachment 8835611 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/html/dom/interfaces.worker.js.ini
@@ +1,4 @@
> +[interfaces.worker.html]
> +  type: testharness
> +  [Path2D interface: existence and properties of interface object]
> +    expected: FAIL

These failures match the failures we get in window tests, right?

::: testing/web-platform/tests/html/dom/interfaces.worker.js
@@ +21,5 @@
> +};
> +
> +function fetchData(url) {
> +  return new Promise(function(resolve, reject) {
> +    var xhr = new XMLHttpRequest();

Again I think we could use fetch().

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8835612 [details] [diff] [review]
part 6.  Implement WindowOrWorkerGlobalScope.origin

Review of attachment 8835612 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +9962,5 @@
>  
>  void
> +nsGlobalWindow::GetOrigin(nsAString& aOrigin)
> +{
> +  MOZ_ASSERT(IsInnerWindow());

This is a cheap test, so lets use MOZ_DIAGNOSTIC_ASSERT().

::: dom/base/nsGlobalWindow.h
@@ +1003,5 @@
>                        const mozilla::dom::Optional<int32_t>& aTimeout,
>                        const mozilla::dom::Sequence<JS::Value>& /* unused */,
>                        mozilla::ErrorResult& aError);
>    void ClearInterval(int32_t aHandle);
> +  void GetOrigin(nsAString& aOrigin);

Sad GetPrincipal() prevents us from making this const.

::: dom/workers/WorkerPrivate.cpp
@@ +1873,5 @@
>  
>    MOZ_ALWAYS_SUCCEEDS(
>      PrincipalToPrincipalInfo(aPrincipal, mPrincipalInfo));
> +
> +  nsContentUtils::GetUTFOrigin(aPrincipal, mOrigin);

SetPrincipalOnMainThread() is about to become fallible in bug 1338782 (I'm going to land today).  Please change this to return the nsresult.

::: dom/workers/WorkerScope.h
@@ +139,5 @@
>    void
>    ClearInterval(int32_t aHandle);
>  
>    void
> +  GetOrigin(nsAString& aOrigin);

I think this can be const.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

NI for comment 14.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Good catch; I need to give that commit a much better commit message.

So there are two problems being solved in attachment 8835609 [details] [diff] [review], and maybe I should pull them out into separate commits...  then again, this already got merged as a single commit upstream, so there's not much point.  And upstream won't have the better commit message either.  :(

1) Consider this interface:

  [Exposed=(Window,Worker)]
  interface Foo {
    [Exposed=(Window)]
    void someMethod();
  };

Without my changes, idlharness will check for a someMethod method on Foo.prototype in all globals where the Foo interface is exposed, and will fail in a worker, because someMethod is not exposed there.  That's what the blocks ending with "continue" in IdlInterface.prototype.test_members and IdlInterface.prototype.test_interface_of fix.  This needs the "default_set" argument to exposure set computation, because if there isn't any exposure set specified on a member, it gets the exposure set of its interface.

2)  Consider this interface:

  [Exposed=SharedWorker]
  interface Foo {
  };

The code without my changes does:

        var globals = exposed.length === 1
                    ? exposed[0].rhs.value
                    : ["Window"];

which sets "globals" to the _string_ "SharedWorker".  Then it does member.exposed = exposed_in(globals).  expose_in tries to figure out which sort of global we're running in.  The interesting part is _just_ visible in the context in the diff.  Say we're running in a dedicated worker.  Then this test:

     if ('DedicatedWorkerGlobalScope' in self &&
         self instanceof DedicatedWorkerGlobalScope) {

tests true and this return value:

         return globals.indexOf("Worker") >= 0 ||
                globals.indexOf("DedicatedWorker") >= 0;

returns true, because if you recall the variable globals is the _string_ "SharedWorker", so globals.indexOf("Worker") >= 0.  Yay Python typing.

The fix for this is to make sure that globals ends up as the _list_ ["SharedWorker"] in this case, so our indexOf checks in exposed_in do the right things.  That's the:

+    if (typeof set == "string") {
+        set = [ set ];

bit.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8835609 [details] [diff] [review]
part 3.  Fix idlharness to properly handle single-element exposure sets on interfaces and exposure sets on members

Thanks!

Comment 20

[Boris Zbarsky [:bzbarsky]]

> nit:  The spec also has this comment:

Added.

> Splinter did not really make it easy to read the hg copy + changes here.

:(  Anyway, I just hg copied and then deleted stuff (and added a few comments, I guess).  No other changes.

> Might be easier to just use that [fetch] here since you want a promise anyway?

Good point.  Done:

  function fetchData(url) {
    return fetch(url).then((response) => response.text());
  }

I agree this is much nicer.  ;)

> This exercises the FrozenArray test via the ports member?

Yes, exactly.

> These failures match the failures we get in window tests, right?

No.  There are three sources of failures here:

1) The Path2D failures are because per spec a bunch of the canvas stuff is exposed in workers, but we don't implement any of that yet.

2) We have an onlanguagechange event on Window, but we don't have it on WorkerGlobalScope.  Of course it's not like we ever fire languagechange events.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1154779

3) We don't support SharedWorker inside a worker.

> Again I think we could use fetch().

Done.

> This is a cheap test, so lets use MOZ_DIAGNOSTIC_ASSERT().

Done.

> Sad GetPrincipal() prevents us from making this const.

Hmm.  I could probably make GetPrincipal() const.  I'll throw a changeset on top of these.

> SetPrincipalOnMainThread() is about to become fallible

Updated:

  rv = nsContentUtils::GetUTFOrigin(aPrincipal, mOrigin);
  NS_ENSURE_SUCCESS(rv, rv);

> I think this can be const.

Yep, done.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Hmm.  I could probably make GetPrincipal() const.  I'll throw a changeset on top of these.

No, this doesn't work, because GetParentInternal() is _very_ hard to make const (e.g. it can return "this" in some cases).  So not going to worry about this for now.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb9b3880581
part 1.  Change the HTML general interfaces web platform test to have WindowOrWorkerGlobalScope.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/1907c1299943
part 2.  Split the IDLs out of the HTML interfaces web platform test so we can run it in both windows and workers.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a928f98527cc
part 3.  Fix idlharness to properly handle single-element exposure sets on interfaces and exposure sets on members.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/622aa19582d4
part 4.  Fix idlharness to properly handle FrozenArray return types.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ee2ce41916
part 5.  Run the HTML interfaces web platform test in workers as well as windows.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f74cfa4af59
part 6.  Implement WindowOrWorkerGlobalScope.origin.  r=bkelly

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abb9b3880581
https://hg.mozilla.org/mozilla-central/rev/1907c1299943
https://hg.mozilla.org/mozilla-central/rev/a928f98527cc
https://hg.mozilla.org/mozilla-central/rev/622aa19582d4
https://hg.mozilla.org/mozilla-central/rev/a3ee2ce41916
https://hg.mozilla.org/mozilla-central/rev/5f74cfa4af59

Comment 24

[Julien Cristau [:jcristau]]

Per the intent to ship on dev-platform the target for this is 54, marking 52/53 wontfix.

Comment 25

[Chris Mills [:cmills]]

I've documented this:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope
https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/origin

And added a note to the Fx54 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/54#Web_workers_and_Service_Workers

Let me know if these look OK. Thanks!

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fixed some parts of the docs that weren't quite right.

Comment 27

[Chris Mills [:cmills]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #26)
> Fixed some parts of the docs that weren't quite right.

Thanks Boris!