If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Directive prologue detection needs to look across line boundaries

RESOLVED FIXED in mozilla19

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: André Bargull, Unassigned)

Tracking

({regression})

Trunk
mozilla19
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
Created attachment 664541 [details] [diff] [review]
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.
(Reporter)

Comment 2

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

Updated

5 years ago
Keywords: regression
OS: Windows 7 → All
Hardware: x86_64 → All
(Reporter)

Comment 3

5 years ago
Created attachment 670514 [details] [diff] [review]
extend processDirectives() to check for operators

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

5 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

5 years ago
Created attachment 670785 [details] [diff] [review]
extend processDirectives() to check for operators

Updated the patch according to Benjamin's suggestions.
Attachment #670514 - Attachment is obsolete: true
Attachment #670785 - Flags: review?(n.nethercote)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 800909
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

5 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 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+
Eurgh, Bugzilla collision fail.  :-(

Updated

5 years ago
Attachment #670785 - Flags: review?(benjamin) → review+
(Reporter)

Comment 10

5 years ago
Created attachment 670956 [details] [diff] [review]
extend processDirectives() to check for operators

Addressed last review comments.
Attachment #670785 - Attachment is obsolete: true
Attachment #670956 - Flags: checkin?(benjamin)
(Reporter)

Comment 11

5 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

5 years ago
If you want, you can use mq to generate a patch which will have your name on it.
(Reporter)

Comment 13

5 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

5 years ago
No longer blocks: 800909

Updated

5 years ago
Duplicate of this bug: 800909

Updated

5 years ago
Attachment #670956 - Flags: checkin?(benjamin) → checkin+

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b3592edb37
https://hg.mozilla.org/mozilla-central/rev/31b3592edb37
Status: NEW → RESOLVED
Last Resolved: 5 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.