481989 - TM: " Assertion failure: SPROP_HAS_STUB_SETTER(sprop), at ../jstracer.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

y = this.watch("x", function(){}); for each (let y in ['', '']) x = y

Assertion failure: SPROP_HAS_STUB_SETTER(sprop), at ../jstracer.cpp:7019

Seems to work as expected, both in opt and without -j. Possible regression of bug 476871?

Comment 1

[Andreas Gal :gal]

Attachment: patch

for (var i = 0; i < 100; i++)
    {
        var o = {valueOf: undefined, toString: toString};
        var q = 5 + o;
    }

The attached patch fails to compile the above code from trace-tests.js because of:

00020:   3  name "undefined"
00023:   3  initprop "valueOf"
abort: 1840: slotless obj property
Abort recording of tree x.js:1@17 at x.js:3@23: SetPropHit.

I wonder whether thats true and we shouldn't be tracing this.

Comment 2

[Brendan Eich [:brendan]]

Argh, activeCallOrGlobalSlot has grown three ABORT_TRACE hidden returns before obligatory OBJ_DROP_PROPERTY, first one r=me by graydon, then two by you based on it. Sorry -- we should probably use an explicit return and avoid repeated DROP then ABORT then-clauses.

Anyway, I don't se why initprop should abort in that case with the patch. What is sprop->slot, what is scope->map.freeslot, when that "slotless obj property" abort happens?

/be

Comment 3

[Brendan Eich [:brendan]]

Argh^2, the TRACE_2(SetPropHit...) in js_Interpret's JSOP_INITPROP case is too early. It should come just before the break from the do-while(0); loop.

This doesn't affect the rules for OBJ_DROP_PROPERTY after OBJ_LOOKUP_PROPERTY (or a call to a wrapper around it, namely js_FindProperty -- or to its concrete impl js_LookupProperty).

/be

Comment 4

[Andreas Gal :gal]

Attachment: v2

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 366081 [details] [diff] [review]
v2

>+TraceRecorder::isDirectSlot(JSScope* scope, JSScopeProperty* sprop)

Should be isValidSlot or some such. Longer names possible, I like that one currently.

>+{
>+    uint32 setflags = (js_CodeSpec[*cx->fp->regs->pc].format & (JOF_SET | JOF_INCDEC | JOF_FOR));
>+
>+    if (setflags && !SPROP_HAS_STUB_SETTER(sprop))
>+        ABORT_TRACE("non-stub setter");
>+    if (setflags != JOF_SET && !SPROP_HAS_STUB_GETTER(sprop))
>+        ABORT_TRACE("non-stub getter");
>+    if (setflags && (sprop->attrs & JSPROP_READONLY))
>+        ABORT_TRACE("writing to a read-only property");

Two of the above three ifs start with setflags, so common that:

    if (setflags) {
        if (!SPROP_HAS_STUB_SETTER(sprop))
            ABORT_TRACE("non-stub setter");
        if (sprop->attrs & JSPROP_READONLY)
            ABORT_TRACE("writing to a read-only property");
    }

    /* This check applies even when setflags == 0. */
    if (setflags != JOF_SET && !SPROP_HAS_STUB_GETTER(sprop))
        ABORT_TRACE("non-stub getter");

The setflags != JOF_SET case has to work even when !setflags. Worth a comment?

r=me with above addressed.

/be

Comment 6

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/4c66b8ccb1b8

Comment 7

[Bob Clary [:bc] (inactive)]

also covered by js1_5/extensions/regress-455413.js. 

definitely regressed by bug 476871 http://hg.mozilla.org/tracemonkey/rev/1852d9fe853d

This was tracemonkey only right? Can we mark if fixed and move on?

Comment 8

[Brendan Eich [:brendan]]

Looks like tm only.

/be

Comment 9

[Bob Clary [:bc] (inactive)]

also covered by js1_5/Regress/regress-210682.js, js1_5/Regress/regress-462989.js (browser at least)

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4c66b8ccb1b8

Comment 11

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/20c36d75e58a
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-481989.js,v  <--  regress-481989.js
initial revision: 1.1

Comment 12

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7f4a7e78eb95

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2 modulo js1_5/Regress/regress-210682.js (bug 488203 which should be fixed as well).