Remove MAX_INLINE_CALL_COUNT check

VERIFIED FIXED in mozilla1.9alpha8

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

Trunk
mozilla1.9alpha8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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
(Assignee)

Comment 1

11 years ago
(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
(Assignee)

Updated

11 years ago
Priority: -- → P1
(Assignee)

Comment 2

11 years ago
Created attachment 277467 [details] [diff] [review]
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
Attachment #277467 - Flags: review?(crowder)

Comment 3

11 years ago
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+
(Assignee)

Updated

11 years ago
Attachment #277467 - Flags: approval1.9+
(Assignee)

Comment 4

11 years ago
Fixed:

js/src/jsinterp.c 3.366

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 393368

Updated

11 years ago
Flags: in-testsuite-
(Assignee)

Comment 5

11 years ago
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

11 years ago
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

11 years ago
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.
(Assignee)

Comment 8

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

/be
Status: RESOLVED → VERIFIED
Depends on: 398086

Updated

10 years ago
Blocks: 349638

Updated

10 years ago
Blocks: 424683
You need to log in before you can comment on or make changes to this bug.