Closed Bug 800407 Opened 7 years ago Closed 7 years ago

Assertion failure: !funCon || !addUseStrict, at jsfun.cpp:668

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(1 file, 2 obsolete files)

The following testcase asserts on mozilla-central revision 5cca0408a73f (no options required):


options("strict_mode");
function test(str, arg, result) {
    var fun = new Function('x', str);
    var got = fun.toSource();
}
test('return let (y) x;');
options("strict_mode") triggers unconditional strict mode. It seems appropriate to carry that strict mode over to functions created with 'new Function(...)' as well, unlike ES5 "use strict";

The attached patch adds a check for strict_mode and changes some branch conditions as required. Asking benjamin for review because he wrote the original.
Comment on attachment 670931 [details] [diff] [review]
Assert only if strict_mode is off, change conditionals to deal with strict mode

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

Thanks for the patch.

I suggest that instead of modifying jsfun.cpp, you simply force |script->explicitUseStrict| to true in frontend::CompileFunctionBody if the function is strict.

::: js/src/tests/ecma_5/extensions/uneval-strict-functions.js
@@ +49,5 @@
>  assertEq(eval(uneval(lenient_outer_expr()))(), true);
>  
> +// forced strict mode should be active
> +// inside function constructor defined functions as well
> +options("strict_mode");

|options("strict_mode")| will set JSOPTION_STRICT_MODE for the rest of this file, which is not desirable. I suggest you put this testcase in its own file. jit-test/tests/basic/function-to-source-*.js would be a good place.
Attachment #670931 - Flags: review?(benjamin) → review-
Attachment #670931 - Flags: review-
(In reply to Benjamin Peterson [:benjamin] from comment #3)
> Comment on attachment 670931 [details] [diff] [review]
> Assert only if strict_mode is off, change conditionals to deal with strict
> mode
> 
> Review of attachment 670931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch.
> 
> I suggest that instead of modifying jsfun.cpp, you simply force
> |script->explicitUseStrict| to true in frontend::CompileFunctionBody if the
> function is strict.

Ignore this. I wasn't thinking.
Comment on attachment 670931 [details] [diff] [review]
Assert only if strict_mode is off, change conditionals to deal with strict mode

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

::: js/src/jsfun.cpp
@@ +663,5 @@
>          bool addUseStrict = script->strictModeCode && !script->explicitUseStrict;
>  
>          // Functions created with the constructor can't have inherited strict
> +        // mode unless JSContext has runtime option on. (see bug 736792)
> +        JS_ASSERT_IF(!cx->hasRunOption(JSOPTION_STRICT_MODE), !funCon || !addUseStrict);

I think this assertion should just be removed. It's possible to change JSOPTION_STRICT_MODE, so this would fail if a function was compiled with the option on, the option was removed, and toSource was called.
Attachment #670931 - Flags: review+
Can you please add a test that decompileBody (a function in the shell) works on "new Function" with JSOPTION_STRICT_MODE?
Once change:
bodyEnd is used only when bodyOnly is true AND funCon is false. When funCon is true, the find body call has never happened, so use the whole source.

Added test, crashes without patch, passes with patch.
Attachment #671483 - Attachment is obsolete: true
Attachment #671483 - Flags: review?(benjamin)
Attachment #671529 - Flags: review?(benjamin)
Attachment #671529 - Flags: review?(benjamin)
Attachment #671529 - Flags: review+
Attachment #671529 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/28f11e31359b
https://hg.mozilla.org/mozilla-central/rev/6593f27f1898
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Btw, Benjamin, you can use hg commit --user="Foo" or hg qrefresh --user="Foo" if you didn't write the patch.
Thanks for the tip.
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.