Closed Bug 1008467 Opened 6 years ago Closed 6 years ago

Promise resolution fails with "too much recursion"

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: yury, Assigned: nsm)

References

()

Details

Attachments

(1 file)

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') });
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') });
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...
(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)
(btw, works fine in chrome, and also with polyfill promises enabled https://github.com/mozilla/pdf.js/blob/master/src/shared/util.js#L1063)
Flags: needinfo?(nsm.nikhil)
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?
> 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.
I'm looking at this.
Flags: needinfo?(nsm.nikhil)
> 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...
(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.
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.
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....
yes :)
Assignee: nobody → nsm.nikhil
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?
Attachment #8425117 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/5b90c47a36f0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.