988122 - Expose Promise on non-window non-worker globals

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Masatoshi Kimura [:emk]]

We are implementing Promise in the DOM due to limitations of SpiderMonkey.
It should be exposed on every global object by default without using something like Cu.importGlobalProperties or wantGlobalProperties as if it is one of standard ES objects.

Comment 1

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=2659b2a683a0

Comment 2

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=e12aa0253641

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch

See comment #0 on the rationale for adding a property to the global object.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8397901 [details] [diff] [review]
patch

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

I still have some misgivings about exposing the whole world of WebIDL to Sandboxes where we didn't previously, but in theory it's only a matter of time. So I'm tentatively ok with it, if Boris is.

::: dom/contacts/tests/test_migration_chrome.js
@@ +21,5 @@
>    DB_NAME,
>    ContactService,
>    Promise
>  } = imports;
> +Object.freeze(imports);

This hack (here and elsewhere) needs some explanation.

::: js/xpconnect/src/Sandbox.cpp
@@ +1009,5 @@
>  
>  bool
>  xpc::GlobalProperties::Define(JSContext *cx, JS::HandleObject obj)
>  {
> +    if (!dom::PromiseBinding::GetConstructorObject(cx, obj))

Please add a boolean for this, just like everything else, and default it to true in the GlobalProperties constructor. That way, people can still switch it off if they like.

::: js/xpconnect/tests/unit/test_promise.js
@@ +3,5 @@
> +  sb = new Cu.Sandbox('http://www.example.com');
> +  sb.do_check_eq = do_check_eq;
> +  Cu.evalInSandbox('do_check_eq(typeof new Promise(function(resolve){resolve();}), "object");',
> +                   sb);
> +  do_check_eq(typeof new Promise(function(resolve){resolve();}), "object");

And add some tests here to make sure that Promise can be switched off.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8397901 [details] [diff] [review]
patch

sr=me, though I'd _really_ like comments explaining what's going on with Object.freeze there.

Comment 6

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8a8379714c

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fc8a8379714c

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, now with those comments the "freeze" makes no sense, at either of the callsites.  It's freezing the RHS of the assignment in the one case, and the Promise object itself in the other case, which is totally different from what the "const" did!  Can you explain what you're trying to do there?

Comment 9

[Masatoshi Kimura [:emk]]

I was trying to prevent Promise from being deleted or replaced accidentally.

Comment 10

[Boris Zbarsky [:bzbarsky]]

That's not what Object.freeze(Promise) does, though.  Here's a simple testcase:

  var Promise = {};
  Object.freeze(Promise);
  Promise = 5;
  alert(Promise);

this alerts 5.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Followup: fix bogus constification code

Will this code work?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8399373 [details] [diff] [review]
Followup: fix bogus constification code

Yes, that's more like it!

Comment 13

[Masatoshi Kimura [:emk]]

Thanks for the review.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d28b17b283c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d28b17b283c