Closed Bug 1223961 Opened 9 years ago Closed 8 years ago

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

Categories

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

42 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: m.assar2002, Unassigned)

References

Details

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
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.
Depends on: 715918
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.
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.
Is this with async stacks enabled or not?
> 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.
Boris suggests we should clear any pending timeouts when "stop script" is clicked.
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer depends on: 1300118
I believe this should be fixed now that bug 1300659 has landed. The browser should remain responsive.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.