648837 - TM: Crash [@ js_NativeGetInline] or [@ js_GetProperty] or "Assertion failure: newShape->slot == lastProperty()->slot,"

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

for (let b = 0; b < 9; b++) {
    function a() {}
    for (d in [<x/>]) {
        new a()
    }
}

crashes js opt shell on JM changeset d3215d1e985a without -m, -a, -j nor -n at js_NativeGetInline or js_GetProperty and asserts js debug shell at Assertion failure: newShape->slot == lastProperty()->slot,

Comment 1

[Brian Hackett [Laid off!]]

This looks like a TM bug.  Similar (but different cause) to bug 642894, we are trying to clone a function that has had properties added to it.  When a JSOP_DEFFUN appears in global code in a loop like this, only a single function object is supposed to be created, right? (js/jsc/d8 agree on this)  The problem is that we do clone the function in the code in the JSOP_DEFFUN case below:

    /*
     * If static link is not current scope, clone fun's object to link to the
     * current scope via parent. We do this to enable sharing of compiled
     * functions among multiple equivalent scopes, amortizing the cost of
     * compilation over a number of executions.  Examples include XUL scripts
     * and event handlers shared among Firefox or other Mozilla app chrome
     * windows, and user-defined JS functions precompiled and then shared among
     * requests in server-side JS.
     */
    if (obj->getParent() != obj2) {
        obj = CloneFunctionObject(cx, fun, obj2);
        if (!obj)
            goto error;
    }

obj->getParent() is the global object, obj2 is the scope chain, a block object for the 'let'.  The first time around the outer loop, the scope chain has not been updated with the block object yet and the function gets a .prototype property from the 'new a()'.  The second time around, the block chain has been created, this test passes and CloneFunctionObject dies.

This test looks wrong to me.  Isn't the comment just describing non-compileAndGo code (in which case the test would be !script->compileAndGo)?  This changes the semantics of the op though and I don't know if this matches the circumstances under which a DEFFUN should be cloned.

Comment 2

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

Attachment: stacks

a = <x><y/></x>;
a.function::__proto__ = null;
-a

This testcase seems to also be related. Tested on JM rev 17cbc8fed578 with -n, 64-bit shell in 10.6.

Comment 3

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

(In reply to comment #2)
> Created attachment 527734 [details]
> stacks
> 
> a = <x><y/></x>;
> a.function::__proto__ = null;
> -a
> 
> This testcase seems to also be related. Tested on JM rev 17cbc8fed578 with -n,
> 64-bit shell in 10.6.

This is more related to bug 642894 than this one.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #1)
> This looks like a TM bug.  Similar (but different cause) to bug 642894, we are
> trying to clone a function that has had properties added to it.  When a
> JSOP_DEFFUN appears in global code in a loop like this, only a single function
> object is supposed to be created, right? (js/jsc/d8 agree on this)

According to SpiderMonkey's semantics for nested function declarations, no, we're supposed to create a function every time around.

Comment 5

[Brian Hackett [Laid off!]]

OK, then the 'new a()' should go through a method barrier, or the function should always have been cloned.  It seems easiest to just always clone in JSOP_DEFFUN, since the interpreter can't tell if it's in a loop or not.  (In the JM branch, CloneFunctionObject does not actually do the clone for functions which have singleton types, i.e. are in compileAndGo code and not in loops or functions.)

Comment 6

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

Attachment: 3rd set of stacks

[].__proto__[Object.getOwnPropertyNames([].__proto__)] = []
eval("\
  (function() {\
    function y(m) {\
      m.__proto__ = function(){};\
      for(v in m) {}\
    }\
    for each(z in[String]) {\
      y(z)\
    }\
  })\
")()
gc()

crashes js opt 64-bit shell on Mac on JM changeset 5bcf457d942c without any CLI parameters at js::Value::isMarkable with gc on the stack, and asserts identically in debug shell.

Comment 7

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

Here's another gc-related testcase, turning bug s-s due to prevalence of gc-related testcases.


function f(a) {
    var b = a[b] = a;
}
f([].__proto__);
function g(c) {
    c.__proto__ = function() {};
    for (d in c) {}
}
for each(let e in [function() {}]) {
    try {
        g(e)
    } catch (m) {}
}
gc();


crashes js opt 64-bit shell on Mac on JM changeset aec367836312 without any CLI parameters at js::GCMarker::drainMarkStack with gc on the stack, and asserts identically in debug shell.

Comment 8

[Johnny Stenback (:jst)]

David, can you find an owner here and someone to suggest a sg: rating for this bug per https://wiki.mozilla.org/Security_Severity_Ratings. This sounds like sg:critical, but please have someone confirm.

Comment 9

[Brian Hackett [Laid off!]]

This bug is on the TM branch, but should be fairly innocuous there --- it breaks assumptions which are only made on the Jaegermonkey branch.

Comment 10

[Brendan Eich [:brendan]]

Cc'ing Jason and Igor. I swear there are other bugs about this.

We need a fast single-term test for when to clone, based on compiler knowledge and not subject to dynamic lazy scope as object reification happening (or not). Case analysis has to cover CNG x global x function x eval-from-global x eval-from-function (some of those may collapse).

/be

Comment 11

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

Note that JM has just landed on TM branch, so this issue is likely to still exist in TM.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

None of the testcases in this bug crash with a current js shell.

Comment 13

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

(In reply to Brendan Eich [:brendan] from comment #10)
> Cc'ing Jason and Igor. I swear there are other bugs about this.
> 
> We need a fast single-term test for when to clone, based on compiler
> knowledge and not subject to dynamic lazy scope as object reification
> happening (or not). Case analysis has to cover CNG x global x function x
> eval-from-global x eval-from-function (some of those may collapse).
> 
> /be

Is this bug, even though marked WFM, still applicable even though the testcases are not crashing?