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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: gkw, Assigned: gal)

Tracking

(Blocks: 1 bug, {assertion, testcase, verified1.9.1})

Trunk
x86
Linux
assertion, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

v2
8.83 KB, patch
brendan
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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?
Flags: blocking1.9.1?
(Assignee)

Updated

10 years ago
Assignee: general → gal
(Assignee)

Comment 1

10 years ago
Created attachment 366077 [details] [diff] [review]
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.
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
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
(Assignee)

Comment 4

10 years ago
Created attachment 366081 [details] [diff] [review]
v2
Attachment #366077 - Attachment is obsolete: true
Attachment #366081 - Flags: review?(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
Attachment #366081 - Flags: review?(brendan) → review+
(Assignee)

Comment 6

10 years ago
http://hg.mozilla.org/tracemonkey/rev/4c66b8ccb1b8
Whiteboard: fixed-in-tracemonkey
(Reporter)

Updated

10 years ago
Flags: in-testsuite?
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?
Blocks: 476871
Looks like tm only.

/be
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
also covered by js1_5/Regress/regress-210682.js, js1_5/Regress/regress-462989.js (browser at least)

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+
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
Flags: in-testsuite? → in-testsuite+
v 1.9.1, 1.9.2 modulo js1_5/Regress/regress-210682.js (bug 488203 which should be fixed as well).
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
(Reporter)

Updated

9 years ago
Depends on: 495962
(Reporter)

Updated

9 years ago
Depends on: 498137
(Assignee)

Updated

9 years ago
No longer depends on: 495962
You need to log in before you can comment on or make changes to this bug.