Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
It seems like inlineCallCount is subsumed by scriptStackQuota in its role of preventing DoS by recursion.  inlineCallCount is also used in a few places to check "are there any more inline frames?", but perhaps these uses can be replaced by |cx->fp->down == NULL| (what about js_Execute with down != NULL?).
fp->down links across js_Interpret activations, so while null down does mean no more inline (or "out of line") frames, non-null does not tell you anything about inlineCallCount != 0.

I should have remembered the obvious non-DOS-prevention aspect here, of routing control flow out of js_Interpret when inlineCallCount decrements to 0 on return from JSOP_STOP and JSOP_RETURN/JSOP_RETRVAL (latter two if js_Invoke is calling js_Interpret). These need something tantamount to inlineCallCount, but a frame flag would suffice, maybe.

/be

Comment 2

9 years ago
(In reply to comment #0)
> It seems like inlineCallCount is subsumed by scriptStackQuota in its role of
> preventing DoS by recursion.

At some point js_Interpret has relied only on scriptStackQuota for infinite recursion detection. But that have not worked well. The problem is that it takes some time before a bogus recursion exhausts scriptStackQuota leading to usability problems for web developers. I.e. inlineCallCount allows to detect much faster a run-away recursion.
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> The problem is that it
> takes some time before a bogus recursion exhausts scriptStackQuota leading to
> usability problems for web developers. I.e. inlineCallCount allows to detect
> much faster a run-away recursion.

Just so I can understand: is the issue when the recursive function does a lot of computation before recursing?  Like "function f() { expensive(); f() }".  Because a simple "function f() { f() }" seems to error out instantly with/without the inlineCallCount check.

Comment 4

9 years ago
(In reply to comment #3)
> Just so I can understand: is the issue when the recursive function does a lot
> of computation before recursing?

See bug 424683 for details
(Assignee)

Comment 5

9 years ago
Ah, thanks.  It looks like the JS_MAX_INLINE_CALL_COUNT test can't go then.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 6

7 years ago
Fast forward in time and now there is a max stack depth (4MB) and much faster calls (even from the interpreter).  With the patch I'll attach shortly, we can hit the end of the stack and throw in 28ms for an interp-only opt build and 17ms with the methodjit.  Even a debug interp-only shell only takes 138ms.

To make this an even more obvious choice: measuring the max recursion depth before an exception is thrown, while we bottom out at 3000-8000, all other engines (ie9, jsc, v8, opera) go to at least 20,000.  With the patch, we bottom out at 58,000 and win the who-goes-deepest recursion game.

This was motivated by bug 653785 and the never ending sadness that is the methodjit stackLimit heuristic.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 7

7 years ago
Created attachment 535801 [details] [diff] [review]
kill inlineCallCount and the stackLimit quota hack

Two notes:
 1) currently, fp->pc(cx) is O(n) where n is the distance from fp to the top of the top of the stack. In a few cold paths (error handling), we iterate down the stack and call fp->pc, which is quadratic.  Increasing the max n from 8000 to 50000 is quite noticeable (milliseconds turn to seconds), so I switched these sites to use FrameRegsIter, changed the name of 'pc' to 'pcQuadratic', and audited where we use it.
 2) some of the jit-tests intend to do infinite recursion and do something slow on each step. to keep jit-tests fast, I made their frames fatter (via apply).

Also, I couldn't help myself with the Helter Skelter reference on the pcQuadratic comment.  Let me know if its too weird.
Assignee: general → luke
Status: REOPENED → ASSIGNED
Attachment #535801 - Flags: review?(dvander)
(Assignee)

Comment 8

7 years ago
Created attachment 536775 [details] [diff] [review]
kill inlineCallCount and the stackLimit quota hack

Remove 'next' default parameter of StackFrame::pcQuadratic (all uses replaced by FrameRegsIter).
Attachment #535801 - Attachment is obsolete: true
Attachment #536775 - Flags: review?(dvander)
Attachment #535801 - Flags: review?(dvander)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 653436
Comment on attachment 536775 [details] [diff] [review]
kill inlineCallCount and the stackLimit quota hack

Review of attachment 536775 [details] [diff] [review]:
-----------------------------------------------------------------

Wow it's good to see that go away.
Attachment #536775 - Flags: review?(dvander) → review+
(Assignee)

Comment 11

7 years ago
http://hg.mozilla.org/tracemonkey/rev/abd2dcd555f4

and http://hg.mozilla.org/tracemonkey/rev/c72fed47c034 to dial down the recursion in test-apply-many-args.js.
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

7 years ago
Duplicate of this bug: 653785
http://hg.mozilla.org/mozilla-central/rev/abd2dcd555f4
http://hg.mozilla.org/mozilla-central/rev/c72fed47c034
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 696018
You need to log in before you can comment on or make changes to this bug.