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

Chrome Canary breaks regexes run on function toString() #4111

Closed
AshleyScirra opened this issue Feb 18, 2016 · 8 comments
Closed

Chrome Canary breaks regexes run on function toString() #4111

AshleyScirra opened this issue Feb 18, 2016 · 8 comments

Comments

@AshleyScirra
Copy link

In Chrome Canary 50.0.2654.0, function toString() now includes the function name. This breaks zlib-asm, and while investigating it (reported as crbug.com/587818) I found that asm.js code appears to run a regex on the result of function toString(), and since it doesn't expect the name to be there it fails to match and then throws when it tries to slice the result.

The workaround is to find this regex:

/^function\s*\(([^)]*)\)\s*{\s*([^*]*?)[\s;]*(?:return\s*(.*?)[;\s]*)?}$/

and (my hack, feel free to improve) add a [A-Za-z0-9]* to optionally match the function name if emitted:

/^function\s*[A-Za-z0-9]*\(([^)]*)\)\s*{\s*([^*]*?)[\s;]*(?:return\s*(.*?)[;\s]*)?}$/

Then this works in both current and Canary versions of Chrome. The Chrome bug will allow Google to determine the web compat impact, but this might need fixing in Emscripten anyway.

@rossberg
Copy link

Note that Chrome's new behaviour conforms to the current ES.next proposal for standardising toString:

http://tc39.github.io/Function-prototype-toString-revision/

It basically amounts to "return the original source code", modulo white space changes. This was agreed upon at the last TC39 meeting, and is currently at stage 2.

@kripken
Copy link
Member

kripken commented Feb 18, 2016

Thanks for the report and the info. Fixed on incoming.

The reason we need this is that for ccall we want to emit fast code, and we figure out what code to emit using a regex on a minified function (otherwise minification will mix us up). If you build with NO_DYNAMIC_EXECUTION = 0 then we go through a slow path that avoids this, btw.

@kripken kripken closed this as completed Feb 18, 2016
@AshleyScirra
Copy link
Author

FWIW, this only happens in Canary with "experimental javascript" enabled, and it looks like a V8 patch is on the way to correct it as well. However I would guess the change ought to be made anyway for robustness.

@kripken
Copy link
Member

kripken commented Feb 18, 2016

Yes, agreed.

@rodneyrehm
Copy link

Unless I'm missing something, [a-zA-Z]*\s*is not going to match all possible function names.

String(function _(a, b){ return a + b; })
String(function ಠ_ಠ(a, b){ return a + b; })

If I didn't miss anything, I'd suggest matching for [^(]* instead (anything but opening parens)

@kripken
Copy link
Member

kripken commented Feb 19, 2016

You're right, but this is only run a very specific set of methods, that we provide.

Although, come to think of it, you're right, the minified name might not fall in that subset. But I'm worried about matching anything but ( as it might allow non-function things.

Reopening, we should improve this.

@kripken
Copy link
Member

kripken commented Feb 20, 2016

I pushed a commit that makes it support all the values we can have as minified outputs. There is a risk of another minifier being used, however - but AFAIK none use unicode tricks.

@kripken kripken closed this as completed Feb 20, 2016
@rodneyrehm
Copy link

In such a case I would've added a comment to the source explaining why we're limiting function names to the given set, but LGTM. :)

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

4 participants