Closed Bug 960523 Opened 7 years ago Closed 7 years ago

Too much recursion error starting with Firefox 26

Categories

(Core :: JavaScript Engine, defect)

26 Branch
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: guillaume.smet, Assigned: jandem)

References

()

Details

(Whiteboard: [bugday-20140120])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206152524

Steps to reproduce:

Hi,

Starting with Firefox 26, it seems that the limit for too much recursion error is really lower than before.

It causes a lot of problems in our applications, especially because the Wicket Ajax stack is using a lot of recursion to perform its magic.

Our applications were running fine before the upgrade to Firefox 26.

On my laptop, the following http://jsfiddle.net/m4tCE/ gives me a Too much recursion error in the Firefox console, just after Firefox startup.


Actual results:

too much recursion error


Expected results:

Javascript runs without any error, considering we are not recursing so much.
Not sure which console it is about; can't see any mention of recursion in Web Console, Browser Console, or the terminal (Firefox 26, Nightly 2014-01-20-03-02-02-mozilla-central-firefox-29.0a1.en-US.linux-x86_64).
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Whiteboard: [bugday-20140120]
(In reply to [:Aleksej] from comment #1)
> Not sure which console it is about; can't see any mention of recursion in
> Web Console, Browser Console, or the terminal (Firefox 26, Nightly
> 2014-01-20-03-02-02-mozilla-central-firefox-29.0a1.en-US.linux-x86_64).

The dev console (ctrl+shift+K).

FWIW, it's the Firefox 26 of Ubuntu Precise on i686 platform. We reproduce it on several boxes. We sometimes have to tweak the value a little to get a recursion error but it's perfectly reproducible each time.
Hmm.  On Mac, I get no recursion error even with 50000 as the argument to the function in the fiddle.

Guillaume, this may be specific to the compiler used for your platform or to platform configuration.  Would you be willing to use http://mozilla.github.io/mozregression/ to figure out when the problem first appears for you?  Ideally, that would find the changeset urls for the last nightly without the problem and first nightly with the problem...
Flags: needinfo?(guillaume.smet)
Hi Boris,

Sorry for the delay. I was a bit swamped on other things. I made some bisection to at least have the major version introducing the problem.

This is with the 32 bits binary for Linux officially distributed by Mozilla on ftp.mozilla.org.

* Firefox up to 22: no problem with *5000* iterations on the jsfiddle I provided

* Starting with Firefox 23: recursion problem with *500* recursions reproducible for every version, 27.0.1 included (and it's even lower than that starting with 25: we consistently have the problem at 350 recursions as originally posted in my jsfiddle).

This is with a new profile, no extension, vanilla Firefox on a standard Ubuntu LTS 32 bits.

Note that you really need to start the console before trying even once to reproduce the problem. Once you have the first Too much recursion, the behavior of Firefox is unpredictable and, most of the time, the error doesn't appear in the console anymore.

If you can provide me a little guidance for the mozregression thing, I can take a look. I mostly need the nighly builds dates corresponding to release 22 and 23. But I'm not sure they are still available for mozregression?

Thanks for your feedback.
Flags: needinfo?(guillaume.smet)
Note discussion in bug 966173 for Mac vs Linux differences.  But I can't reproduce with a Linux nightly on the testcase here either (though I'm testing a 64-bit Linux build, in case it matters)...

> I mostly need the nighly builds dates corresponding to release 22 and 23.

According to https://wiki.mozilla.org/RapidRelease/Calendar the 22 nightly cycle started on 2013-02-19 and ended on 2013-04-01.  The 23 cycle started on 2013-04-01 and ended on 2013-05-13.  Those nightlies should certainly still be available.

That drop from 5000 to 500 or 350 really doesn't seem OK, but matches the numbers in bug 966173.  :(
Yes we should fix this somehow; I'll look into it.
Flags: needinfo?(jdemooij)
Thanks Jan.

Feel free to ping me if you need further information. This one is really a blocker for us.

Note: 5000 for 22 and below is just the number for which I stopped my test. I'm pretty sure it was even higher than that.
We have some pretty big C++ stack frames for every recursive call. Tomorrow I'll post patches to make these a bit smaller, that should allow us to push a few hundred extra frames I think. Still not great, but an easy win and something we can backport hopefully.

After that I'll see if we can bring this back to at least 3000 or so.
And the "pretty big C++ stack frames for every recursive call" weren't in Firefox 22?

A few hundreds extra frames would solve our immediate problem I think and, AFAICS, the problem in the other bug report.

Would be nice to bring back the old behavior thought as you suggested.
Hi Jan,

Any progress on this one?

Thanks for your feedback.
(In reply to Guillaume Smet from comment #9)
> And the "pretty big C++ stack frames for every recursive call" weren't in
> Firefox 22?

Firefox 22 had a different JIT (JM) that inlined these calls. Now we do that only in our optimizing JIT (Ion), but it kicks in after ~1000 calls, before we reach the stack limit :(

(In reply to Guillaume Smet from comment #10)
> Any progress on this one?

Sorry, I got sidetracked. Will look into it right now.
Depends on: 982186
This patch adds a Baseline IC stub for fun.call with a scripted function as |this|.

On some fun.call() testcases I tried, it increases the max recursion depth from 440 to 5000-13000 (non-deterministic due to parallel Ion compilation).

Kannan, it would be great if you could review this before the weekend so that we can land this before the merge next Monday. Bug 966173 is the same issue, we really want to get this in FF 30.
Assignee: nobody → jdemooij
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8389776 - Flags: review?(kvijayan)
Flags: needinfo?(jdemooij)
Blocks: 966173
Starting review on it now.
Comment on attachment 8389776 [details] [diff] [review]
Add fun.call stub

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

Looks pretty good.  Can't see any holes in the generated jitcode, which is where my biggest concern generally lies.

r=me with comments addressed.

::: js/src/jit-test/tests/baseline/funcall.js
@@ +1,1 @@
> +function test1() {

Reading this test case, it occurs to me that it might be useful to expose JitOptions such as the useCount thresholds for various optimization tiers to the shell for use in cases like this, so we don't have to build them into the loop constants into these tests with no explanation...

Not sure if that's relevant for this patch, though...  could you make a bug for that and CC me?  It would be a good quick pick up bug or a mentor bug for a new contributor.

::: js/src/jit/BaselineIC.cpp
@@ +7818,5 @@
> +    if (!thisv.isObject() || !thisv.toObject().is<JSFunction>())
> +        return true;
> +    RootedFunction target(cx, &thisv.toObject().as<JSFunction>());
> +
> +    if (target->hasScript() && target->nonLazyScript()->canBaselineCompile() &&

Why can't these two checks simply be |target->hasBaselineScript()|.  Do we really want to attach stubs when the target script hasn't compiled to baseline yet?  We won't take the stub until the target script is compiled anyway, why would we want to attach it early?

::: js/src/jit/BaselineIC.h
@@ +5583,5 @@
>          }
>      };
>  };
>  
> +class ICCall_ScriptedFunCall : public ICMonitoredStub

I should have started doing this with the previous stubs, but maybe we can do it now.  Can you add a small comment to this class explaning in code terms what cases the stub handles?  E.g.  "Handles calls of the form |fun.call(...)| where fun is a scripted function"?

All the different call variants are starting to get confusing.
Comment on attachment 8389776 [details] [diff] [review]
Add fun.call stub

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

Forgot to r+.
Attachment #8389776 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a5dcbe33e5

Thanks for the quick review.

(In reply to Kannan Vijayan [:djvj] from comment #14)
> Why can't these two checks simply be |target->hasBaselineScript()|.  Do we
> really want to attach stubs when the target script hasn't compiled to
> baseline yet?  We won't take the stub until the target script is compiled
> anyway, why would we want to attach it early?

Good question. That's what I had at first, but while writing the tests I noticed something tricky: we were attaching a CallNative stub instead of FunCall stub because the callee's use count was one less than the Baseline threshold. Then we Baseline-compiled the callee and never added a FunCall stub because there was a CallNative one handling everything. Because Baseline compiles pretty much all scripts, it seemed most efficient to attach a FunCall stub in this case, worst case we can't compile the callee and we'll have a FunCall + CallNative stub, that should be fine I think. I added a comment to explain this (should have done that immediately).

> I should have started doing this with the previous stubs, but maybe we can
> do it now.  Can you add a small comment to this class explaning in code
> terms what cases the stub handles?  E.g.  "Handles calls of the form
> |fun.call(...)| where fun is a scripted function"?

Done.
https://hg.mozilla.org/mozilla-central/rev/55a5dcbe33e5
https://hg.mozilla.org/mozilla-central/rev/51da1475de06
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.