392973 - Remove MAX_INLINE_CALL_COUNT check

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Now that we have stack depth monitoring via JS_SetThreadStackLimit, it is unnecessary and often wrong.

For backward compatible sanity checking, we could leave the check but condition it by JS_SetThreadStackLimit not having been called. However, when bug 339652, the test of cx->interpLevel against MAX_INTERP_LEVEL, was removed we did not preserve such compatibility. Igor argued that a stack overflow crash is no worse than a script error, but a crash is worse than a catchable exception. Comments?

/be

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Now that we have stack depth monitoring via JS_SetThreadStackLimit, it is
> unnecessary and often wrong.

This is wrong, there's no point in the check at all. Inline calls stay in the same js_Interpret activation, and we don't use alloca, so no net stack increase is possible as the counter increments.

Patch next.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

My comments about an exception being nicer to end users who stumble upon a DOS or a buggy web app than a stack overflow crash (which is not exploitable except on a buggy OS, but which is mighty unpleasant to sit through) do apply to the removed cx->interpLevel test, still. If JS_SetThreadStackLimit has not been called, perhaps we should do that MAX_INTERP_LEVEL test. But I'm not restoring it here.

Again, comments welcome.

/be

Comment 3

[Brian Crowder]

Comment on attachment 277467 [details] [diff] [review]
fix

Woops, missed taking the MAX_INTERP_LEVEL stuff out in bug 339652, sorry.  This looks good to me.

Comment 4

[Brendan Eich [:brendan]]

Fixed:

js/src/jsinterp.c 3.366

/be

Comment 5

[Brendan Eich [:brendan]]

Bob, did this really cause bug 393368 ? What buggy OS powered off on account of running out of memory?

Ok, so maybe we need some kind of limit other than heap growth failure...

/be

Comment 6

[Bob Clary [:bc] (inactive)]

I think so, but am not completely positive that this bug was the cause. Make it 90% sure. This certainly fit the resolution of nightly build regression range finding I was using. Fedora Core 6 is my OS. It didn't power off but the machine became completely unresponsive after all memory and swap were eaten. The only way I could recover was by powering off the machine :-)

Comment 7

[Igor Bukanov]

With this bug fixed on my laptop with 1.1 GB under Fedora js1_5/Exceptions/regress-121658.js runs for 20 minutes with extremely heavy swapping before it exits with out-of-memory. This is not suprising given that the test contains:

function f()
{
  ++i;

  // try...catch should catch the "too much recursion" error to ensue
  try
  {
    f();
  }
  catch(e)
  {
  }
}

i=0;
f();

Thus now it only exists when calling malloc to get more memory for stack's arena fails.

Comment 8

[Brendan Eich [:brendan]]

I've taken bug 393368 so would like to verify this bug and fix that one.

/be