Closed Bug 1306170 Opened 8 years ago Closed 7 years ago

Consider implementing .origin on Window and WorkerGlobalScope

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(6 files)

Need to check whether the spec makes sense.
Priority: -- → P3
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."
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?
(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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Blocks: 931884
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
Attachment #8835607 - Flags: review?(bkelly) → review+
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?
Attachment #8835608 - Flags: review?(bkelly) → review+
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?
Attachment #8835610 - Flags: review?(bkelly) → review+
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.
Attachment #8835609 - Flags: review?(bkelly)
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().
Attachment #8835611 - Flags: review?(bkelly) → review+
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.
Attachment #8835612 - Flags: review?(bkelly) → review+
NI for comment 14.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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!
Attachment #8835609 - Flags: review+
> 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.
> 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.
Pushed by bzbarsky@mozilla.com:
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
Per the intent to ship on dev-platform the target for this is 54, marking 52/53 wontfix.
Fixed some parts of the docs that weren't quite right.
(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!
Depends on: 1359022
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.