Closed Bug 810490 Opened 7 years ago Closed 7 years ago

Constant stack space promise

Categories

(Add-on SDK Graveyard :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: jwalker, Assigned: Paolo)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [Async:P1])

Attachments

(1 file, 3 obsolete files)

The current implementation of promise is good for 'live' but is quite hard to debug. I'd like a version of the promise API with some behavioural differences:

* The ability to ask what promises are outstanding
  This is clearly a performance issue so shouldn't be on by default

* Non-closure-scoped deferred/promise data
  It's currently not possible in a debugger to see anything about a promise
  being debugged. It would be good to see the resolved value, and handlers
  perhaps as _private variables

* Much reduced stack depth
  Debugging promises is made even harder by the large number of stack frames
  created by even trivial operations. Some of the other proposals may be possible
  using some type of a wrapper, but this one wouldn't be

* The ability to switch to an always-async model which is useful for smoking
  out bad assumptions in tests. i.e. what Q does by default

* Default console.error on 'uncaught' error


The logic for having a second implementation is:

* I estimate that the wrapper approach would be a similar if not greater amount
  of code to the second implementation approach

* It's not possible to reduce stack depth using a wrapper

I expect that you don't want to put every use of promises in mozilla-central into 'debug' mode, but more likely want to select promises from file X/file Y.
Assignee: nobody → rFobic
Joe I really would not like to have two promises, and maybe addressing some of these could help ? Please see more details inline

> 
> * The ability to ask what promises are outstanding
>   This is clearly a performance issue so shouldn't be on by default

It looks like we can do weak-refs to a JS values via xpcom, so we could probably use them to store weak refs in the array that will grow and will have to be balanced every now and then. I'll try to look into this, although if we did not removed optional `prototype` argument that was originally there it would have being very simple to add for specific prototypes :( 


> 
> * Non-closure-scoped deferred/promise data
>   It's currently not possible in a debugger to see anything about a promise
>   being debugged. It would be good to see the resolved value, and handlers
>   perhaps as _private variables
> 

The problem with such privets for us are capabilities being exposed, although it this point I'm ready to give up and expose everything and have a version that we
will use that doesn't exposes those things.

>
> * Much reduced stack depth
>   Debugging promises is made even harder by the large number of stack frames
>   created by even trivial operations. Some of the other proposals may be
> possible
>   using some type of a wrapper, but this one wouldn't be
>

I don't see how would you reduce a stack, do you have any suggestions ?

> 
> * The ability to switch to an always-async model which is useful for smoking
>   out bad assumptions in tests. i.e. what Q does by default
> 
> * Default console.error on 'uncaught' error
> 

We could implement `end` that other implementations do and is like `then` with except that it logs errors if error handler is not given and does not returns a
promise back.

> 
> The logic for having a second implementation is:
> 
> * I estimate that the wrapper approach would be a similar if not greater
> amount
>   of code to the second implementation approach
> 
> * It's not possible to reduce stack depth using a wrapper
> 
> I expect that you don't want to put every use of promises in mozilla-central
> into 'debug' mode, but more likely want to select promises from file X/file
> Y.

I think most of the problems can be addressed without introduction of another implementation. If you still think that's not enough I'm not sure how that actually relates to SDK.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> Joe I really would not like to have two promises, and maybe addressing some
> of these could help ? Please see more details inline
> 
> > 
> > * The ability to ask what promises are outstanding
> >   This is clearly a performance issue so shouldn't be on by default
> 
> It looks like we can do weak-refs to a JS values via xpcom, so we could
> probably use them to store weak refs in the array that will grow and will
> have to be balanced every now and then. I'll try to look into this, although
> if we did not removed optional `prototype` argument that was originally
> there it would have being very simple to add for specific prototypes :( 

If we can do this without performance impact, then excellent.

> > * Non-closure-scoped deferred/promise data
> >   It's currently not possible in a debugger to see anything about a promise
> >   being debugged. It would be good to see the resolved value, and handlers
> >   perhaps as _private variables
> 
> The problem with such privets for us are capabilities being exposed,
> although it this point I'm ready to give up and expose everything and have a
> version that we
> will use that doesn't exposes those things.

If we had 2 implementations of promise then this would be easy :) I guess we could have a config switch to store the data in a debuggable place. Fundamentally the requirements of security and debugging ease are in conflict, so some sort of a switch is required here, I think.

> > * Much reduced stack depth
> >   Debugging promises is made even harder by the large number of stack frames
> >   created by even trivial operations. Some of the other proposals may be
> > possible
> >   using some type of a wrapper, but this one wouldn't be
> 
> I don't see how would you reduce a stack, do you have any suggestions ?

From memory, there are a number of wrappers, which could be in-lined. It wouldn't be so elegant, but would help. I wonder also if we need to create sub-promises quite so often. Other promise implementations that I've worked with don't have this issue though, so I'm guessing that it's possible.

> > * The ability to switch to an always-async model which is useful for smoking
> >   out bad assumptions in tests. i.e. what Q does by default
> > 
> > * Default console.error on 'uncaught' error
> 
> We could implement `end` that other implementations do and is like `then`
> with except that it logs errors if error handler is not given and does not
> returns a
> promise back.

I guess that ".end()" is like ".then(null, console.error);" ? In which case, that would help, although I was wanting an implementation in debug mode to just tell you when it was throwing away an error without the need for more code.

> > The logic for having a second implementation is:
> > 
> > * I estimate that the wrapper approach would be a similar if not greater
> > amount
> >   of code to the second implementation approach
> > 
> > * It's not possible to reduce stack depth using a wrapper
> > 
> > I expect that you don't want to put every use of promises in mozilla-central
> > into 'debug' mode, but more likely want to select promises from file X/file
> > Y.
> 
> I think most of the problems can be addressed without introduction of
> another implementation. If you still think that's not enough I'm not sure
> how that actually relates to SDK.

I guess the implementation isn't something I particularly care about. I'm happy either way - so long as we don't use the argument "we can't help your debugging request because that would make it too hard/complex/slow/etc for the 'live' case".
As observed in Bug 845842, the current promise implementation is stack-hungry, and it's also written in fairly archaic style (var, non-strict, module boilerplate).

This is leading us down the road of adding utility functions to hack around its limitations (such as CommonUtils.laterTickResolvingPromise).

I think it would be better to just provide a better promise API, one that already knows how to not blow the stack, and one that we can all commit to improving.

Is there a reason why this bug is under Add-on SDK, rather than starting afresh under Toolkit/Modules? It would seem like a better v1 implementation, focused on perf-related improvements and based on the existing code, would be a fairly trivial project.

Can anyone give me some context that disagrees?
(In reply to Richard Newman [:rnewman] from comment #3)
> As observed in Bug 845842, the current promise implementation is
> stack-hungry, and it's also written in fairly archaic style (var,
> non-strict, module boilerplate).
> 
> This is leading us down the road of adding utility functions to hack around
> its limitations (such as CommonUtils.laterTickResolvingPromise).
> 
> I think it would be better to just provide a better promise API, one that
> already knows how to not blow the stack, and one that we can all commit to
> improving.
> 
> Is there a reason why this bug is under Add-on SDK, rather than starting
> afresh under Toolkit/Modules? It would seem like a better v1 implementation,
> focused on perf-related improvements and based on the existing code, would
> be a fairly trivial project.
> 
> Can anyone give me some context that disagrees?

The promises implementation started as an add-on SDK module which we then made available for all to use. I don't object to starting a new implementation somewhere else but I wish we could just reach some agreement and have a single implementation. Maybe the route is to have a full featured implementation in toolkit that we then wrap in add-on SDK land to only expose the minimal API we want.
Blocks: 852411
(In reply to Dave Townsend (:Mossop) from comment #4)

Thanks for weighing in!

> Maybe the route is to have a full featured
> implementation in toolkit that we then wrap in add-on SDK land to only
> expose the minimal API we want.

That sounds like a good course of action to me.
Requests of the API:
* It should comply with http://promises-aplus.github.com/promises-spec/ which means that promise consumers have a consistent experience
* I'm no expert on what the thinking of the spec people is on async in general, but last time I looked, the Q API seemed to be favored. If this is still the case, and maybe if it isn't then we should look to have something similar.

Q is a big API though, so perhaps we shouldn't take everything.
(In reply to Richard Newman [:rnewman] from comment #5)
> > Maybe the route is to have a full featured
> > implementation in toolkit that we then wrap in add-on SDK land to only
> > expose the minimal API we want.
> 
> That sounds like a good course of action to me.

Me too.

How do we get there? I think that means:
1) Moving the current code as-is to toolkit/modules, and wrap it in the SDK module
2) Adjust the code in toolkit/modules to address bug 842347
3) Decide how much of the adjustments in 2) should be exposed in the SDK-wrapped version

Sounds like the SDK team should be responsible for 1) and 3), and anyone can do 2).

Does that seem like a reasonable plan? Shall we WONTFIX this bug and get separate bugs on file for the these steps?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> (In reply to Richard Newman [:rnewman] from comment #5)
> > > Maybe the route is to have a full featured
> > > implementation in toolkit that we then wrap in add-on SDK land to only
> > > expose the minimal API we want.
> > 
> > That sounds like a good course of action to me.
> 
> Me too.
> 
> How do we get there? I think that means:
> 1) Moving the current code as-is to toolkit/modules, and wrap it in the SDK
> module
> 2) Adjust the code in toolkit/modules to address bug 842347
> 3) Decide how much of the adjustments in 2) should be exposed in the
> SDK-wrapped version
> 
> Sounds like the SDK team should be responsible for 1) and 3), and anyone can
> do 2).
> 
> Does that seem like a reasonable plan? Shall we WONTFIX this bug and get
> separate bugs on file for the these steps?

On step 2: bug 842347 is probably fixed, but I've not had a chance to check yet.

A number of people have noted that the code is hard to read, which is especially a problem as you need to step through it often when debugging. It could be that there are other start points which make life easier. I'd be happy to propose something but it will take me a while to get to it.
Trying to rehash individual issues brought up. Let's have individual bugs per issue so we can iterate over them independently and have priorities set according
to their severity, let's also close this one. More details inline:

(In reply to Joe Walker [:jwalker] from comment #0)
> The current implementation of promise is good for 'live' but is quite hard
> to debug. I'd like a version of the promise API with some behavioural
> differences:
> 
> * The ability to ask what promises are outstanding
>   This is clearly a performance issue so shouldn't be on by default
> 

Is this still relevant if so let's submit a separate bug so we can set priority
based on severity of this.

>
> * Non-closure-scoped deferred/promise data
>   It's currently not possible in a debugger to see anything about a promise
>   being debugged. It would be good to see the resolved value, and handlers
>   perhaps as _private variables
> 

Is this still relevant if so let's submit a separate bug so we can set priority
based on severity of this.

>
> * Much reduced stack depth
>   Debugging promises is made even harder by the large number of stack frames
>   created by even trivial operations. Some of the other proposals may be
> possible
>   using some type of a wrapper, but this one wouldn't be
>

We already have Bug 842347 in place for this. Hopefully it solves the issue,
if not we can iterate more there.

> 
> * The ability to switch to an always-async model which is useful for smoking
>   out bad assumptions in tests. i.e. what Q does by default
>

I have submitted another issue for this Bug 853222 let's agree on the implementation strategy and an assignee. 

>
> * Default console.error on 'uncaught' error
> 

I have submitted Bug 851321 to solve this in a way Q does it.
I am starting to believe, as Joe, that promises are trying to cater to two very different crowds, and that a single implementation cannot please everybody.

Joe, myself and most if not all of the participants of this week's Performance Work Week represent crowd #1 ("platform crowd"?). We deal with existing masses of code that we need to refactor, debug and maintain. We provide APIs that will be used primarily by platform developers. Purity of code is in the "nice to have" column, but we care much more about performance, debugging and auditability. Defending against hostile uses is very much a non-objective.

Irakli (and certainly others) represent crowd #2 ("add-ons crowd"?). I am not quite certain I understand your priorities, but they seem to place foremost code purity and some notion of safety to defend add-ons against each other. Experience seems to indicate that debugging support, for instance, comes much further down the road.

I conclude that we will lose much less time and sleep with two distinct implementations of promise. I suspect that we will want to maintain API compatibility, though.
Irakli was asking for separate bugs. I agree that this is a good idea *if* we are going to tack these requirements onto the jetpack implementation. In bug 853222, I argue against this this.

> Once of the common issues that people have with promise/core is hard-to-follow
> code, so having adding an extra wrapper might make things worse here.
>
> Given that testing and stability are easy (there are plenty of test suites to
> use) I don't have a problem with multiple implementations here so long as we can
> agree on a core API.

Maybe we should resolve that issue first.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> (In reply to Richard Newman [:rnewman] from comment #5)
> > > Maybe the route is to have a full featured
> > > implementation in toolkit that we then wrap in add-on SDK land to only
> > > expose the minimal API we want.
> > 
> > That sounds like a good course of action to me.
> 
> Me too.

So we discussed about this at the Performance Work Week in Paris, and came
up with a solution that solves the problem that already hurt the most in
real-case scenarios, i.e. the recursion occurring when iterating over
promises. Bug 842347 reduced the stack depth, this prevents recursion
entirely using virtually a non-reentrant depth-first walker over the tree
of resolved promises.

In line with what was proposed above, though with a slightly different route,
I'm preparing a simple Toolkit implementation from scratch. It does not
provide the safety of the SDK version yet, but we can easily adapt it to
either provide the required features or allow the SDK to wrap the base API
and provide the features it needs on top of it.

The attached patch is still missing:
 - All the comments and introductory documentation.
 - Execution of the test suite on both the SDK and Toolkit entry points.
 - More efficient JavaScript, where applicable without hindering readability
   (we can certainly already reverse the array of handlers).

I think the basic xpcshell suite that exists now should be the same for
both entry points, to provide the best possible internal consistency.
Whether the suite should live in Toolkit or SDK I don't know.

I'm attaching this first version now, to get early feedback from interested
parties. I'll be away for a few days, then I'll be able to refine and
document the patch as requested, starting from the middle of next week.
Comment on attachment 728206 [details] [diff] [review]
Proof of concept: Alternate implementation of promises, preventing recursion on chaining.

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

::: toolkit/modules/Promise.jsm
@@ +40,5 @@
> +      promise: promise,
> +      resolve: function (aValue) promise._complete(STATUS_RESOLVED, aValue),
> +      reject: function (aReason) promise._complete(STATUS_REJECTED, aReason),
> +    };
> +  },

At the moment, the state is embedded in the instances of |PromiseImpl|. Couldn't the state rather be embedded in the closure and/or in |defer| itself?

@@ +55,5 @@
> +    let promise = new PromiseImpl();
> +    promise._complete(STATUS_REJECTED, aReason);
> +    return promise;
> +  },
> +

I feel that everything below this point should be moved outside of |this.Promise|.
Changing title & assignee to mirror the result of the long e-mail exchange.
Assignee: rFobic → paolo.mozmail
Summary: Add a second promise implementation → Constant stack space promise
Whiteboard: [Async:P1]
Dave, can you please see if this is the right direction, and maybe suggest
someone for detailed review and style guidance?

There is intentionally no documentation and no optimizations, like in the
previous state of the patch, but the module is moved to SDK and the "then"
function is now guaranteed to return before the callbacks are invoked.

One note about module loading: the "promise.js" file can be loaded both as a
JSM and by the SDK module loader. To ensure that we have only one instance of
the "walker" on the main thread, I used another "promise.jsm" module that is
always loaded as a JSM. If you have better alternatives, they're welcome.
Attachment #728206 - Attachment is obsolete: true
Attachment #738638 - Flags: feedback?(dtownsend+bugmail)
Comment on attachment 738638 [details] [diff] [review]
Rough skeleton, with delayed "then" and no recursion

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

Sorry for the long delay here. This looks fantastic, so easy to follow. I'm going to contradict myself a little though, in general I think code can live in either add-on sdk or toolkit, however we're avoiding putting JSMs into add-on sdk as we do various things to allow us to develop sdk code without rebuilding firefox. As this requires a singleton in a jsm I'm going to say that should live in toolkit/modules, should be a simple file move here.

There was one case where I thought for a moment we'd end up not running the walker properly but I think it's correct. Regardless I'd like to see a test for this:

var deferred1 = Promise.defer();
var deferred2 = Promise.defer();

deferred1.resolve();
deferred2.promise.then(foo);
deferred2.resolve();

I can do the final review and it'll be pretty quick since I've been over this pretty well already. Unless there's a big reason not to I'd get comments added to the code then we can get this landed and see whether there is any fall-out and then do optimizations in follow-ups.

::: addon-sdk/source/lib/sdk/core/promise.jsm
@@ +58,5 @@
> +    promise._complete(STATUS_REJECTED, aReason);
> +    return promise;
> +  },
> +
> +  _handlers: [],

The paranoid part of me would like to see these private members in a separate object (PromiseWalker?) that isn't exposed, nothing outside this module calls them right?

@@ +67,5 @@
> +    if (aPromise._status == STATUS_PENDING) {
> +      return;
> +    }
> +
> +    // TODO: Reversing the order in "_handlers" will make this more efficient.

Can you explain this?

@@ +82,5 @@
> +  },
> +
> +  _walkerLoop: function Promise_walkerLoop()
> +  {
> +    while (this._handlers[0]) {

Use this._handlers.length instead?

@@ +145,5 @@
> +    if (this._status != STATUS_PENDING) {
> +      return;
> +    }
> +
> +    if (aStatus == STATUS_RESOLVED && aValue &&

Why only for STATUS_RESOLVED? Is it not possible to reject with a promise?
Attachment #738638 - Flags: feedback?(dtownsend+bugmail) → feedback+
Comment on attachment 738638 [details] [diff] [review]
Rough skeleton, with delayed "then" and no recursion

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

Micro-driveby!

::: addon-sdk/source/lib/sdk/core/promise.jsm
@@ +33,5 @@
> +
> +/**
> + * This object provides the public module functions.
> + */
> +this.Promise = {

Object.freeze({

@@ +34,5 @@
> +/**
> + * This object provides the public module functions.
> + */
> +this.Promise = {
> +  defer: function Promise_defer()

Unless there's something I don't know about, we can lose the Function_names here. Our tools understand methods now.
Please don't forget this:

(In reply to Joe Walker [:jwalker] from comment #6)
> * It should comply with http://promises-aplus.github.com/promises-spec/
> which means that promise consumers have a consistent experience

It has a test suite: https://github.com/promises-aplus/promises-tests
Apparently, getting all the details of promises right is hard.

And just in case you haven't seen this I'll mention https://github.com/tildeio/rsvp.js, "a tiny implementation of Promises/A+".
Work on this bug is proceeding, but not as fast as I hoped. It turns out that
there are several edge cases we indirectly rely upon, in the use of promises
we make in the current mozilla-central code. Three in particular:

1. Session Restore: It is assumed to be initialized synchronously during the
   _delayedStartup call (initialization calls "then" on a promise that turns
   out to be already resolved). It's easy to defer the end of _delayedStartup
   to the next tick, but browser-chrome tests start failing because they make
   assumptions and use executeSoon() instead of better ways to wait for browser
   window initialization. This is easy to fix but there are many such tests.

2. Firefox Health Report: Again, this is a "many tests to fix" case. Various
   events occur that cause data to be reported, and tests may wait for the same
   events (for instance, a DOM mutation or other DOM event). But FHR writes data
   in the background, and we can't know when the operation completes - again,
   tests use executeSoon after receiving the event that triggers FHR.
   Fortunately, FHR has a write queue internally, thus if we change tests to
   trigger a fake write after receiving the event, and wait for it, we know
   for sure that the previous asynchronous write has completed and we can get
   a reliable result.

3. The JavaScript Debugger: This is tricky because it uses promises while
   enqueueing client-server packets, and delayed resolution of "then" may make
   those packets go out of order. This would be easy to fix by making each
   outgoing packet wait on the completion of the previous one, _except_ that
   an outgoing packet handler may trigger a nested event loop, thus blocking
   the queue. Again, this can be fixed with some patience, but well, this
   requires patience and again going through all the failing tests.

At this point, since bug 867864 is waiting on the new module being present to
build upon it, we may want to land an initial version with delayed resolution
and non-recursion disabled (it is a one-line change). We can then do all the
test and implementation changes across the tree and enable the new behavior,
while bug 867864 can already be implemented.

The alternative is landing everything together, and work on bug 867864 later.
By the way, I haven't seen discussion on the newsgroups about that bug yet, so
it might turn out that the other bug is actually already waiting on design.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Nickolay_Ponomarev from comment #18)
> It has a test suite: https://github.com/promises-aplus/promises-tests

Thanks! We should definitely look into running the current version of this suite
on our new implementation, and/or import test cases as appropriate.
(In reply to Paolo Amadini [:paolo] from comment #19)
> Work on this bug is proceeding, but not as fast as I hoped. It turns out that
> there are several edge cases we indirectly rely upon, in the use of promises
> we make in the current mozilla-central code. Three in particular:
> 
> 1. Session Restore: It is assumed to be initialized synchronously during the
>    _delayedStartup call (initialization calls "then" on a promise that turns
>    out to be already resolved). It's easy to defer the end of _delayedStartup
>    to the next tick, but browser-chrome tests start failing because they make
>    assumptions and use executeSoon() instead of better ways to wait for
> browser
>    window initialization. This is easy to fix but there are many such tests.

Can you file a bug and Cc me and ttaubert?

> 2. Firefox Health Report: Again, this is a "many tests to fix" case. Various
>    events occur that cause data to be reported, and tests may wait for the
> same
>    events (for instance, a DOM mutation or other DOM event). But FHR writes
> data
>    in the background, and we can't know when the operation completes - again,
>    tests use executeSoon after receiving the event that triggers FHR.
>    Fortunately, FHR has a write queue internally, thus if we change tests to
>    trigger a fake write after receiving the event, and wait for it, we know
>    for sure that the previous asynchronous write has completed and we can get
>    a reliable result.

Can you file a bug and Cc me and gps?

> 
> 3. The JavaScript Debugger: This is tricky because it uses promises while
>    enqueueing client-server packets, and delayed resolution of "then" may
> make
>    those packets go out of order. This would be easy to fix by making each
>    outgoing packet wait on the completion of the previous one, _except_ that
>    an outgoing packet handler may trigger a nested event loop, thus blocking
>    the queue. Again, this can be fixed with some patience, but well, this
>    requires patience and again going through all the failing tests.

Can you file a bug and Cc me and jwaler?
(In reply to David Rajchenbach Teller [:Yoric] from comment #21)

I also have work-in-progress for all the three cases, so if we decide to land the
new module before those changes, I'll definitely do the work of splitting the
current patch into separate pieces each with its own bug.
I reckon the FHR issues are mostly isolated to the mochitests, not the FHR core or FHR's xpcshell tests. There may be 1 or 2 stragglers in FHR's core code and xpcshell tests, sure, but I know there are sketchy async-isms all over the mochitests. Sadly, reliance on executeSoon() and friends is a known shortcoming of how mochitests are written. Hopefully Mike de Boer's work on a new async test harness will eventually reduce the number of these poor practices.
In the current patch the onResolved functions will not be executed in order if there is a call to resolve() in between the then()s. I'd expect them to be executed in order (and the promise a+ spec recommends this).

Example:
var deferred = Promise.defer();
deferred.promise.then(function() { console.log('1'); });
deferred.resolve();
deferred.promise.then(function() { console.log('2'); });

Would output:
2
1

Simple fix:
Change:
this._handlers = aPromise._handlers.concat(this._handlers);
To:
this._handlers = this._handlers.concat(aPromise._handlers);
(In reply to Paolo Amadini [:paolo] from comment #19)
> At this point, since bug 867864 is waiting on the new module being present to
> build upon it, we may want to land an initial version with delayed resolution
> and non-recursion disabled (it is a one-line change). We can then do all the
> test and implementation changes across the tree and enable the new behavior,
> while bug 867864 can already be implemented.

Sounds good to me.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Brendan Dahl from comment #24)
> In the current patch the onResolved functions will not be executed in order
> if there is a call to resolve() in between the then()s. I'd expect them to
> be executed in order (and the promise a+ spec recommends this).

Yeah, I noticed this while re-reading the implementation and the specification.

> Simple fix:
> Change:
> this._handlers = aPromise._handlers.concat(this._handlers);
> To:
> this._handlers = this._handlers.concat(aPromise._handlers);

This changes the walker to be breadth-first, but it's also the simplest way to
implement the correct order, so I'll go for it. The only thing to be aware of:

resolvedPromise.then(function() { console.log('Doing 1'); throw new Error(); });
               .then(null, function() { console.log('Error'); });
resolvedPromise.then(function() { console.log('Doing 2'); });

Would output:

Doing 1
Doing 2
Error

That's correct according to Promises/A+, and also it's sufficiently infrequent
to have multiple handlers on the same promise that I think it is fine.
Attached patch Toolkit module (obsolete) — Splinter Review
So, the changes I did to the addon-sdk version removed the error checking
code, so they were not ready for going live yet.

To prevent further delays, I finished the Toolkit module and moved the
xpcshell tests from the addon-sdk directory to Toolkit (I left an empty
moz.build in place, not sure how and if I should remove it).

In fact, the Add-on SDK implementation already has its own separate test
suite, located in a file called "test-promise.js" that executes with the
other SDK tests, so a duplicate under addon-sdk is unneeded.

This patch can already land, and then either error reporting could be
implemented properly in Toolkit before switching, or we could find a way to
preserve error reporting in the wrapped SDK version. I'll need help both
with this task and with understanding how the multiple module boilerplate
in the SDK version works.

Most of your previous review comments should now be explained in the code.

(In reply to Dave Townsend (:Mossop) from comment #16)
> There was one case where I thought for a moment we'd end up not running the
> walker properly but I think it's correct. Regardless I'd like to see a test
> for this:
> 
> var deferred1 = Promise.defer();
> var deferred2 = Promise.defer();
> 
> deferred1.resolve();
> deferred2.promise.then(foo);
> deferred2.resolve();

I've added a test for comment 24, would you like to see this one also?
Attachment #738638 - Attachment is obsolete: true
Attachment #750346 - Flags: review?(dtownsend+bugmail)
Comment on attachment 750346 [details] [diff] [review]
Toolkit module

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

This only thing I dislike about this patch is how much internal state it exposes, so this passes review but fails superreview I guess. We try to avoid this where possible because people will abuse it and while it's nice to say that they are asking for trouble and we shouldn't worry about breaking them the truth is that history has shown that often we do have to worry about breaking them and it can hurt our ability to make changes in the future. Sometimes we have no choice but to expose this stuff anyway but in this case I've pointed out a few simple changes that hide all the internals of PromiseImpl from callers. With those changes this will be a quick r+

::: toolkit/modules/Promise.jsm
@@ +118,5 @@
> +   *         properties.  See the Deferred documentation for details.
> +   */
> +  defer: function Promise_defer()
> +  {
> +    return new Deferred();

The Deferred object isn't really necessary here and exposes the internals unnecessarily:

let promise = new PromiseImpl();
return {
  promise: { then: promise.then.bind(promise) },
  resolve: promise._complete.bind(promise, STATUS_RESOLVED),
  reject: promise._complete.bind(promise, STATUS_REJECTED)
};

@@ +137,5 @@
> +  resolve: function Promise_resolve(aValue)
> +  {
> +    let promise = new PromiseImpl();
> +    promise._complete(STATUS_RESOLVED, aValue);
> +    return promise;

There's no need for this to rely on the internals:

let deferred = this.defer();
deferred.resolve(aValue);
return deferred.promise;

@@ +158,5 @@
> +  reject: function Promise_reject(aReason)
> +  {
> +    let promise = new PromiseImpl();
> +    promise._complete(STATUS_REJECTED, aReason);
> +    return promise;

Ditto

@@ +200,5 @@
> +
> +    // Schedule the walker loop on the next tick of the event loop.
> +    if (!this._walkerLoopScheduled) {
> +      this._walkerLoopScheduled = true;
> +      Services.tm.currentThread.dispatch(this._walkerLoop,

Use bind() here so we can make |this| correct in _walkerLoop

@@ +368,5 @@
> +    if (this._status != STATUS_PENDING) {
> +      PromiseWalker._schedulePromise(this);
> +    }
> +
> +    return handler.nextPromise;

return { then: handler.nextPromise.then.bind(handler.nextPromise) }
Attachment #750346 - Flags: review?(dtownsend+bugmail) → review-
(In reply to Dave Townsend (:Mossop) from comment #28)

The force behind the choice of putting internal state directly on the Promise
object returned to the consumers is debuggability (second bullet point in
comment 0). If the promise state is wrapped instead, there is currently no way
to see it in the debugger.

But I also see your point, making tampering with promise state as easy as
writing "myPromise._status = 2;" is definitely calling for trouble.

A middle ground that I think would allow debuggability, while still protecting
us from all but the most extreme techniques to access internals, is to emulate
something similar to a "private name" where state is actually stored:

// At the file level.
const privateName = "{private:" + Math.floor(Math.random() * 100) + "}";

// Inside internal functions.
promise[privateName].status = STATUS_REJECTED;

This way, state would still be visible in a debugger after expanding the
"{private:##}" object. Or, we could use separate private names like
"{status:##}" and "{value:##}", thus saving one more object creation and
making those properties visible directly on the promise. What do you think?

> ::: toolkit/modules/Promise.jsm
> @@ +118,5 @@
> > +   *         properties.  See the Deferred documentation for details.
> > +   */
> > +  defer: function Promise_defer()
> > +  {
> > +    return new Deferred();
> 
> The Deferred object isn't really necessary here

I forgot to mention that the Deferred object exists only for the sake of
JavaDoc-style documentation, because describing everything in the "defer"
function was much less intelligible.
Flags: needinfo?(dtownsend+bugmail)
This patch implements the debuggable solution.

You can try the debuggability feature by running this in the Browser Console:

  Cu.import("resource://gre/modules/Promise.jsm"); let d = Promise.defer(); d;

Clicking the object link will open the inspection window. If you do:

  d.resolve("value");

You can click on the object link again to see the changed state and value.
Attachment #750346 - Attachment is obsolete: true
Attachment #752640 - Flags: review?(dtownsend+bugmail)
Flags: needinfo?(dtownsend+bugmail)
Promises are now documented on MDN, optimistically assuming that the new module
conforming to the Promises/A+ proposal is landing soon in Gecko 25.

The following link points to the landing page for consumers of the Promise object:

https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

The link above is referenced by the documentation of functions returning promises,
like OS.File for the main thread. Instead, the overall module introduction and the
global function documentation can be found here:

https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm
Comment on attachment 752640 [details] [diff] [review]
Promise state inaccessible but visible in the dubugger

This looks ok for now but we should define a real inspectable API (ala inspectState) and replace the random property names with that a.s.a.p.
Attachment #752640 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/87bbe2a5b08a
Target Milestone: --- → mozilla24
(In reply to Dave Townsend (:Mossop) from comment #32)
> This looks ok for now but we should define a real inspectable API (ala
> inspectState) and replace the random property names with that a.s.a.p.

A debugger-specific API to make the properties visible in the inspection window
was also suggested. If I understand correctly, inspectState has a slightly
different use, as it would allow state to be inspected from code as well,
and (being a function requiring an explicit call to return a frozen object)
would make inspection in the debugger just a bit more difficult.
https://hg.mozilla.org/mozilla-central/rev/87bbe2a5b08a
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 881047
Blocks: 881049
Blocks: 881050
(In reply to David Rajchenbach Teller [:Yoric] from comment #21)

I just filed the bugs to convert all the consumers to the new implementation.
You need to log in before you can comment on or make changes to this bug.