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

less.js with strictMath enabled runs out of memory trying to compile Bootstrap #1725

Closed
cvrebert opened this issue Dec 9, 2013 · 11 comments
Closed

Comments

@cvrebert
Copy link

cvrebert commented Dec 9, 2013

Versions:

  • lessc 1.5.1 (LESS Compiler) [JavaScript]
  • node.js v0.10.22
  • OS X 10.7.5

With Bootstrap v3.0.3:

~/bootstrap/ $ lessc --strict-math=off less/bootstrap.less 
<compiles fine and finishes within seconds>
~/bootstrap/ $ lessc --strict-math=on less/bootstrap.less 
<uses massive amounts of memory and eventually dies>

See also: twbs/bootstrap#11790 (comment)

@seven-phases-max
Copy link
Member

I can confirm that in Windows too. I'm not sure what exactly happens but here's what I have already found so far:

  • The offending file is "grid.less" - something there causes compiler to enter an infinite loop.
  • Curiously, compiler should stop with a error a while before it enters "grid.less" right here, but for some reason it does not.

Minimal imports to reproduce it:

@import "variables.less";
@import "mixins.less";
@import "grid.less";

@cvrebert
Copy link
Author

cvrebert commented Dec 9, 2013

Problem still persists after fixing all reported strictMath violations.

@seven-phases-max
Copy link
Member

OK, I think I found a minimal example to make the compiler to enter infinite loop with strictMath:on:

.loop(1);
.loop(@i) when (@i < 2) {
    .loop(@i + 1);
}

@seven-phases-max
Copy link
Member

@cvrebert

Problem still persists after fixing all reported strictMath violations.

Putting extra parens on all recursive calls in "mixins.less" (there're a lot) should help.

@cvrebert
Copy link
Author

cvrebert commented Dec 9, 2013

Putting extra parens on all recursive calls in "mixins.less" (there're a lot) should help.

Yes, that indeed fixes the problem, although it kinda sucks that .loop(@i + 1); doesn't trigger an error like other strictMath violations.

@cvrebert
Copy link
Author

@seven-phases-max Thanks for the help!

cvrebert added a commit to twbs/bootstrap that referenced this issue Dec 10, 2013
@seven-phases-max
Copy link
Member

Doh! Now I see what is the problem with it: .loop(@i + 1); does not trigger a error because it is absolutely valid .loop call (argument just becomes @i + 1 "string"). So for each next call the when condition becomes 1 + 1 < 2, 1 + 1 + 1 < 2 etc. and they all evaluate to true. I.e.:

.loop(1 + 1);
.loop(1 + 1 + 1);
.loop(1 + 1 + 1 + 1);
// etc.

.loop(@i_) {
    @i: @i_;
    @cond: "@{i} < 2";
    @{cond} {
        & when    (@i < 2) {-: true}
        & when not(@i < 2) {-: false}
    }
}

So strictly speaking it turns out to be a "LESS infinite loop" not a "compiler infinite loop" (i.e. not really a compiler bug), but unlike more simple infinite loops which usually stop quite soon with "SyntaxError: Maximum call stack size exceeded" this one causes some significant slowdown with "Out of memory" in the end before it reaches "maximum call stack".

Hmm, now I wonder what would be a way to fix that... Maybe the solution is somewhere around (whatever + 1 < 2) expression? (it looks like it should not be valid with strictMath:on at all).

@lukeapage
Copy link
Member

@seven-phases-max great work I agree that should error ((whatever + 1 < 2)) as its nonsensical and is always true.

We could/should also have loop detection that has a maximum stack size, lower than the javascript one so we detect these loops and throw a nice error.

@lukeapage
Copy link
Member

@cvrebert will you do a pull to bootstrap with the strict math violations? a while ago I did one but they are not enforcing it - it would be nice if bootstrap worked with and without strict math.

@cvrebert
Copy link
Author

@lukeapage We've already fixed them in Bootstrap and are now using strictMath.
twbs/bootstrap#11807
twbs/bootstrap#11808
twbs/bootstrap#11809

@seven-phases-max
Copy link
Member

Closing as fixed in v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants