Closed Bug 988122 Opened 6 years ago Closed 6 years ago

Expose Promise on non-window non-worker globals

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: emk, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Blocks: 988130
Attached patch patchSplinter Review
See comment #0 on the rationale for adding a property to the global object.
Attachment #8397901 - Flags: review?(bobbyholley)
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.
Attachment #8397901 - Flags: superreview?(bzbarsky)
Attachment #8397901 - Flags: review?(bobbyholley)
Attachment #8397901 - Flags: review+
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.
Attachment #8397901 - Flags: superreview?(bzbarsky) → superreview+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8a8379714c
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite+
Blocks: 885333
Blocks: 941920
https://hg.mozilla.org/mozilla-central/rev/fc8a8379714c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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?
Flags: needinfo?(VYV03354)
I was trying to prevent Promise from being deleted or replaced accidentally.
Flags: needinfo?(VYV03354)
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.
Flags: needinfo?(VYV03354)
Will this code work?
Attachment #8399373 - Flags: review?(bzbarsky)
Flags: needinfo?(VYV03354)
Comment on attachment 8399373 [details] [diff] [review]
Followup: fix bogus constification code

Yes, that's more like it!
Attachment #8399373 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.