Last Comment Bug 878520 - "Assertion failure: isInterpreted()" with asm module inside recursive function
: "Assertion failure: isInterpreted()" with asm module inside recursive function
Status: RESOLVED FIXED
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla24
Assigned To: Benjamin Bouvier [:bbouvier]
:
Mentors:
: 879132 (view as bug list)
Depends on:
Blocks: jsfunfuzz odinfuzz 851421
  Show dependency treegraph
 
Reported: 2013-06-01 19:53 PDT by Jesse Ruderman
Modified: 2013-06-07 08:32 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (2.84 KB, patch)
2013-06-04 11:56 PDT, Benjamin Bouvier [:bbouvier]
luke: review+
Details | Diff | Splinter Review
proposed fix addressing comments (3.31 KB, patch)
2013-06-06 13:22 PDT, Benjamin Bouvier [:bbouvier]
luke: review+
Details | Diff | Splinter Review
same patch witch fixed nit (3.30 KB, patch)
2013-06-06 14:50 PDT, Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2013-06-01 19:53:36 PDT
(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] 2013-06-04 11:56:11 PDT
Created attachment 758080 [details] [diff] [review]
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 2 Benjamin Bouvier [:bbouvier] 2013-06-04 11:58:55 PDT
*** Bug 879132 has been marked as a duplicate of this bug. ***
Comment 3 Nicholas Nethercote [:njn] 2013-06-04 16:42:28 PDT
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] 2013-06-05 17:35:21 PDT
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] 2013-06-05 17:45:03 PDT
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] 2013-06-05 17:51:20 PDT
> JS_ASSERT(innerFun->compartment() == cx->compartment);
Or maybe assertSameCompartment(cx, innerFun).
Comment 7 Benjamin Bouvier [:bbouvier] 2013-06-06 13:22:52 PDT
Created attachment 759377 [details] [diff] [review]
proposed fix addressing comments
Comment 8 Luke Wagner [:luke] 2013-06-06 14:20:14 PDT
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] 2013-06-06 14:50:30 PDT
Created attachment 759430 [details] [diff] [review]
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] 2013-06-06 14:54:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e50dde140bc
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-06-07 08:32:52 PDT
https://hg.mozilla.org/mozilla-central/rev/1e50dde140bc

Note You need to log in before you can comment on or make changes to this bug.