Calling function expression by name via .call() method hits stack limit

REOPENED
Assigned to

Status

()

Core
JavaScript Engine
REOPENED
5 years ago
4 years ago

People

(Reporter: Fedor Indutny, Assigned: dvander)

Tracking

unspecified
mozilla21
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox18 affected, firefox19+ wontfix, firefox20+ fixed, firefox21+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 664900 [details]
1.js

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/536.5 (KHTML, like Gecko) YaBrowser/1.0.1084.5401 Chrome/19.0.1084.5401 Safari/536.5

Steps to reproduce:

I'm trying to run attaching code.


Actual results:

Recursion depth is limited to ~260


Expected results:

Recursion depth should be as it is usual (around 20k-30k).
Status: UNCONFIRMED → NEW
Ever confirmed: true
I just experienced something similar with generators while adding one variable inside a if scope of one of the interpreted bytecode.  Apparently the goto-style error handling cause the compiler to skip the liveness analysis on this function and just accumulate variables on top of each others.

I haven't checked if we have added any local variable to the Interpreter with IonMonkey but I won't be surprised if we did.  An option I used for my other patch was to move temporary usage inside external functions and call these functions.

To check the frame size, One can set a breakpoint on js_ReportOverRecursed and look at the stack pointer of each frame.
See also bug 803182.

/be
Attachment #664900 - Attachment mime type: application/octet-stream → text/plain
Duplicate of this bug: 832580

Comment 4

5 years ago
This has now been reported post-release as bug 832580.

Terrence - bug 803182 only fixed debug builds according to https://bugzilla.mozilla.org/show_bug.cgi?id=832580#c7. Please investigate a low risk fix for FF19. We're only a couple of weeks away from release.
Assignee: general → terrence
tracking-firefox19: --- → +
tracking-firefox20: --- → +
tracking-firefox21: --- → +
Created attachment 707743 [details] [diff] [review]
v0

Try run up at:
https://tbpl.mozilla.org/?tree=Try&rev=3e7d9cb79b06

This is a simple hack which just doubles the (somewhat arbitrary) JS stack limit on all platforms except for Windows, where we are already up against the C stack limit. The purpose of this is simply to get some breathing room until we have time to come up with a better solution.
Attachment #707743 - Flags: review?(nihsanullah)
(Reporter)

Comment 6

5 years ago
Terrence,

How deep will be recursion after your patch applied? As far as I can see you've increased stack size by a factor of 2. But if recursion depth will in a same manner, it'll still be 50 times shallower than in v8 (and I suppose other JS VMs).
Hmm, there must be something else going on here then: in bug 803182 Brendan found that the recursion limit for this test was only 400 in Firefox 3 -- 4x deeper. If v8 is able to do 50x deeper stacks through |.call()|, similar to normal calls in both engines, then we should probably find out why we are so much worse here. Nicolas, do you know what's going on here?

In the meantime, I still think we should take the attached patch as it gets us back to ~TM era capabilities.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Terrence Cole [:terrence] from comment #7)
> Hmm, there must be something else going on here then: in bug 803182 Brendan
> found that the recursion limit for this test was only 400 in Firefox 3 -- 4x
> deeper. If v8 is able to do 50x deeper stacks through |.call()|, similar to
> normal calls in both engines, then we should probably find out why we are so
> much worse here. Nicolas, do you know what's going on here?

The problem is that fun_call is handled as a native function call of fun_call.  The native function call enforce us to push a new Interpreter on the stack every time we evaluate a fun.call in JavaScript.

The same problem arise for fun_call, fun_apply and also for generators.  I don't know what to do for generators, but for fun_apply and fun_call we should use self-hosted functions for calls with a few (< 16) arguments.

function call(obj) {
  var fun = this;
  switch(arguments.length - 1) {
    case 0: return obj.fun();
    case 1: return obj.fun(arguments[0]);
    case 2: return obj.fun(arguments[0], arguments[1]);
    …
    default: return this.buildtin_fun_apply(obj, arguments);
  }
}

self-hosted functions will reuse the StackFrame of the current interpreter instead of adding a new one.  This will likely solved most of the common issues encountered here.

The only tricky part is that currently no JIT provide an optimized version of a self-hosted function.  So JM & Ion will have to be modified to match this new kind of native function.
Flags: needinfo?(nicolas.b.pierron)
Fix typo:

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #8)
> function call(obj) {
>   var fun = this;
>   switch(arguments.length - 1) {
>     case 0: return obj.fun();
    case 1: return obj.fun(arguments[1]);
    case 2: return obj.fun(arguments[1], arguments[2]);
>     …
>     default: return this.buildtin_fun_apply(obj, arguments);
>   }
> }
Comment on attachment 707743 [details] [diff] [review]
v0

On second thought, this is a bit gross and doesn't help much at all, so I'm going to change my mind and advocate against taking this.
Attachment #707743 - Flags: review?(nihsanullah)
Terrence/nbp - what's your recommendation on an action to resolve bugs like bug 832580 in an upcoming release?
(In reply to Alex Keybl [:akeybl] from comment #11)
> Terrence/nbp - what's your recommendation on an action to resolve bugs like
> bug 832580 in an upcoming release?

My recommendation is to do something as describe in comment 18.  Except that this implies adding a way to if a function is THE self-hosted function that we are looking for.

till: Any idea on how to match if a self-hosted function against a specific function, such as JIT can optimize the self-hosted function instead of inlining it?
Flags: needinfo?(tschneidereit)
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #12)
> (In reply to Alex Keybl [:akeybl] from comment #11)
> > Terrence/nbp - what's your recommendation on an action to resolve bugs like
> > bug 832580 in an upcoming release?
> 
> My recommendation is to do something as describe in comment 18.  Except that
> this implies adding a way to if a function is THE self-hosted function that
> we are looking for.
> 
> till: Any idea on how to match if a self-hosted function against a specific
> function, such as JIT can optimize the self-hosted function instead of
> inlining it?

I'll look into this and check if we can somehow make reusing the intrinsic CallFunction mechanism work for Function#call. It certainly won't work for Function#apply, however, as we can't statically know the number of arguments for that.
Flags: needinfo?(tschneidereit)
Clearing ownership since this is an Ion bug and I have no idea how to fix it in that case.
Assignee: terrence → general
Next action with till, it would seem.

Wontfixing for FF19 at this point since we're close to release and aren't close to resolution. We expect the FF18 behavior to carry over.
Assignee: general → tschneidereit
status-firefox18: --- → affected
status-firefox19: --- → wontfix
(Assignee)

Comment 16

5 years ago
Self-hosting special cases of function.call doesn't sound like a robust solution to me. Why did this regress in Fx18?
Assignee: tschneidereit → general
(Assignee)

Comment 17

5 years ago
Created attachment 713198 [details] [diff] [review]
a fix

To avoid the interpreter we just need to run the function in the JIT. The given testcase has an opcode that JM can't compile, but luckily is very easy to implement. This patch brings the test's recursion limit from ~370 to ~36,000 (on a debug build - numbers are actually lower on an opt build).
Assignee: general → dvander
Attachment #707743 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #713198 - Flags: review?(bhackett1024)
Attachment #713198 - Flags: review?(bhackett1024) → review+

Updated

5 years ago
Duplicate of this bug: 840008
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/507316db9c59
https://hg.mozilla.org/mozilla-central/rev/507316db9c59
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox21: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Should we consider uplifting this to Fx20?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Should we consider uplifting this to Fx20?

By looking at the patch, I don't see any restriction for doing so.
Comment on attachment 713198 [details] [diff] [review]
a fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): ??? Might be coming from IonMonkey's landing and the sharing of VM functions with the interpreter.

User impact if declined: JS Stack overflow => exception & bad execution.
Testing completed (on m-c, etc.): yes, 5 days ago.

Risk to taking this patch (and alternatives if risky): This seems extremely low, as it does not introduces either new data / new functions.  And the callee value is supposed to be valid for the current frame.

String or UUID changes made by this patch: N/A
Attachment #713198 - Flags: approval-mozilla-aurora?
Note that you probably want beta approval now, since the merge occurred.
Comment on attachment 713198 [details] [diff] [review]
a fix

[Approval Request Comment]
see comment 23
Attachment #713198 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
status-firefox20: --- → affected
Comment on attachment 713198 [details] [diff] [review]
a fix

Early in FF 20 betas, low risk, approving for branch uplift.
Attachment #713198 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/b3e46a3bea04
status-firefox20: affected → fixed
Depends on: 842300

Updated

5 years ago
Depends on: 845340
(Assignee)

Updated

5 years ago
No longer depends on: 842300

Comment 28

5 years ago
If some of you Stack Trace experts has time could you please take a look at: -
https://bugzilla.mozilla.org/show_bug.cgi?id=856969 
"Too much recursion error with Java LiveConnect Applet"

It's Windows rather than Mac OS and has existed long berfor version 18 but I would really, really appreciate the insight of those familiar with working with "Too much recurssion" bugs in FF.

Cheers Richard Maher

Comment 29

5 years ago
Firefox 21.0, Windows 7
I run this code in firebug's console (1.11.12):

(function(i) {
    i && arguments.callee.call(this, --i);
})(600);

and get "too much recursion" :(
(Reporter)

Comment 30

4 years ago
This bug still hits us in all firefox versions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.