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

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Fedor Indutny]

Attachment: 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).

Comment 1

[Nicolas B. Pierron [:nbp]]

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.

Comment 2

[Brendan Eich [:brendan]]

See also bug 803182.

/be

Comment 4

[Alex Keybl [:akeybl]]

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.

Comment 5

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 6

[Fedor Indutny]

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).

Comment 7

[Terrence Cole [:terrence]]

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.

Comment 8

[Nicolas B. Pierron [:nbp]]

(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.

Comment 9

[Nicolas B. Pierron [:nbp]]

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 10

[Terrence Cole [:terrence]]

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.

Comment 11

[Alex Keybl [:akeybl]]

Terrence/nbp - what's your recommendation on an action to resolve bugs like bug 832580 in an upcoming release?

Comment 12

[Nicolas B. Pierron [:nbp]]

(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?

Comment 13

[Till Schneidereit [:till]]

(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.

Comment 14

[Terrence Cole [:terrence]]

Clearing ownership since this is an Ion bug and I have no idea how to fix it in that case.

Comment 15

[Alex Keybl [:akeybl]]

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.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Self-hosting special cases of function.call doesn't sound like a robust solution to me. Why did this regress in Fx18?

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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).

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/507316db9c59

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/507316db9c59

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Should we consider uplifting this to Fx20?

Comment 22

[Nicolas B. Pierron [:nbp]]

(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 23

[Nicolas B. Pierron [:nbp]]

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

Comment 24

[Josh Matthews [:jdm]]

Note that you probably want beta approval now, since the merge occurred.

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 713198 [details] [diff] [review]
a fix

[Approval Request Comment]
see comment 23

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 713198 [details] [diff] [review]
a fix

Early in FF 20 betas, low risk, approving for branch uplift.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b3e46a3bea04

Comment 28

[Richard Maher]

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

[Vyacheslav Aristov]

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" :(

Comment 30

[Fedor Indutny]

This bug still hits us in all firefox versions.

Comment 31

[André Bargull [:anba]]

The low recursion limit (570 frames) now only applies for the case when only the interpreter runs (i.e. when even the baseline-interpreter is disabled). But since that configuration is unlikely to matter in practice, I'm resolving this issue as WFM.