791465 - Directive prologue detection needs to look across line boundaries

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

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`

Comment 1

[André Bargull [:anba]]

Attachment: extend processDirectives() to check for operators

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.

Comment 2

[André Bargull [:anba]]

Err, regression more likely introduced by bug 769072, because that's the first change which used "peekTokenSameLine()"...

Comment 3

[André Bargull [:anba]]

Attachment: extend processDirectives() to check for operators

Updated patch, now also includes a test case. jit-tests and jstests pass with the proposed changes.

Comment 4

[:Benjamin Peterson]

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.

Comment 5

[André Bargull [:anba]]

Attachment: extend processDirectives() to check for operators

Updated the patch according to Benjamin's suggestions.

Comment 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[:Benjamin Peterson]

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 ... :)

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[Jeff Walden [:Waldo]]

Eurgh, Bugzilla collision fail.  :-(

Comment 10

[André Bargull [:anba]]

Attachment: extend processDirectives() to check for operators

Addressed last review comments.

Comment 11

[André Bargull [:anba]]

(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

[:Benjamin Peterson]

If you want, you can use mq to generate a patch which will have your name on it.

Comment 13

[André Bargull [:anba]]

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.

Comment 15

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31b3592edb37

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31b3592edb37