1008467 - Promise resolution fails with "too much recursion"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Yury Delendik (:yury)]

The following code fails with "too much recursion"


var k = 3000;
Promise.resolve().then(function next() {
  k--;
  if (k > 0) return Promise.resolve().then(next);
}).then(function () { console.log('ok') });

Comment 1

[Yury Delendik (:yury)]

As workaround using atm:

var k = 30000;
new Promise(function next(resolve, reject) {
  k--;
  if (k > 0) {
    Promise.resolve().then(function () { next(resolve, reject); }, reject);
    return;
  }
  resolve();
}).then(function () { console.log('ok') });

Comment 2

[Boris Zbarsky [:bzbarsky]]

So basically the recursion looks like this:

#68 0x000000010606645b in mozilla::dom::Promise::RunTask (this=0x12a7a50a0) at Promise.cpp:882
#69 0x0000000106066ddd in mozilla::dom::Promise::RunResolveTask (this=0x12a7a50a0, aValue={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5f52d690}, aState=mozilla::dom::Promise::Resolved, aAsynchronous=mozilla::dom::Promise::SyncTask) at Promise.cpp:1106
#70 0x00000001060635bd in mozilla::dom::Promise::ResolveInternal (this=0x12a7a50a0, aCx=0x11aae3650, aValue={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5f52d690}, aAsynchronous=mozilla::dom::Promise::SyncTask) at Promise.cpp:1041
#71 0x0000000106062c36 in mozilla::dom::Promise::ThenableResolverCommon (aCx=0x11aae3650, aTask=0, aArgc=1, aVp=0x7fff5f52d680) at Promise.cpp:378
#72 0x000000010606364a in mozilla::dom::Promise::JSCallbackThenableResolver (aCx=0x11aae3650, aArgc=1, aVp=0x7fff5f52d680) at Promise.cpp:389
#73 0x0000000102ac7215 in js::CallJSNative (cx=0x11aae3650, native=0x106063620 <mozilla::dom::Promise::JSCallbackThenableResolver(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5f52d550) at jscntxtinlines.h:239
#74 0x0000000102a9ee53 in js::Invoke (cx=0x11aae3650, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x7fff5f52d690}, <No data fields>}, argc_ = 1}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:475
#75 0x0000000102a9f7a0 in js::Invoke (cx=0x11aae3650, thisv=@0x102be93a0, fval=@0x7fff5f52d8b0, argc=1, argv=0x7fff5f52d928, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5f52d998}) at Interpreter.cpp:531
#76 0x000000010284c164 in JS::Call (cx=0x11aae3650, thisv={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x102be93a0}, fval={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5f52d8b0}, args=@0x7fff5f52d870, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5f52d998}) at jsapi.cpp:5206
#77 0x00000001054712a6 in mozilla::dom::AnyCallback::Call (this=0x115072190, cx=0x11aae3650, aThisVal={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x102be93a0}, value={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5f52df78}, aRv=@0x7fff5f52df30) at PromiseBinding.cpp:77
#78 0x00000001060713f4 in mozilla::dom::AnyCallback::Call (this=0x115072190, value={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5f52df78}, aRv=@0x7fff5f52df30, aExceptionHandling=mozilla::dom::CallbackObject::eRethrowExceptions) at PromiseBinding.h:127
#79 0x000000010606f687 in mozilla::dom::WrapperPromiseCallback::Call (this=0x1150722b0, aCx=0x11aae3650, aValue={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5f52e0a8}) at PromiseCallback.cpp:223
#80 0x000000010606645b in mozilla::dom::Promise::RunTask (this=0x12a7a51c0) at Promise.cpp:882

Nikhil, could we do some of that stuff somewhat async, or would that make things too slow in terms of having to hit the event loop too much?  I'm having a hard time figuring out what the code in comment 0 is actually trying to do and whether it would just hit the recursion limit the same way if it were written without promises involved...

Comment 3

[Yury Delendik (:yury)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> I'm having a hard time figuring out what the code in comment 0 is actually
> trying to do and whether it would just hit the recursion limit the same way
> if it were written without promises involved...

The code is just a minimal test case; original code did something like:

var item;
while ((item = readItem())) {
  if (item.needAsync) {
    wait_for_result(runAsync(item));
  }
}

(and full patch can be found at https://github.com/yurydelendik/pdf.js/commit/b077edadae5fe1855ec9bd9d7053c252eda651f8?w=1#diff-0b94c2e77a5259f7a728122fdbf9f46aR562)

Comment 4

[Yury Delendik (:yury)]

(btw, works fine in chrome, and also with polyfill promises enabled https://github.com/mozilla/pdf.js/blob/master/src/shared/util.js#L1063)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, I see part of why I was confused now; I was trying to match up the code from comment 1 and the stack from running the code in comment 0.  ;)

But I'm still not quite following what's going on in comment 0, much less the actual testcase from comment 3.

So we create a promise.  We resolve it with undefined.  We call then() on it with a function.

When we then async land in RunTask we would invoke this function.  The function creates a promise, resolves it with undefined, calls then() on it, and returns the resulting promise etc.

How do we end up in a situation where we resolve a promise with a thenable, which is what the recursion is showing?

Comment 6

[Yury Delendik (:yury)]

> So we create a promise.  We resolve it with undefined. 

That just creates a resolved promise, nothing more. The same is below.

> We call then() on it with a function...  The function creates a promise, resolves it with undefined, calls then() on it, and returns the resulting promise etc.

then(), of the original promise or promise of the next function, returns a promise which will be resolved when 1) function will return some result, which is not a promise; 2) or the promise/thenable returned by the function is resolved. See http://promises-aplus.github.io/promises-spec/, corresponding sections 2.3.3 and 2.3.2.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I'm looking at this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> then(), of the original promise or promise of the next function, returns a promise which
> will be resolved when 1) function will return some result, which is not a promise

I don't see how.  As far as I can tell, then() returns a promise that is resolved when the promise then() was called on is resolved...

Comment 9

[Yury Delendik (:yury)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > then(), of the original promise or promise of the next function, returns a promise which
> > will be resolved when 1) function will return some result, which is not a promise
> 
> I don't see how.  As far as I can tell, then() returns a promise that is
> resolved when the promise then() was called on is resolved...

It will return promise first 3000 times, after that it will return undefined. The code in comment 0 works if k is e.g. 300.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, I see what happens.  Resolving the promise calls the function passed to then(), which returns a promise.  So then the promise that's returned by then() ends up waiting on that promise to be resolved, which is where our long chain of promises comes from.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Or more precisely, the return value of then() is resolved with the return value of the function passed to then(), which in this case is itself a promise....

Comment 12

[Yury Delendik (:yury)]

yes :)

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Callbacks to then() resolve Promise asynchronously.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8425117 [details] [diff] [review]
Callbacks to then() resolve Promise asynchronously.

r=me

Can you please file a followup to do all this stuff off some sort of lightweight microtask queue, not the main event queue, if we don't have one already?

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b90c47a36f0

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b90c47a36f0