Closed Bug 837192 Opened 11 years ago Closed 8 years ago

"Assertion failure: fun->isExprClosure() == !braced,"

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gkw, Assigned: jorendorff)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:])

Attachments

(3 files)

Attached file stack
"use strict";
function x(x=({l:/[)]/}),...l) {}
print(x)

asserts js debug shell on m-c changeset 50cf5bbcb180 without any CLI arguments at Assertion failure: fun->isExprClosure() == !braced,
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   112609:247fb0614615
user:        Luke Wagner
date:        Thu Nov 01 21:35:09 2012 -0700
summary:     Bug 807228 - clean up the JSFUN_* flags situation (r=jorendorff)

Luke, do you think bug 807228 caused the assertion to appear?
Blocks: 807228
Flags: needinfo?(luke)
I don't think so; I believe the patch just fixes a bug where we were incorrectly flagging 'x' in comment 0 as a non-expression-closure (due to the previous insane JSFUN flag situation).

I'm not positive I'm parsing 'x' correctly in comment 0, but I think this is just a bug in FindBody where /[)]/ is a regular expression but we are thinking it is the closing ) of the arguments list?  Is that right Benjamin?  (I hope I'm wrong, because otherwise it seems like we'll need to rope in the regexp parsing logic...)
Flags: needinfo?(luke)
Oops, I should have said "incorrectly flagging 'x' as an *expression* closure" in the first sentence.
The following changeset may have been the one that started asserting:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   99950:e080642175e6
user:        Benjamin Peterson
date:        Fri Jul 20 20:17:38 2012 +0200
summary:     Bug 761723 - Save script sources to implement Function.prototype.toString. r=jorendorff,njn,jimb,jst,Ms2ger

(and this assertion later morphed to the one in this bug after Luke's changeset landing in comment 1)
Blocks: savesource
No longer blocks: 807228
Flags: needinfo?(benjamin)
(In reply to Luke Wagner [:luke] from comment #2)
> (I hope I'm wrong, because otherwise it seems like we'll need to rope in the
> regexp parsing logic...)

The ES regexp grammar is designed so you can do a quick lex without knowing all the details. It's not as trivial as one might like, but it's not as bad as it could be.
Cool, I didn't know that.
This works and doesn't seem to break anything else. Not sure if it's actually more correct, though.
Attachment #710188 - Flags: review?(jorendorff)
Flags: needinfo?(benjamin)
Comment on attachment 710188 [details] [diff] [review]
use TSF_OPERAND flag

Review of attachment 710188 [details] [diff] [review]:
-----------------------------------------------------------------

Not good enough, unfortunately:

function y(y=({a: 3/2, b: /[)]/, c: 2/3}), ...l) {}
y.toSource();

Clearing review flag.
Attachment #710188 - Flags: review?(jorendorff)
Comment on attachment 710188 [details] [diff] [review]
use TSF_OPERAND flag

Review of attachment 710188 [details] [diff] [review]:
-----------------------------------------------------------------

Not good enough, unfortunately:

function y(y=({a: 3/2, b: /[)]/, c: 2/3}), ...l) {}
y.toSource();

Clearing review flag.
Assigning to Benjamin since he has created a patch, please change this if necessary.
Assignee: general → benjamin
Status: NEW → ASSIGNED
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2c41bf87b4e5).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   125318:bf3ce88c6ea3
user:        Jason Orendorff
date:        Sun Mar 17 20:42:36 2013 -0700
summary:     Bug 846406 - Implement arrow functions. r=bhackett. Changes to Y.js r=brendan.

This iteration took 130.377 seconds to run.
Jason, is the fix in comment 12 likely to have fixed this issue? Should we close as dup/wfm?
Flags: needinfo?(jorendorff)
It makes no sense for my changes to FindBody to have fixed it. Looking closer.
Flags: needinfo?(jorendorff)
Oh, I weakened the assertion. The code is still bogus and generates bogus output, but the assertion no longer detects it. Here's a test that detects it:

"use strict";

function f(x=/[)]/) {}
assertEq("" + f, 'function f(x=/[)]/) {\n"use strict";\n}');

function g(a=3/2, b=/[)]/, c=2/3) {}
assertEq("" + g, 'function g(a=3/2, b=/[)]/, c=2/3) {\n"use strict";\n}');
jimb claims that you can tokenize JS using a finite amount of state; you don't have to do a full parse.

It occurs to me we could just teach the TokenStream how to do this and stop using parser feedback. But what's the algorithm?
I guess it is pretty straightforward.

* At the beginning of a program, / starts a regexp.

* After an identifier, keyword, literal, or one of the punctuators ) ] },
  / is division.

* After any other punctuator, except ++ or --, / starts a regexp.

* If you the previous token is ++ or --, ignore it and look at the token before that:

    a / 2;         division
    a++ / 2;       still division

    a = / 2 /;         regexp
    a = ++/ 2 /.x;     still regexp

Having TokenStream do it automatically would fix this bug.
I might be missing something, but here are some fun test cases:
* a = 1 <newline> ++/
* if (1) /
* if (1) {} /
* a = {} /
(In reply to Simon Lindholm from comment #18)
> * if (1) /

Oh, yeah, you're right, this is awful. jimb, this makes comment 5 a little less true, right? What do you think?

To cope correctly with this case, the tokenizer would have to keep a stack of matching () [] {} characters. Something like SUPPORTED_NESTING_DEPTH bytes. :-P
QA Contact: general
(In reply to Jason Orendorff [:jorendorff] from comment #19)
> Oh, yeah, you're right, this is awful. jimb, this makes comment 5 a little
> less true, right? What do you think?

Setting needinfo? from :jimb to help move this forward.
Flags: needinfo?(jimb)
(In reply to Jason Orendorff [:jorendorff] from comment #19)
> (In reply to Simon Lindholm from comment #18)
> > * if (1) /
> 
> Oh, yeah, you're right, this is awful. jimb, this makes comment 5 a little
> less true, right? What do you think?

Yeah, my hack loses in that case. :( I guess I'd better fix the idutils scanner.
Flags: needinfo?(jimb)
Not sure what needs to be done here since it seems to have dropped off people's radar in the past 8-9 months, so re-pinging needinfo, or please feel free to push this to someone else.
Flags: needinfo?(jorendorff)
Flags: needinfo?(jimb)
I have no easy fix for this.
Flags: needinfo?(jimb)
I propose we rip out the part of Function.p.toString()/.toSource() where we try to inject "use strict"; if it's not already present. Sure, the meaning of a strict-mode function depends on its context. So what? The same goes for every closure; we don't try to rewrite those.

What do you think, Waldo?
Assignee: benjamin → jorendorff
Flags: needinfo?(jorendorff) → needinfo?(jwalden+bmo)
I agree entirely.
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8729627 [details] [diff] [review]
Stop trying to inject "use strict"; into Function.prototype.toString() output

Review of attachment 8729627 [details] [diff] [review]:
-----------------------------------------------------------------

YES.  FINALLY.

::: js/src/tests/ecma_5/extensions/strict-function-toSource.js
@@ -7,5 @@
> -function testRunOptionStrictMode(str, arg, result) {
> -    var strict_inner = new Function('return typeof this == "undefined";');
> -    return strict_inner;
> -}
> -assertEq(eval(uneval(testRunOptionStrictMode()))(), true);

This test doesn't look like it *really* cares about "use strict" insertion, so it should probably stick around.  But this, should change to:

assertEq(testRunOptionStrictMode()(), true);
assertEq(eval(uneval(testRunOptionStrictMode()))(), false);

@@ -9,5 @@
> -    return strict_inner;
> -}
> -assertEq(eval(uneval(testRunOptionStrictMode()))(), true);
> -
> -assertEq((new Function('x', 'return x*2;')).toSource().includes('\n"use strict"'), true);

And seems like this should check for false.
Attachment #8729627 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b7b0aae0f722d93d9bff69d5051d97fe5cf2c42
Bug 837192 - Stop trying to inject "use strict"; into Function.prototype.toString() output. r=Waldo.
This bug's landing (comment 28) left a variable with only 1 usage, inside of a MOZ_ASSERT_IF statement, which caused Werror unused-variable build errors in opt builds & closed the tree.

I pushed a followup (comment 29) to fix that -- I folded the variable into its only usage site, which is an assertion.
https://hg.mozilla.org/mozilla-central/rev/5b7b0aae0f72
https://hg.mozilla.org/mozilla-central/rev/e79cc51e486c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: