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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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;');

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Assert only if strict_mode is off, change conditionals to deal with strict mode

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 3

[:Benjamin Peterson]

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.

Comment 4

[:Benjamin Peterson]

(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 5

[:Benjamin Peterson]

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.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Remove assertion that Function constructor defined functions cannot have inherited strict mode

Comment 7

[:Benjamin Peterson]

Can you please add a test that decompileBody (a function in the shell) works on "new Function" with JSOPTION_STRICT_MODE?

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Remove assertion that Function constructor defined functions cannot have inherited 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.

Comment 9

[:Benjamin Peterson]

Thanks for the patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f11e31359b

Comment 10

[:Benjamin Peterson]

Followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6593f27f1898

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/28f11e31359b
https://hg.mozilla.org/mozilla-central/rev/6593f27f1898

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Btw, Benjamin, you can use hg commit --user="Foo" or hg qrefresh --user="Foo" if you didn't write the patch.

Comment 13

[:Benjamin Peterson]

Thanks for the tip.

Comment 14

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929