648088 - Avoid nested invocations after the stack has been exhausted

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Attachment: Suppress debugErrorHook for too much recursion

Bug 643360 deals with the case where a control flow cycle from JS -> JSD -> JS is created (there's a rogue toString that triggers an exception that triggers a JSD callback to handle the exception etc.), causing stack exhaustion. The stack exhaustion itself triggers an exception that gets caught again by JSD. This will sometimes crash the browser.

It seems incorrect to invoke callbacks that could conceivably reenter JS when the stack is known to already be exhausted. The specific case here is the debugHooks exception handler (debugErrorHook); I'm not sure if there are others.

The complication is that the semantics of debugErrorHook say that it can override whether or not an error gets reported, so simply omitting the call would change those semantics in this situation. I won't cry over that.

I'm attaching a simple-minded patch to suppress the callback hook only for js_ReportOverRecursed, which may not be the right answer. Especially since a debugErrorHook user now has no way to report this particular error.

Comment 1

[David Mandelin [:dmandelin]]

Comment on attachment 524236 [details] [diff] [review]
Suppress debugErrorHook for too much recursion

The patch itself seems fine. RAII would be nice, but it's such a small thing I don't think it matters.

On the approach, it seems to me the right general idea would be to detect recursion happening in this case and stop it then. I think any such approach is likely to have at least some false positives (i.e., suppress when the debug hook is called even if unbounded recursion is not actually happening). So it seems reasonable to start with something simple, like this patch, and try to be smarter only if we find out we need to.

Comment 2

[timeless]

So, the problem is that jsd is really the debugger (well, its proxy), and deciding whether or not to do something should really be the debugger's choice.

I think what we need is for jsd (or any other debugger) to quickly look at the error to see if it's recursion. the other thing we need to know is how much space we have.

Infinite recursion errors are actually common, and debugging them is incredibly important -- look how unfortunate it was that the debugger in question couldn't be debugged so it could identify and fix the problem!

probably jsd needs to have a thread local recursion count and when that exceeds <x [default=1]> it should stop sending them to its client.

if you're familiar w/ jsdb, it has 3 levels of debuggers, each is able to handle an error, so if the first script (not a debugger) triggers an exception, that's sent to the first debugger, and if that in turn triggers an exception, that is sent to *its* debugger (#2), and when that messes up, you get one last chance.

the <default=1> thing i'm describing is roughly jsdb w/o its second and third debuggers. if someone would want to debug the debugger itself, they'd have to set the pref to e.g. 2 or 3.

offhand, i'd vote against this solution (not in favor of the patch i've posted to the other bug either, but in favor of what i've outlined here).

Comment 3

[Steve Fink [:sfink] [:s:]]

JSD is gone.