Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

too much recursion error on Firefox #3028

Closed
dohomi opened this issue Nov 6, 2014 · 40 comments
Closed

too much recursion error on Firefox #3028

dohomi opened this issue Nov 6, 2014 · 40 comments

Comments

@dohomi
Copy link

dohomi commented Nov 6, 2014

Hello,

I curious if the Meteor team is aware of the too much recursion bug in Firefox:
http://autoform.meteor.com/qfdetails
The error mainly occurs because of the usage of https://docs.meteor.com/#/full/template_parentdata Template.parentData. We receive this error on all older Computer/Mac/Linux, the page is not opening at all. On newer Macs the website opens with some minor errors.

Thanks, Dominic

@dohomi
Copy link
Author

dohomi commented Nov 13, 2014

Any idea on this issue?

@glasser glasser added the Blaze label Dec 3, 2014
@brylie
Copy link
Contributor

brylie commented Dec 25, 2014

This is also affecting clients using Firefox 34+ on Ubuntu Linux 14.10.

@brylie
Copy link
Contributor

brylie commented Dec 25, 2014

I have experienced this issue with FF 34 using the Iron Router package as well. I get the error:

// Without {{> yield }} in the base template
Too much recursion ejson.js:394
// With {{> yield }} in the base template
too much recursion. underscore:164

I am using Iron Router with one base template, without and with a {{> yield }} statement.

@brylie
Copy link
Contributor

brylie commented Jan 3, 2015

For easy reproduction, I have created a minimal example using aldeed:autoform:

<body>
 {{> quickForm id="quickform" schema="schema"}}
</body>
schema = new SimpleSchema({
    name: {
        type: String
    }
});

@avital
Copy link
Contributor

avital commented Jan 6, 2015

Does someone have a simple repro without external packages? That will make it easier to investigate.

@lalomartins
Copy link

@avital I was trying to write one but a deeper look at Meteor-Community-Packages/meteor-autoform#333 points to this being not really a Meteor bug but rather a question of Firefox having a smaller recursion limit (at least on Linux). I'd wait and see what they end up with as a fix.

@darkship
Copy link

I just run into this problem. I'm using linux mint
I don't use aldeed/meteor-autoform but I have this error on FF 35 and FF 36 both on linux and it doesn't always happen.

I tried http://adamrich.name/recursion.html on different browsers and here are my results:

  • Windows 8 firefox 34 : 7k5 to 12k recursions with a peak at 20k
  • Windows 8 firefox 35 : 8k to 11k recursions with a peak at 18k
  • Windows 8 firefox 36 (dev edition) : 10k to 13k recursions with a peak at 25k
  • Windows 8 chrome 39 : 22k to 24k recursions
  • linux mint 17 firefox 35 : 5k to 6k (sometimes 9K) recursions with a peak at 16k
  • linux mint 17 firefox 36 (dev edition) : 5k to 6k recursions with a peak at 16k
  • linux mint 17 chromium 39 : 23k recursions

I thought first that was a bug in Firefox dev edition. I updated to firefox 35 so I have no tests with FF34.

I have something like 15 nested templates in my app and when I remove 1 or 2 of them, the bug doesn't show anymore.

@avital Repo with no package : https://github.com/darkship/test_much_recursion
If you increment 1 by 1, there is no error until a while (tried until 100) but if you increment by 12 it breaks.

@brylie
Copy link
Contributor

brylie commented Jan 15, 2015

I ran the browser recursion test too:

Stopped at iteration 7181 with error message: too much recursion

Thanks for the suggestion :-)

@brylie
Copy link
Contributor

brylie commented Jan 15, 2015

That is an interesting recursion repo @darkship. How can I track down the recursion in Meteor templates? I would like to help shine some light on this issue, and hopefully improve Meteor's scalability, etc :-)

@benjamn
Copy link
Contributor

benjamn commented Feb 4, 2015

After some investigation, it appears that a combination of deeply nested template contexts in the quickform package, together with relatively deep call stacks in the Blaze implementation, are conspiring to trigger the RangeError. In other words, the call stacks in question are large, but not infinitely large, so there is probably no single bug here that we can fix to make the problem go away entirely, but many opportunities to alleviate the problem in realistic use cases.

Two opportunities that we've thought about so far:

  • We can make individual stack frames smaller by moving parameters and other local variables onto the heap. Apparently the number of local variables in a function affects the memory footprint of its stack frames, which limits the total number of stack frames that can be nested. So replacing multiple local variables with a single object whose properties represent the original variables might let us get away with more recursion.
  • We can reimplement some algorithms that are currently recursive as iterative algorithms, for example HTML.visitor (in the htmljs package), which shows up on call stacks for this reproduction.

Given the exploratory nature of these solutions, I'm not sure when we'll be able to provide real results, but we're very glad to know about the problem, and we acknowledge that fixing it will require optimizations in the Blaze implementation.

@benjamn benjamn self-assigned this Feb 4, 2015
@levino
Copy link

levino commented Feb 4, 2015

Would love a fix too. We have the issue at coyno.

@laurentpayot
Copy link

Same autoform issue here with Firefox 35.0.1 and Meteor 1.0.3.1 on Ubuntu 14.10. Works fine with Chromium.

@aldeed
Copy link
Contributor

aldeed commented Feb 12, 2015

An update from the autoform side: I've done some work in devel branch to remove unnecessary layers of templates. It's helped a bit but it's still pretty easy to reproduce this issue. I think the primary culprit is the way autoform is using parentData. We recurse through a bunch of layers of templates and then look up the tree with parentData. I don't think parentData itself uses much recursion, but it might be resulting in a reactive dependency that leads to recursion upon reruns.

Still pretty much guessing. Hopefully I can figure out a clean reproduction.

@brylie
Copy link
Contributor

brylie commented Feb 12, 2015

Thanks for your work on this issue Eric. :-) How can we coordinate efforts to narrow this down?

Edited I thought this was the autoform issue thread.

@brylie
Copy link
Contributor

brylie commented Feb 12, 2015

Somewhat off-topic, but interestingly relevant is the paper Natural language – no infinity
and probably no recursion (Luuk and Luuk, 2012)
.

@BradRyan
Copy link

I'm getting "Maximum call stack size exceeded" in Chrome when Autovalue is taking another field value, this.siblingField(). I am able to store the field value in a local variable and return a value from the nested array even. The error occurs after I return the value.

My form is somewhat complex (arrays of objects in a few fields). Might have to manually create input fields for my use case since this sounds like a complicated fix. I'm still happy all my other simpler forms are working well!

@mxab
Copy link

mxab commented Feb 27, 2015

Seems to be worse with the latest Firefox (36) version on Windows 8

@mxab
Copy link

mxab commented Feb 27, 2015

I made a nested template example which fails in Firefox in Windows 8 (not tested in 7)
it fails as soon if there are more than 23 nested templates, in chrome it still works if you render more than 150 nested templates
see this example:
https://github.com/mxab/meteor-call-stack-exceed

@dohomi
Copy link
Author

dohomi commented Feb 28, 2015

Hi @benjamn
Does anybody use Meteor for a production app, because in my opinion this is a major blocker and Blaze shouldn't call itself stable. I overlooked this issue for too long already, but Meteor is calling itself stable for almost 4 months and the biggest browser Firefox isn't supported yet. How should production apps survive without the 20-40% of visitors who won't be able to use the app?
Would be great to hear any news on this issue. Thanks

@lalomartins
Copy link

@dohomi it's quite stable; this bug is a corner case that doesn't come up in normal circumstances. You might flip it around and ask if Firefox is stable! (After all it is, technically speaking, a Firefox bug.)

@dohomi
Copy link
Author

dohomi commented Mar 1, 2015

@lalomartins sorry to say but Firefox is one of the major browser at the moment.. Meteor has to adopt its code to Firefox not other way around.. Who can waive all Firefox users on Windows, Linux or older Macs away because you would state its a Firefox bug? Check @mxab simple reproduction package where you see how easy it is to crash your app. In the end as a startup you depend on every single visitor and they won't care who's responsible for the black page they are visiting - they simply never come back.

@lalomartins
Copy link

@dohomi you're entirely missing the point, so let me spell it out for you.

Coming here to disparage the project with “can you really call yourself stable” FUD is unproductive and not acceptable behaviour in an issue tracker.

Your users are correct if they don't come back, because as a startup neither meteor or firefox is responsible for the black page your visitors see — you are. You're the only one here trying to shift blame and responsibility. As a startup it's your job to test thoroughly, and honestly, working around browser and/or framework bugs is at least 30% of a web developer's job description. It's especially unpleasant to see you disparaging a project which, I'm sure, already saved you weeks, if not months, of development time, compared to developing whatever you have on any other platform.

So really, play nice. This is not the space for this kind of talk. In fact, I don't think anywhere is.

And it's not the point how easy it is to crash it. The point is, it doesn't happen under normal usage. Yes, there are big production apps out there. You could easily find out by doing a little research instead of asking here. None of those ran into this bug. I have deployed a few production apps myself and only recently I ran into this. That's the definition of a corner case. It's like saying Windows is not stable because it can bluescreen on occasion. Well d'oh. Software fails. Welcome to real life.

@brylie
Copy link
Contributor

brylie commented Mar 1, 2015

Hey all. Please consider continuing this discussion on the Meteor forums. At this point it is neither constructive nor salient.

On 1 March 2015 14:01:06 EET, Lalo Martins notifications@github.com wrote:

@dohomi you're entirely missing the point, so let me spell it out for
you.

Coming here to disparage the project with “can you really call yourself
stable” FUD is unproductive and not acceptable behaviour in an issue
tracker.

Your users are correct if they don't come back, because as a startup
neither meteor or firefox is responsible for the black page your
visitors see — you are. You're the only one here trying to shift blame
and responsibility. As a startup it's your job to test thoroughly, and
honestly, working around browser and/or framework bugs is at least 30%
of a web developer's job description. It's especially unpleasant to see
you disparaging a project which, I'm sure, already saved you weeks, if
not months, of development time, compared to developing whatever you
have on any other platform.

So really, play nice. This is not the space for this kind of talk. In
fact, I don't think anywhere is.


Reply to this email directly or view it on GitHub:
#3028 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@mxab
Copy link

mxab commented Mar 4, 2015

@lalomartins I would not specify the error as a bug of firefox since I am not aware of any html/js specification about max callstack / recursion limitations ( but also I haven't googled for it hardcore)

I would neither call it a meteor "bug" if you do not take any recursion limitations into account it works as expected, further I think this issue has not surfaced more as there is not official windows development support

due to the tree structured nature of my mini cms I cannot avoid deep nesting of templates

as soon have you have some more "complex" nesting this problem occurs faster
https://github.com/mxab/meteor-call-stack-exceed/tree/more-samples

so I think as @benjamn said, blaze/htmljs needs to be tuned. I tried some peeking in blaze / htmljs parts and was printing out the stack size but I haven't found the point where it "explodes". So if someone could point me in the right direction I really would like to find some workarrounds for this issue

@mxab
Copy link

mxab commented Mar 5, 2015

Hi @stbaer and I did some more research on the blaze package, after inserting the "catch" statement to log the errors at
master...stbaer:ff_recursion_testing#diff-61be8b0b328aaf8a6ae2f89b67662389R416
and
master...stbaer:ff_recursion_testing#diff-52584623fe717d42fab5db3d19d9a498R481
we noticed several accesses on undefineds, after preventing theses errors at

The original example allow the Win8/Firefox 36 to easily nest more then 300 templates
see https://github.com/mxab/meteor-call-stack-exceed/tree/with-undefined-catches

@benjamn could this be a possible optimisation or are the undefined access in these cases expected or should be handled differently
I haven't tested it against any other packages like @aldeed autoform

@aldeed
Copy link
Contributor

aldeed commented Mar 5, 2015

@mxab, sounds promising! I will try to find some time to test those changes against autoform recursion errors.

@stbaer
Copy link

stbaer commented Mar 11, 2015

Hi @aldeed, like @mxab said, the changes work very well for the https://github.com/mxab/meteor-call-stack-exceed example. But in our project with autoform included firefox shows an 'unresponsive script' popup.

@avital or anybody from the meteor team, any thoughts on this?

@shtefcs
Copy link

shtefcs commented Mar 20, 2015

To much people have problem with it Meteor-Community-Packages/meteor-autoform#333, can you guys @mdg give us some updates on it plsss ?

@lalomartins
Copy link

Here's the thing though. When I say it's a Firefox issue, I'm not arguing about blame assignment, because that doesn't help solve the problem. What I'm saying is, it's not clear there's anything autoform or meteor can do to fix it. That doesn't mean it's not worth trying, but ultimately, the best fix just might end up being a pull request to Mozilla (or whatever the Mercurial equivalent is).

tl;dr chill

@dgreensp
Copy link
Contributor

Fixed on devel. Thanks for your patience, everyone.

@mquandalle
Copy link
Contributor

wow!

@benjamn
Copy link
Contributor

benjamn commented Mar 26, 2015

Awesome!

@benjamn benjamn removed their assignment Mar 26, 2015
@ripdog
Copy link

ripdog commented Mar 26, 2015

Yes, thank you! Now Firefox can once again be a first class citizen of
my app! :)

On Thu, Mar 26, 2015, at 03:55 PM, Ben Newman wrote:

Awesome!

— Reply to this email directly or view it on GitHub[1].

Links:

  1. too much recursion error on Firefox #3028 (comment)

@boustanihani
Copy link

COOL :) When will the fix get published ?

@aldeed
Copy link
Contributor

aldeed commented Mar 26, 2015

@dgreensp, hoping you can sneak this into 1.1 or a patch soon after.

dgreensp added a commit that referenced this issue Mar 26, 2015
This way we don't get a stack overflow when materializing nested
Views.  Certain browser/OS combinations seem to have particularly
low budgets (especially Firefox/Windows apparently).

Verified by running https://github.com/mxab/meteor-call-stack-exceed
on Chrome/Mac.  Nesting limit used to be about 160, but now you get
unlimited nesting (tried up to 10,000, which renders in about 7-8
seconds).

Tested for correctness by running all package tests.
@avital
Copy link
Contributor

avital commented Mar 28, 2015

Yup, this is in 1.1-rc.1

@aldeed
Copy link
Contributor

aldeed commented Mar 30, 2015

Seems to have fixed the autoform issues. Thanks!

@shtefcs
Copy link

shtefcs commented Mar 31, 2015

Awesome, tnx guys!

@dohomi
Copy link
Author

dohomi commented Apr 1, 2015

Thanks guys!

@darkship
Copy link

darkship commented Apr 1, 2015

This is definitely much, much better, thanks !

I updated my repo https://github.com/darkship/test_much_recursion. I could go from 1 to 457 (instead of 1 to 12) recursions. Some errors remains when I go back to 1 but it doesn't seem to be bothering.

martijnwalraven pushed a commit that referenced this issue Feb 22, 2016
This way we don't get a stack overflow when materializing nested
Views.  Certain browser/OS combinations seem to have particularly
low budgets (especially Firefox/Windows apparently).

Verified by running https://github.com/mxab/meteor-call-stack-exceed
on Chrome/Mac.  Nesting limit used to be about 160, but now you get
unlimited nesting (tried up to 10,000, which renders in about 7-8
seconds).

Tested for correctness by running all package tests.
martijnwalraven pushed a commit that referenced this issue Feb 22, 2016
This way we don't get a stack overflow when materializing nested
Views.  Certain browser/OS combinations seem to have particularly
low budgets (especially Firefox/Windows apparently).

Verified by running https://github.com/mxab/meteor-call-stack-exceed
on Chrome/Mac.  Nesting limit used to be about 160, but now you get
unlimited nesting (tried up to 10,000, which renders in about 7-8
seconds).

Tested for correctness by running all package tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests