Nika Layzell [:nika] (ni? for response) Assignee
	Description • 6 years ago

Follow-up to bug 1455217. After this patch, promises can only be passed through the explicit promise type.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 6 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 6 years ago

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...

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 5 • 6 years ago

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*.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 6 years ago

Comment on attachment 8972176 [details] [diff] [review]
Part 2: Use native refcounting for Promises

Makes a lot more sense now.  ;)

r=me

	Pulsebot
	Comment 7 • 6 years ago

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

	Natalia Csoregi [:nataliaCs]
	Comment 8 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/51cdd0595094
https://hg.mozilla.org/mozilla-central/rev/0b5c103fcb70

	Pulsebot
	Comment 9 • 6 years ago

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

	Andrei Ciure[:aciure]
	Comment 10 • 6 years ago

Backed out 2 changesets (bug 1458043) for dom/presentation/tests/mochitest/test_presentation_1ua_connection_wentaway_inproc.html failures a=backout

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=a9ff6f58c296e9616e070017ba452d23c3f0bf05&group_state=expanded&selectedJob=178842021&filter-searchStr=Linux+debug+Mochitests+test-linux32%2Fdebug-mochitest-6+M%286%29

log: https://treeherder.mozilla.org/logviewer.html#?job_id=178842021&repo=autoland

	Ed Lee :Mardak
	Comment 11 • 6 years ago

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?

	Ed Lee :Mardak
	Comment 12 • 6 years ago

nanj, perhaps saving on one level object wrapping:

notifyObserver({ wrappedJSObject: new Promise(…) }, …)

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 6 years ago

> Backed out 2 changesets (bug 1458043)

Is there a reason this bug wasn't reopened?

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 6 years ago

> 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...

	Ed Lee :Mardak
	Comment 15 • 6 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 6 years ago

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....

	Dan Mosedale (:dmosedale, :dmose)
	Comment 17 • 6 years ago

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...

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 6 years ago

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).

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 19 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 6 years ago

So effectively auto-.wrappedJSObject things?  Again, the question is whether this will end up exposing implementation details people do not want to expose.

	Andrei Ciure[:aciure]
	Comment 21 • 6 years ago

No reason, overlooked it.

	Pulsebot
	Comment 23 • 4 years ago

Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28af275956bd
Stop converting promises to nsISupports in xpconnect, r=kmag