Closed Bug 1227035 Opened 4 years ago Closed 4 years ago

Firefox 42.0 (64 bit only) fails with "too much recursion" error

Categories

(Core :: JavaScript Engine, defect, critical)

48 Branch
x86_64
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- verified
firefox47 --- verified
firefox48 --- verified
firefox-esr45 --- fixed

People

(Reporter: ziborov.sasha, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.86 Safari/537.36

Steps to reproduce:

I reproduced this issue in Firefox 42 x64 on Windows 10 too. Also I checked on Nightly 45.0a1 (2015-11-19) and got the same result. Here is an example to reproduce the issue: http://jsfiddle.net/tLun99kb/show/

Note, that Firefox 42.0 32 bit works correctly without any errors.


Actual results:

The "too much recursion" error occurs in Firefox 64 bit.


Expected results:

Firefox 42.0 64 bit should work work as Firefox 42.0 32 bit.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Severity: normal → critical
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
"Too much recursion" typically means we ran out of stack space.  Stacks are pretty small on Windows, and 64-bit builds can end up using more stack space than 32-bit ones (if nothing else because all the pointers are twice as big).

That said, it's possible that we're doing something particularly dumb on 64-bit Windows here... Jan, do you have time to take a look?
Flags: needinfo?(jdemooij)
This is ObjectGroup::useSingletonForClone biting us again; see also bug 1114347.

We give small functions that use this/apply/arguments a singleton type and clone the scripts, to improve type information in certain cases. This hurts when we call such functions recursively though: because their scripts are clones, they all have a different use count and we spend a lot of time in the interpreter. JIT -> Interpreter -> JIT round trips use a ton of stack space.

One heuristic we could add is checking for a return statement; that probably means the function is not a constructor.. It'd be really nice to replace useSingletonForClone with something more generic though.
Hi Jan, Boris.


Are any news on this issue? We see it in Firefox 42.0 64 bit, too.
We see this issue in Firefox 42.0 64 bit, too.
Sorry for flood.
Hi Alexander. Sorry for the delay, I'll try to get to it either this week or next week.
Attached patch Patch (obsolete) — Splinter Review
Don't use a singleton type for function clones with a return statement, as those are unlikely to be used as constructors.
Assignee: nobody → jdemooij
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jdemooij)
Attachment #8729088 - Flags: review?(bhackett1024)
Attached patch PatchSplinter Review
Attachment #8729088 - Attachment is obsolete: true
Attachment #8729088 - Flags: review?(bhackett1024)
Attachment #8729089 - Flags: review?(bhackett1024)
Attachment #8729089 - Flags: review?(bhackett1024) → review+
Blocks: 1154339
https://hg.mozilla.org/mozilla-central/rev/f97f2238854e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8729089 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Unclear.
[User impact if declined]: Broken or slower websites, see this bug and bug 1154339.
[Describe test coverage new/current, TreeHerder]: Code is tested on treeherder.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8729089 - Flags: approval-mozilla-esr45?
Attachment #8729089 - Flags: approval-mozilla-beta?
Attachment #8729089 - Flags: approval-mozilla-aurora?
Alexander can you see if this is fixed now in nightly builds? Thanks!
Flags: needinfo?(ziborov.sasha)
Flags: qe-verify+
Hello - I am facing the same recursion issue with firefox 45 - 64 bit. Does not happen with firefox 45 - 32 bit. Could you please let me know what the resolution was to the bug - thanks
(In reply to Vinod Narayanan from comment #13)
> Hello - I am facing the same recursion issue with firefox 45 - 64 bit. Does
> not happen with firefox 45 - 32 bit. Could you please let me know what the
> resolution was to the bug - thanks

Also tried nightly build and same issue exists - 48.0a1 (2016-03-16)
(In reply to Vinod Narayanan from comment #14)
> Also tried nightly build and same issue exists - 48.0a1 (2016-03-16)

It's likely a different issue so could you please file a new bug and CC me?  Thanks!
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Alexander can you see if this is fixed now in nightly builds? Thanks!

Liz,

I checked out the fix on Nightly Firefox 48.0a1 (2016-03-16) and regret to report that the issue is still relevant.
 
It seems that the fix helps only for certain rendering dimensions. I have modified my example and increased the number of rendered elements:
http://jsfiddle.net/xdtcnp7u/show/
 
Note that this example continues operating correctly in the Firefox 32-bit version (for instance, 45.0).
Status: RESOLVED → REOPENED
Flags: needinfo?(ziborov.sasha)
Resolution: FIXED → ---
Version: 42 Branch → 48 Branch
Alexander, would you mind testing this build too:

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win64-pgo/1458322264/firefox-48.0a1.en-US.win64.zip

That one has the fix for bug 1257234, it bumps our stack limit on Win64 from 1 MB to 2 MB.
Depends on: 1257234
Flags: needinfo?(ziborov.sasha)
Hi Jan,

I've checked FF 48.0a1 (2016-03-20) - it works properly now! Even for large number of editors - http://jsfiddle.net/xdtcnp7u/show/

Thank you!
Flags: needinfo?(ziborov.sasha)
Thanks for verifying it works now!

I think we should uplift this patch + bug 1257234.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Jan, it is not possible to write a test for this? Thanks
Flags: needinfo?(jdemooij)
(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Jan, it is not possible to write a test for this? Thanks

Stack limits are different on each platform, and frame sizes heavily depend on the compiler and its optimization flags. That usually makes such tests brittle and I'm wary of adding them..
Flags: needinfo?(jdemooij)
Bug 1257234 is on beta now, so let's try uplifting this too.
Comment on attachment 8729089 [details] [diff] [review]
Patch

Fix for some slowness/crashing issues for win 64
This should land for beta 5 which goes to build today.
Attachment #8729089 - Flags: approval-mozilla-beta?
Attachment #8729089 - Flags: approval-mozilla-beta+
Attachment #8729089 - Flags: approval-mozilla-aurora?
Attachment #8729089 - Flags: approval-mozilla-aurora+
I was unable to reproduce this on Windows 10 x64 (14295 insider preview) using the x64-bit build of Firefox 42

Alexander, could you please confirm that this also fixed for 46 beta 6 [1] and latest 47 Aurora [2]?

[1] https://archive.mozilla.org/pub/firefox/candidates/46.0b6-candidates/build1/win64/
[2] https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/firefox-47.0a2.en-US.win64.zip
Flags: needinfo?(ziborov.sasha)
Hi Cornel
 
I have checked my sample and confirm that it works properly in both of the versions!
Flags: needinfo?(ziborov.sasha)
Many thanks!

Closing this issue based on your verification in comment 18 and comment 27.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 8729089 [details] [diff] [review]
Patch

Even if the patch is big, it landed on most of the branches without regressions, taking it.
Attachment #8729089 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.