Closed Bug 392973 Opened 17 years ago Closed 17 years ago

Remove MAX_INLINE_CALL_COUNT check

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha8

People

(Reporter: brendan, Assigned: brendan)

References

Details

Attachments

(1 file)

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
(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
Status: NEW → ASSIGNED
Priority: -- → P1
Attached patch fixSplinter Review
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
Attachment #277467 - Flags: review?(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.
Attachment #277467 - Flags: review?(crowder) → review+
Attachment #277467 - Flags: approval1.9+
Fixed:

js/src/jsinterp.c 3.366

/be
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 393368
Flags: in-testsuite-
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
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 :-)
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.
I've taken bug 393368 so would like to verify this bug and fix that one.

/be
Status: RESOLVED → VERIFIED
Depends on: 398086
Blocks: 349638
Blocks: 424683
You need to log in before you can comment on or make changes to this bug.