Closed
Bug 791465
Opened 12 years ago
Closed 12 years ago
Directive prologue detection needs to look across line boundaries
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: anba, Unassigned)
References
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
3.88 KB,
patch
|
Benjamin
:
checkin+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427
Steps to reproduce:
A Directive Prologue is the longest sequence of expression statements, that means Parser::processDirectives() needs to look across line boundaries while detecting directives.
Regression introduced in bug 773153.
js> function f() {
"use strict"
+ "not!";
}
js> f.caller
Actual results:
Accessing `f.caller` throws
->
TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them
Expected results:
`f.caller` should yield `null`
Reporter | ||
Comment 1•12 years ago
|
||
A possible extension for Parser::processDirectives() to check whether the next line starts with an operator.
Note: The patch is missing a test case, I didn't run any test suite and "TokenKindIsBinaryOperatorLike" is an awful name. But maybe someone else likes to chime in and finish the remaining issues to get this bug fixed.
Reporter | ||
Comment 2•12 years ago
|
||
Err, regression more likely introduced by bug 769072, because that's the first change which used "peekTokenSameLine()"...
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Comment 3•12 years ago
|
||
Updated patch, now also includes a test case. jit-tests and jstests pass with the proposed changes.
Attachment #664541 -
Attachment is obsolete: true
Attachment #670514 -
Flags: feedback?(benjamin)
Comment 4•12 years ago
|
||
Comment on attachment 670514 [details] [diff] [review]
extend processDirectives() to check for operators
Review of attachment 670514 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/frontend/Parser.cpp
@@ +1857,5 @@
> bool isDirective = IsEscapeFreeStringLiteral(directive);
> JSAtom *atom = directive.atom();
> TokenKind next = tokenStream.peekTokenSameLine();
> + if (next != TOK_EOF && next != TOK_SEMI && next != TOK_RC &&
> + (next != TOK_EOL || tokenStream.isNextTokenNotUnary()))
I suggest you remove TokenStream::isNextTokenNotUnary() and instead directly use TokenKindIsNotUnary(tokenStream.peekToken()).
An explanatory comment might be nice, too.
::: js/src/frontend/TokenStream.h
@@ +182,4 @@
> }
>
> inline bool
> +TokenKindIsNotUnary(TokenKind tt)
This ought to be called something like "TokenContinuesStringExpression".
::: js/src/jit-test/tests/basic/bug791465.js
@@ +38,5 @@
> + function () {
> + "use strict"
> + + "not";
> + },
> +];
You should test all the token types that return |true| from TokenKindIsNotUnary. Generating the tests would be fine.
Attachment #670514 -
Flags: feedback?(benjamin) → feedback+
Reporter | ||
Comment 5•12 years ago
|
||
Updated the patch according to Benjamin's suggestions.
Attachment #670514 -
Attachment is obsolete: true
Attachment #670785 -
Flags: review?(n.nethercote)
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 6•12 years ago
|
||
Comment on attachment 670785 [details] [diff] [review]
extend processDirectives() to check for operators
Review of attachment 670785 [details] [diff] [review]:
-----------------------------------------------------------------
Um, this looks ok but I'm not really familiar with this, so benjamin should just review again.
Attachment #670785 -
Flags: review?(n.nethercote) → review?(benjamin)
Comment 7•12 years ago
|
||
Comment on attachment 670785 [details] [diff] [review]
extend processDirectives() to check for operators
Review of attachment 670785 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the comprehensive test.
::: js/src/frontend/Parser.cpp
@@ +1857,4 @@
> bool isDirective = IsEscapeFreeStringLiteral(directive);
> JSAtom *atom = directive.atom();
> TokenKind next = tokenStream.peekTokenSameLine();
> + // We need to check whether the directive ends explicitly or implicitly
Nit: Space between last line of code and comment.
::: js/src/jit-test/tests/basic/bug791465.js
@@ +1,5 @@
> +load(libdir + "asserts.js");
> +
> +function xmlAllowed() {
> + return (typeof options == "function") &&
> + (options() + "").indexOf("allow_xml") >= 0;
You can use String.prototype.contains().
@@ +56,5 @@
> + "use strict"
> + ();
> + },
> + ...(xmlAllowed() ? [Function("'use strict'\n..not")] : []),
> + ...[Function("'use strict'\n " + op + " 'not'") for (op of binary_ops)],
Clever usage of ... :)
Attachment #670785 -
Flags: review?(benjamin) → review+
Comment 8•12 years ago
|
||
Comment on attachment 670785 [details] [diff] [review]
extend processDirectives() to check for operators
Review of attachment 670785 [details] [diff] [review]:
-----------------------------------------------------------------
I'll hand you an r+ on this, since I looked at it out of curiosity as to its sanity, manual token-listing being way scary and all that. :-) I don't much like this particular way, but without greater parser surgery this seems like the only path forward.
JSC handles this by parsing SourceElement lists and then reparsing if an element that's a directive prologue member that's "use strict" gets hit. That avoids a lot of complexity, because "foo"\n+2 gets parsed using the normal parser paths, rather than a special directive-only parsing mode. Now that we require whole scripts at once when parsing, perhaps we should change to that mechanism as well? This would be a followup or separate bug for sure, tho, so I'm fine with this for now.
Attachment #670785 -
Flags: review?(benjamin)
Attachment #670785 -
Flags: review+
Comment 9•12 years ago
|
||
Eurgh, Bugzilla collision fail. :-(
Updated•12 years ago
|
Attachment #670785 -
Flags: review?(benjamin) → review+
Reporter | ||
Comment 10•12 years ago
|
||
Addressed last review comments.
Attachment #670785 -
Attachment is obsolete: true
Attachment #670956 -
Flags: checkin?(benjamin)
Reporter | ||
Comment 11•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> I'll hand you an r+ on this, since I looked at it out of curiosity as to its
> sanity, manual token-listing being way scary and all that. :-) I don't
> much like this particular way, but without greater parser surgery this seems
> like the only path forward.
Yeah, it's a quite brittle solution, it's just too easy to forget updating TokenContinuesStringExpression(). I expect it to break as soon as template strings are added. Maybe a note should be added to bug 688857...
> JSC handles this by parsing SourceElement lists and then reparsing if an
> element that's a directive prologue member that's "use strict" gets hit.
> That avoids a lot of complexity, because "foo"\n+2 gets parsed using the
> normal parser paths, rather than a special directive-only parsing mode.
The directive-only parsing mode is relatively new (bug 769072). And most likely the strict-mode detection needs to updated for arrow functions anyway.
Comment 12•12 years ago
|
||
If you want, you can use mq to generate a patch which will have your name on it.
Reporter | ||
Comment 13•12 years ago
|
||
I'm currently using the mozilla-central mirror hosted at github, that means I'm not even working with mercurial right now. So I don't think it's worth the trouble to install mercurial, pull the source again, re-apply the patch etc. etc. just for getting my name into the commit message. In short: I'm perfectly fine if the patch gets applied without my name.
Updated•12 years ago
|
Attachment #670956 -
Flags: checkin?(benjamin) → checkin+
Comment 15•12 years ago
|
||
Comment 16•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•