878520 - "Assertion failure: isInterpreted()" with asm module inside recursive function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

(function rec(depth) {
    if (depth) {
        function unusedFun() {
            var asmModule = function () { 'use asm'; return {}; };
        };
        rec(depth - 1);
    }
})(2);

Assertion failure: isInterpreted(), at js/src/jsfun.h:220

#0  0x000000010000eca2 in JSFunction::getOrCreateScript ()
#1  0x000000010022e47b in js::CloneScript ()
#2  0x000000010022f116 in js::CloneFunctionScript ()
#3  0x00000001001852fb in js::CloneFunctionObject ()
#4  0x00000001001d258b in js::CloneFunctionObjectIfNotSingleton ()
#5  0x00000001001ce32b in js::DefFunOperation ()
#6  0x00000001001c6076 in js::Interpret ()
#7  0x00000001001c19b3 in js::RunScript ()
#8  0x00000001001cad2a in js::ExecuteKernel ()
#9  0x00000001001caf6e in js::Execute ()
#10 0x000000010012eb24 in JS_ExecuteScript ()
#11 0x00000001000036bd in Process ()
#12 0x0000000100001bb7 in Shell ()
#13 0x000000010000289e in main ()

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/30b977b2b911
user:        Nicholas Nethercote
date:        Thu Mar 14 18:44:03 2013 -0700
summary:     Bug 851421 (part 2) - Don't emit bytecode for asm.js functions unless linking fails.  r=luke.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed fix

If I understand correctly, the problem is that when copying the script, functions are all considered to be interpreted, while they could actually be native.

As asm.js modules are autonomous until linkage and cannot embed closures or external variables, we can just reuse the module function in every clone containing it. What do you think of this?

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 758080 [details] [diff] [review]
proposed fix

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

Better ask luke -- I'm not very familiar with the cloning stuff.  Sorry.

(I can point out that opening braces in |if| statements are meant to go on the same line, though...)

Comment 4

[Luke Wagner [:luke]]

A related question is why this recursively-called function is being given a singleton type!  Investigated and filed bug 880085.  This was a recent change which is why this bug (which is also real and should be fixed) only showed up recently.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 758080 [details] [diff] [review]
proposed fix

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

Btw, thanks for looking into this Benjamin and nice job digging in!  Normally, with every bug-fix, we add the regressing testcase.  In our case, the testcase in comment 0 will cease to be a testcase soon (once bug 880085 is fixed), so could you add this testcase instead:

function surprise(depth) {
    arguments.callee.caller(depth);
}
(function(depth) {
    function foo() { function asmModule() { 'use asm'; return {} } };
    if (depth)
        surprise(depth - 1);
})(2);

(Hitting the same case without using a named lambda.)  The normal file path would be js/src/jit-tests/tests/asm.js/testBug878520.js.

::: js/src/jsscript.cpp
@@ +2302,5 @@
>              } else if (obj->isFunction()) {
>                  RootedFunction innerFun(cx, obj->toFunction());
> +                if (innerFun->isNative())
> +                {
> +                    clone = innerFun;

Can you add
  JS_ASSERT(innerFun->compartment() == cx->compartment);
?

Comment 6

[Andrew McCreight [:mccr8]]

> JS_ASSERT(innerFun->compartment() == cx->compartment);
Or maybe assertSameCompartment(cx, innerFun).

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed fix addressing comments

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 759377 [details] [diff] [review]
proposed fix addressing comments

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

Thanks again!

::: js/src/jsscript.cpp
@@ +2301,5 @@
>                  clone = CloneStaticBlockObject(cx, enclosingScope, innerBlock);
>              } else if (obj->isFunction()) {
>                  RootedFunction innerFun(cx, obj->toFunction());
> +                if (innerFun->isNative())
> +                {

{ on same line as if

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: same patch witch fixed nit

Carrying r+.

I feel like I should read the Spidermonkey coding style every days.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e50dde140bc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e50dde140bc