1223961 - Firefox becomes non responsive because of a recursive function that uses promises

Status: RESOLVED FIXED

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

Description

[mohamad assar]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

This is a function that triggered the issue on my browser, I commented out the unrelated parts:

function showAlert(text, description, duration) {
            var promise = new Promise(function (resolve) {
                //var el = $('#alert-modal');
                //el.find('.alert-title').html(text);
                //el.find('.alert-description').html(description || '');
                //el.modal('show');
                showAlert('asdaddadad');
                window.setTimeout(function () {
                    showAlert('asdaddadad');
                    //el.modal('hide');
                    resolve();
                }, 2000);
            });

            return promise;
        };


Actual results:

executing this function made the firefox non responsive, a window popped up time to time and I hit Stop Script but nothing happened


Expected results:

hitting stop script should have ended the non responsive state of the browser

Comment 1

[Boris Zbarsky [:bzbarsky]]

This doesn't seem like a promise issue per se.  The real problem is the exponential growth in number of timeouts, since each timeout firing triggers a bunch of new timeouts to be added.

As in, this looks identical to bug 715918 to me.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Note, we might be able to improve things a little bit by using a heap data structure for storing the scheduled timeouts instead of a linked list.

Let me see if I can profile this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So this is moderately interesting.  A profile shows that time on the main thread breaks down as follows:

30% capturing current stack inside the Promise constructor (or more precisely the
    CreateWrapper call there).
30% capturing current stack for the PromiseInit callback in the Promise constructor.  We
    should perhaps try to avoid that, since it's useless: the callback is called
    immediately.  On the other hand, I expect this problem to solve itself once we move to
    self-hosted Promises in SpiderMonkey.
37% under the setTimeout call... capturing the current stack for that callback.

As you will note, that doesn't leave much time for anything else.

I guess we'd run into some other bottleneck at some point with the exponential growth, but ccing the folks who know about those stack captures.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Is this with async stacks enabled or not?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Is this with async stacks enabled or not?

Comment 3 was with async stacks enabled, though I didn't notice the async stack code taking up particularly much time.

Note that if you ignore the promise obfuscation (and additional stack captures it triggers) but keep the exception-catching behavior it produces the function in question is basically this:

  function f() {
    try { f() } catch() {}
    setTimeout(f, 2000);
  }

Which I expect can be more simply rewritten as:

  function f() {
    setTimeout(f, 2000);
    f();
  }

Note that this gives exponential growth in number of timeouts, with generation length 2s and generation size (fanout) constrained by the stack depth we can get to before hitting stack overflow.  That depth is probably a lot less in the Promise verion than in the simpler version where the jit will helpfully inline the recursion for us and give us a fanout in the hundreds of thousands.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Boris suggests we should clear any pending timeouts when "stop script" is clicked.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I believe this should be fixed now that bug 1300659 has landed.  The browser should remain responsive.