Stop allowing promises to be implicitly passed as nsISupports

REOPENED
Assigned to

Status

()

enhancement
P2
normal
REOPENED
Last year
Last year

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Depends on 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

Follow-up to bug 1455217. After this patch, promises can only be passed through the explicit promise type.
Priority: -- → P2
Comment on attachment 8972175 [details] [diff] [review]
Part 1: Stop converting promises to nsISupports in xpconnect

Seems OK, but why do we not need to deal with the "NOTE(nika): Add a cleanup mode if Promise becomes non-nsISupports." comment in XPCConvert::JSArray2Native and the "NOTE(nika): Change if Promise becomes non-nsISupports" comment in CallMethodHelper::CleanupParam?
Attachment #8972175 - Flags: review?(bzbarsky) → review+
Comment on attachment 8972176 [details] [diff] [review]
Part 2: Use native refcounting for Promises

The bits in CallMethodHelper::CleanupParam are definitely wrong with this change...
Attachment #8972176 - Flags: review?(bzbarsky) → review-
Comment on attachment 8972176 [details] [diff] [review]
Part 2: Use native refcounting for Promises

Oh. I should've probably clarified. The reason why I don't change those locations is that I already changed them in bug 1457972 which I built this patch upon. That patch unifies all of the cleanup codepaths into a single one, where we explicitly cast promises to Promise*.
Attachment #8972176 - Flags: review- → review?(bzbarsky)
Comment on attachment 8972176 [details] [diff] [review]
Part 2: Use native refcounting for Promises

Makes a lot more sense now.  ;)

r=me
Attachment #8972176 - Flags: review?(bzbarsky) → review+

Comment 7

Last year
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51cdd0595094
Part 1: Stop converting promises to nsISupports in xpconnect, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5c103fcb70
Part 2: Use native refcounting for Promises, r=bz

Comment 8

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/51cdd0595094
https://hg.mozilla.org/mozilla-central/rev/0b5c103fcb70
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
No longer depends on: 1352644

Comment 9

Last year
Backout by aciure@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/1800b8895c08
Backed out 2 changesets for dom/presentation/tests/mochitest/test_presentation_1ua_connection_wentaway_inproc.html failures a=backout
nika, looks like the test failures is caused by

https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/dom/presentation/tests/mochitest/test_presentation_1ua_connection_wentaway.js#63-70

obs.notifyObservers(promise, 'setup-request-promise');

Activity stream was actually maybe just about to add a notifyObserver(new Promise(…), "browser-open-newtab-start") in bug 1425494 instead of a notifyObserver({wrappedJSObject: { newTabCreatedPromise: new Promise(…) }, …). With this change, is there an expected/standard way to pass around JS Promises with notifyObserver?
Flags: needinfo?(nika)
See Also: → 1425494
nanj, perhaps saving on one level object wrapping:

notifyObserver({ wrappedJSObject: new Promise(…) }, …)
> Backed out 2 changesets (bug 1458043)

Is there a reason this bug wasn't reopened?
Status: RESOLVED → REOPENED
Flags: needinfo?(aciure)
Resolution: FIXED → ---
> obs.notifyObservers(promise, 'setup-request-promise');

Right, this is being tracked in bug 1461890.

> is there an expected/standard way to pass around JS Promises with notifyObserver

How would you pass an Array or a Map or Set?  Do the same thing...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> > is there an expected/standard way to pass around JS Promises with notifyObserver
> How would you pass an Array or a Map or Set?  Do the same thing...
Sorry if it this was something clearly obvious. I couldn't find Map/Set usage with notifyObservers, but I did find some nsIMutableArray and various nsISupports*.data wrappers. I do see various { wrappedJSObject } patterns, so we'll just go with that. I'll bring this up in the fx-desktop "this week I learned" in case others were also confused.
Flags: needinfo?(nika)
OK.  So on a serious note, the simplest thing to do is probably:

  var obj = { value: myThing };
  obj.wrappedJSObject = obj;

Then you pass obj and on the other end do obj.wrappedJSObject.value....
Out of curiosity, why are we going out of our way to disable the existing pattern (just passing a promise)?  It does, after all, make calling code more readable...
A few reasons we might want to do it:

1)  It reduces xpconnect complexity.
2)  It makes the C++ promise representation smaller and faster.

Importantly, we didn't realize people were using what you call the "existing pattern".  The ability to pass Promise as nsISupports was added to allow passing Promises through xpidl interfaces without having to deal wit the misery of "jsval", not to allow object-identity round-tripping of them.  Especially because that round-tripping only works in one direction (from JS to C++ and back; if you pass from C++ to JS and then back to C++, you do _not_ preserve Promise object identity).
As far as I can tell this sort of trickery to pass around a random jsobject as a nsISupports through C++ is mostly only relevant to nsIObserverService. We could perhaps just expose a different method to JS for nsIObserverService::AddObserver which takes a callback taking a jsval rather than nsISupports, and unwraps nsXPCWrappedJS objects automatically before passing them in?

Kinda hacky but dodges this issue.
Flags: needinfo?(bzbarsky)
So effectively auto-.wrappedJSObject things?  Again, the question is whether this will end up exposing implementation details people do not want to expose.
Flags: needinfo?(bzbarsky)
No reason, overlooked it.
Flags: needinfo?(aciure)
You need to log in before you can comment on or make changes to this bug.