Turn on Promise for b2g

RESOLVED FIXED in mozilla26

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: bz)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 780897 [details] [diff] [review]
Patch
Attachment #780897 - Flags: review?(amarchesini)
(Reporter)

Updated

5 years ago
Blocks: 871445, 876397
Comment on attachment 780897 [details] [diff] [review]
Patch

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

lgtm
Attachment #780897 - Flags: review?(amarchesini) → review+
(Reporter)

Comment 4

5 years ago
Created attachment 780909 [details] [diff] [review]
Patch, V1.1

Add the following fix with a proper default:

Promise::PrefEnabled()
{
  return Preferences::GetBool("dom.promise.enabled", true);
}

Try: https://tbpl.mozilla.org/?tree=Try&rev=c639ed2bf886
Attachment #780897 - Attachment is obsolete: true
Attachment #780909 - Flags: review?(amarchesini)

Updated

5 years ago
Attachment #780909 - Flags: superreview?(bzbarsky)
(Reporter)

Updated

5 years ago
Attachment #780897 - Attachment is obsolete: false
Attachment #780897 - Flags: superreview?(bzbarsky)
(Reporter)

Comment 5

5 years ago
Comment on attachment 780909 [details] [diff] [review]
Patch, V1.1

Per discussion with Baku, we don't need comment #4. Set it to false if the entry is not available.
Attachment #780909 - Attachment is obsolete: true
Attachment #780909 - Flags: superreview?(bzbarsky)
Attachment #780909 - Flags: review?(amarchesini)
Comment on attachment 780897 [details] [diff] [review]
Patch

Promises are not enabled on purpose. The specification is not finalised yet so we should not ship it for the moment. It has landed in mozilla-central for testing purposes but not for usage by real content.

Gene, why do you want to enable this by default?
Attachment #780897 - Flags: superreview?(bzbarsky) → superreview-
(Reporter)

Comment 7

5 years ago
(In reply to Mounir Lamouri (:mounir) from comment #6)
> Comment on attachment 780897 [details] [diff] [review]
> Patch
> 
> Promises are not enabled on purpose. The specification is not finalised yet
> so we should not ship it for the moment. It has landed in mozilla-central
> for testing purposes but not for usage by real content.
> 
> Gene, why do you want to enable this by default?

Because Inter-App Communication and Data Store API need it. Or we should just enable it for B2G only for this moment?
(Reporter)

Comment 8

5 years ago
Or we should still use DOMRequest for this moment?
I think we can enable Promises on B2G. Especially since what we have implemented is a pretty safe subset.

If we wanted to be extra conservative we could disable the Promise constructors (including the static Promise.resolve and Promise.reject) functions. Those aren't needed for API implementations anyway.
(Reporter)

Comment 10

5 years ago
Created attachment 781612 [details] [diff] [review]
Patch, V2
Attachment #780897 - Attachment is obsolete: true
Attachment #781612 - Flags: superreview?(bzbarsky)
Attachment #781612 - Flags: review+
(Reporter)

Comment 11

5 years ago
Oops... Try server doesn't pass test_promise.html:

SpecialPowers.setBoolPref("dom.promise.enabled", false);
ok(!("Promise" in window), "Promise object should not exist if disabled by pref");

Weird... Will figure it out next Monday.
> Weird... Will figure it out next Monday.

Changing the pref will affect window objects created after that point, but not existing window objects, of course.
Comment on attachment 781612 [details] [diff] [review]
Patch, V2

I think we should in fact disable the static methods....
Attachment #781612 - Flags: superreview?(bzbarsky) → superreview-

Updated

5 years ago
Summary: Turn on Promise for all kinds of platforms and builds → Turn on Promise for b2g
(Reporter)

Comment 14

5 years ago
OK. I see. Thank you guys for all the suggestions. I'll come back with a new patch.
So the plan is to turn off just the static functions, but leave the constructor and the .then/.catch functions?
(In reply to Jonas Sicking (:sicking) from comment #15)
> So the plan is to turn off just the static functions, but leave the
> constructor and the .then/.catch functions?

Currently our Promise implementation supports Promise.resolve(), Promise.reject() as static methods. We don't support Promise.some/any/every yet.
We can make window.Promise hidden too.
(In reply to Jonas Sicking (:sicking) from comment #15)
> So the plan is to turn off just the static functions, but leave the
> constructor and the .then/.catch functions?

I think we should turn-off the ctor unless we have a strong reason to not do so.
it seems strange to me that we have API that are able to create promise, but content code cannot. If we turn-off the ctor we don't allow to create chain of promises, except with then/catch. What is the reason to turn-off the ctor?
Does anyone know how I could reference a Promise in my interface definition in b2g.idl ?
Flags: needinfo?
1) Don't write .idl
2) If you have to, use nsISupports for a promise, I guess.
Flags: needinfo?
(Reporter)

Comment 22

5 years ago
Created attachment 783151 [details] [diff] [review]
Patch, V3

This is just a WIP. It seems the final decision is:

* For B2G
  1. window.Promise is available.
  2. Turn on Promise(), then() and catch().
  3. Turn off other static methods.

* For non-B2G
  1. window.Promise is NOT available, thus we cannot use Promise in any way, including the constructor.

Is that correct?
Attachment #781612 - Attachment is obsolete: true
Attachment #783151 - Flags: feedback?(bzbarsky)
Attachment #783151 - Flags: feedback?(amarchesini)
(Reporter)

Comment 23

5 years ago
(In reply to Andrea Marchesini (:baku) from comment #19)
> it seems strange to me that we have API that are able to create promise, but
> content code cannot. If we turn-off the ctor we don't allow to create chain
> of promises, except with then/catch. What is the reason to turn-off the ctor?

Agree. If we turn off the Promise constructor, how can we write something like:

function API() {
  return new this._window.Promise(...);
}

in the Gecko codes of content process (note that I don't mean content codes)?
Comment on attachment 783151 [details] [diff] [review]
Patch, V3

>  1. window.Promise is available.

Why?  I would prefer we not expose it to web pages even on b2g.  Certified apps or chrome or whatnot is a separate issue.

And "not available" should mean "not defined", not "will mess up you object detection but throw if you call it", for both window.promise and the static methods.  See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BFunc.3D.22funcname.22.5D and https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#PrefControlled

Note that given the IDL and the way PrefControlled works the changes to Constructor here make no sense.
Attachment #783151 - Flags: feedback?(bzbarsky) → feedback-
> how can we write something like:

Is the code in question running as chrome?  Then you can do it pretty easily by making the ctor [ChromeOnly].  I can make [Func] work on interfaces too, if we want to enable if either chrome or pref is set.
(Reporter)

Comment 26

5 years ago
Understand! Thanks! Will come back with new patch.
(Reporter)

Updated

5 years ago
Attachment #783151 - Flags: feedback?(amarchesini)
Duplicate of this bug: 899670
In fact, I suspect that making Func work on interfaces is the right thing here. Please let me know if you want me to do that.
(Reporter)

Comment 29

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #28)
> In fact, I suspect that making Func work on interfaces is the right thing
> here. Please let me know if you want me to do that.

Yeap, thank you very much! I think I should assign this to the expert to save time without too many discussions back and forth. The objectives for the B2G API purposes are:

1. For non-B2G build:
   a. Web pages cannot see window.Promise and cannot use constructor.
   b. Web pages cannot use Promise.resolve() and Promise.reject().
   c. Web pages cannot use promiseObj.then() and promiseObj.catch().

2. For B2G build:
   a. Chrome codes can see window.Promise and can use the constructor.
   b. Web pages cannot see window.Promise and cannot use constructor.
   c. Web pages cannot use Promise.resolve() and Promise.reject().
   d. Web pages can use promiseObj.then() and promiseObj.catch().

Note: #2-a implies the API implementation can return a Promise object to the web page and #2-d implies web page can use then() to run the callback based on the Promise object returned from API.

Please feel free to let me know if you want me to finish it or further fix the test cases. Note that there could be a tricky behaviour stopping B2G passing the tests [1]:

  "Note that on b2g pref setting happens asynchronously. Use PushPrefEnv instead."

[1] https://developer.mozilla.org/en-US/docs/Mochitest#What_if_I_need_to_change_a_preference_to_run_my_test.3F
Assignee: gene.lian → bzbarsky
Hmm.  So here's an interesting question: do you need contentWindow.Promise to exist when touching the window from chrome code?  Or is it enough if chromeWindow.Promise exists?

Also, I see no reason not to use the "For B2G build" setup described in comment 29 for non-b2g builds as well, right?
Flags: needinfo?(gene.lian)
Oh, hmm.  And I guess if we need to create the Promise objects in the content scope that would also immediately resolve window.Promise.  Hrm...
(Reporter)

Comment 32

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #30)
> Hmm.  So here's an interesting question: do you need contentWindow.Promise
> to exist when touching the window from chrome code?  Or is it enough if
> chromeWindow.Promise exists?

Oh, I mean content window. For your reference, please search |Window.Promise| for its usage context. Is that feasible to new window.Promise only for chrome codes but not for web pages?

[1] DataStore API: https://bugzilla.mozilla.org/attachment.cgi?id=776987&action=diff
[2] Inter-App Comm API: https://bugzilla.mozilla.org/attachment.cgi?id=783131&action=diff

> 
> Also, I see no reason not to use the "For B2G build" setup described in
> comment 29 for non-b2g builds as well, right?

One difference is between #1-c and #2-d. It depends on whether the web pages can use .then()/.catch() on the non-b2g builds. Honestly, I don't have strong opinion about that but it seems OK to expose them because these private methods look relatively safe according to previous discussions.

Also, we don't allow chrome code to use window.Promise in non-b2g build (#2-a). Right?
Flags: needinfo?(gene.lian)
> Is that feasible to new window.Promise only for chrome codes but not for web pages?

That's actually pretty difficult.  Getting contentWindow.Promise on an Xray would normally set up window.Promise in the content window and then give chrome an Xray to the constructor object.

So we can make |new window.Promise| throw in content, but we can't easily make |"Promise" in window| false in content while allowing contentWindow.Promise from chrome.

Let me think about whether there's a way to do this sanely.

> One difference is between #1-c and #2-d. 

I see no reason non-b2g builds can't have the #2-d behavior.  It'll only matter if some chrome code gives content a Promise object.

> Also, we don't allow chrome code to use window.Promise in non-b2g build (#2-a). Right?

I want to allow non-b2g chrome code to use Promise; I see no reason to disallow it.
Why don't we want content code to create promises?
I'm not really convinced that it's worth hiding the Promise constructor. My understanding is that the behavior of the constructor is not one of the controversial issues that still remain.
The big problem from my point of view is that it's reasonable for pages to check for window.Promise and then assume the DOM Promises spec is implemented.  So exposing the constructor without exposing most of the rest of the spec is not so great.  :(
Can't we add the whole implementation with a prefix?
We will not add any new prefixed interfaces. (Except on b2g?)
Given that constructable promises is strictly a convenience function, I think we can do without it for now. For 1.3 the spec should have stabilized enough that we can ship it as well as use it in our production code.
So I think I have an idea for how we can expose contentWindow.Promise to chrome without exposing it to content.  Will update later today.
Created attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.
Attachment #785912 - Flags: review?(jonas)
Created attachment 785913 [details] [diff] [review]
part 2.  Allow touching interface objects via an Xray even if the page they're in can't touch them.
Attachment #785913 - Flags: review?(bobbyholley+bmo)
Created attachment 785915 [details] [diff] [review]
part 3.  Enable Promise in chrome and certified apps, even when preffed off.
Attachment #785915 - Flags: review?(bobbyholley+bmo)
Attachment #785915 - Flags: review?(amarchesini)
Gene, I'd appreciate if it if you have time to put together some tests.....
Flags: needinfo?(gene.lian)
Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

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

I'd rather not do this change as part of this bug. It's certainly something that we can discuss, but I'd rather not hold up this bug for it.
Attachment #785912 - Flags: review?(jonas) → review-
Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

Well, it's either this or write much more complicated code for other parts of this bug, as far as I can tell.  If you insist, I can do the latter, but this seems like the quickest way to get things done to me.
Attachment #785912 - Flags: review- → review?(jonas)
Note that if you're just not convinced the change should be made, I can write the more complicated code, of course...
Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

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

Ok, I'd like Mounir's review then. Mounir, given that so many callsites are working around these asserts by checking the app-id first. It's questionable if they are gaining us anything. In particular, it seems that we still end up with Documents that have UNKNONW_APP_ID principals, and as long as that's the case I don't think that asserts are actually getting us anything.
Attachment #785912 - Flags: review?(mounir)
It's entirely possible that the callsites checking the app-id are just cargo-culting... as in, the app-id checks could just be removed anyway.
(Reporter)

Comment 52

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #46)
> Gene, I'd appreciate if you have time to put together some tests.....

Sure. No problem!
Flags: needinfo?(gene.lian)
Comment on attachment 785915 [details] [diff] [review]
part 3.  Enable Promise in chrome and certified apps, even when preffed off.

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

::: dom/promise/Promise.cpp
@@ +126,5 @@
> +  // aCx here will be in the Xray compartment but the object we're passed is in
> +  // the content compartment!  We don't want to ever use the object in this
> +  // method, because we want to base everything on the Xray compartment.
> +  bool isChrome = NS_IsMainThread() ?
> +    xpc::AccessCheck::isChrome(js::GetContextCompartment(aCx)) :

I'm not happy about the AccessCheck include here. Do you have any reason to believe that aCx will not be stack-top here? If so, that's a serious bug in our bindings. Please just use nsContentUtils::GetSubjectPrincipal here and remove the AccessCheck stuff.

@@ +133,5 @@
> +    return true;
> +  }
> +
> +  // XXXbz Would be nice to have soemthing like GetObjectPrincipal but for
> +  // JSContext.

Well, in general there should never be a reason to inspect the context principal if it's not stack-top outside of XPConnect...

::: dom/webidl/Promise.webidl
@@ +24,5 @@
> +  // Belt-and-suspenders: disable the static methods when the interface object
> +  // is supposed to be disabled, just in case someone screws up and manages to
> +  // create a Promise object in this scope without having resolved the interface
> +  // object first... or in case some code decides to walk over to .constructor
> +  // from the proto of a promise object.

The latter seems pretty clearly non-belt-and-suspenders.
Attachment #785915 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 785913 [details] [diff] [review]
part 2.  Allow touching interface objects via an Xray even if the page they're in can't touch them.

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

As usual, I didn't review the Codegen.py changes very closely.

::: dom/base/nsDOMClassInfo.cpp
@@ +3539,5 @@
>        Maybe<JSAutoCompartment> ac;
>        JS::Rooted<JSObject*> global(cx);
>        bool defineOnXray = xpc::WrapperFactory::IsXrayWrapper(obj);
>        if (defineOnXray) {
> +        // Check whether to define this property on the Xray first

Nit - period. And elaborate a bit about how this callback is special.

@@ +3554,5 @@
>        } else {
>          global = obj;
>        }
>  
> +      // Check whether to define on the global too.

Note in the comments how we've (quite un-idomatically) entered a different compartment by this point.

::: dom/bindings/BindingUtils.h
@@ +379,5 @@
>                         unsigned ctorNargs, const NamedConstructor* namedConstructors,
>                         JS::Heap<JSObject*>* constructorCache, const DOMClass* domClass,
>                         const NativeProperties* regularProperties,
>                         const NativeProperties* chromeOnlyProperties,
> +                       const char* name, bool defineOnGlobal);

I think we need some comments somewhere explaining why we might not want to define it on the global.
Attachment #785913 - Flags: review?(bobbyholley+bmo) → review+
> Do you have any reason to believe that aCx will not be stack-top here? 

Hmm.  Not offhand, no.

> Please just use nsContentUtils::GetSubjectPrincipal here

Does that work correctly on workers?

> As usual, I didn't review the Codegen.py changes very closely.

OK.  In this case those are pretty finicky, so I'll try another reviewer for those.
Flags: needinfo?(bobbyholley+bmo)
Comment on attachment 785913 [details] [diff] [review]
part 2.  Allow touching interface objects via an Xray even if the page they're in can't touch them.

Olli, I know you hate it, but I need review on the codegen bits here.  :(
Attachment #785913 - Flags: review?(bugs)
(In reply to Boris Zbarsky (:bz) from comment #55)
> > Please just use nsContentUtils::GetSubjectPrincipal here
> 
> Does that work correctly on workers?

No, it will abort - I meant in the NS_IsMainThread branch.
Flags: needinfo?(bobbyholley+bmo)
Created attachment 786450 [details] [diff] [review]
Part 3 updated to comments
Attachment #786450 - Flags: review?(bobbyholley+bmo)
Attachment #786450 - Flags: review?(amarchesini)
Attachment #785915 - Attachment is obsolete: true
Attachment #785915 - Flags: review?(amarchesini)
Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

Mounir's review should be enough.
Attachment #785912 - Flags: review?(jonas)
Comment on attachment 785913 [details] [diff] [review]
part 2.  Allow touching interface objects via an Xray even if the page they're in can't touch them.

Somewhat odd to define the property occasionally in bindings and in some cases
in nsDOMClassInfo. But this patch doesn't really change that, just moves code.
Attachment #785913 - Flags: review?(bugs) → review+
Comment on attachment 786450 [details] [diff] [review]
Part 3 updated to comments

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

r=bholley

::: dom/promise/Promise.cpp
@@ +124,5 @@
> +  // Note that we have no concept of a certified app in workers.
> +  // XXXbz well, why not?
> +  if (!NS_IsMainThread()) {
> +    return
> +      mozilla::dom::workers::GetWorkerPrivateFromContext(aCx)->IsChromeWorker();

This spacing/indentation decision looks ugly to me.

::: dom/webidl/Promise.webidl
@@ +24,5 @@
> +  // Disable the static methods when the interface object is supposed
> +  // to be disabled, just in case someone screws up and manages to
> +  // create a Promise object in this scope without having resolved the
> +  // interface object first... or in case some code decides to walk
> +  // over to .constructor from the proto of a promise object.

My beef with this comment earlier is that the second reason is much more compelling and universally relevant than the first, which is why it seems wrong to frame it as an afterthought.
Attachment #786450 - Flags: review?(bobbyholley+bmo) → review+
> This spacing/indentation decision looks ugly to me.

Ok, how about:

    return workers::GetWorkerPrivateFromContext(aCx)->IsChromeWorker();

since we're in the mozilla::dom namespace anyway?

> the second reason is much more compelling and universally relevant than the first

Fair.  Flipped the comment around.
sounds great.
(Reporter)

Comment 64

5 years ago
(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #46)
> Gene, I'd appreciate if it if you have time to put together some tests.....

It seems we don't need to fix any test cases [1].

[1] https://tbpl.mozilla.org/?tree=Try&rev=1a129c1f81b0
Well, yes, but we need to add tests for stuff like "web page does not see Promise constructor if pref is off" (if we don't have one already) and "chrome sees Promise constructor even if pref is off" and "certified app sees Promise constructor even if pref is off".
Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

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

I would prefer if the MOZ_ASSERT could be changed for something else. NS_ASSERTION might break DEBUG tests nowadays so maybe NS_WARNING? I think being able to track UNKNOWMN_APP_ID wouldn't hurt given that we do not expect them to exist when those methods are called and even if that proves to be incorrect, keeping track of it wouldn't hurt.

::: content/base/src/nsDocument.cpp
@@ +2549,5 @@
>  
>    nsIPrincipal* principal = NodePrincipal();
>  
> +  uint16_t appStatus = principal->GetAppStatus();
> +  applyAppDefaultCSP = appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED ||

bool applyDefaultCSP = ...;
and remove the init above.

You can as well move the other init below this one.
Attachment #785912 - Flags: review?(mounir) → review+
> I would prefer if the MOZ_ASSERT could be changed for something else.

I can even leave the MOZ_ASSERT if it's ok to leave it and call into this code for a document's principal without checking the app id.... I just don't know whether that's an OK thing to do.  Is it?
Flags: needinfo?(mounir)
(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #67)
> > I would prefer if the MOZ_ASSERT could be changed for something else.
> 
> I can even leave the MOZ_ASSERT if it's ok to leave it and call into this
> code for a document's principal without checking the app id.... I just don't
> know whether that's an OK thing to do.  Is it?

It is okay but I believe that code was added because it was crashing...
Flags: needinfo?(mounir)
It *should* be ok yes. But we've in the past hit rare buggy edge-cases which triggered the assertion. To be clear, in those cases the assertion did indicate that something was wrong. However edge cases only lived in Firefox Desktop code, and these errors don't affect Firefox Desktop. So that's why it's been worked around in various cases.

It's unclear if we'd actually hit the assertion here though. Putting a comment above the assertion saying that if we keep hitting it in hard-to-fix/low-value cases, then changing the assertion to a warning is ok with me. Or simply changing to a warning right now is also ok with me.

Comment 71

5 years ago
Boris, Andrea is currently on vacation, and will be back on Aug 19th.
Comment 70 was in response to comment 67.

I think going with a warning sounds is the way to go for now. Our current APIs are just not good enough to iron-proof enough to be sure that the assert won't happen.
The failure was from bug 902485.
(Reporter)

Updated

5 years ago
Blocks: 902843
Comment on attachment 786450 [details] [diff] [review]
Part 3 updated to comments

lgtm
Attachment #786450 - Flags: review?(amarchesini)
Keywords: dev-doc-needed
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.