817002 - Crash [@ js::shadow::Object::numFixedSlots] or "Assertion failure: (l.asBits & 0x8000000000000000LL) == 0," or "Assertion failure: JSVAL_IS_DOUBLE_IMPL(data),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: partially reduced testcase

The attached testcase asserts js debug shell on m-c changeset abb39d1df815 without any CLI arguments at Assertion failure: (l.asBits & 0x8000000000000000LL) == 0,

https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-11-30-mozilla-central-debug/jsshell-mac64.zip

Happens once every 20-30 times, but may be more reproducible than that. s-s because gc is on the stack.


Stack:

Assertion failure: (l.asBits & 0x8000000000000000LL) == 0, at ../../../js/src/jsval.h:767

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x000000010016540f in js::ArrayBufferObject::obj_trace ()
(gdb) bt
#0  0x000000010016540f in js::ArrayBufferObject::obj_trace ()
#1  0x0000000100208e8c in js::ObjectImpl::markChildren ()
#2  0x0000000100099623 in IncrementalCollectSlice ()
#3  0x0000000100098cfe in GCCycle ()
#4  0x0000000100097e1c in Collect ()
#5  0x00000001000654d6 in js::DestroyContext ()
#6  0x0000000100002ecf in main ()
(gdb)

Comment 1

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

Attachment: stack

Another smaller reproducible testcase:

gc()
evalcx("\
    m0 = new WeakMap;\
    t2 = new Uint8ClampedArray;\
    Object.defineProperty(this, \"t1\", { \
        get: function() {\
            return t0.subarray(10, 3);\
        }\
    });\
    t0 = t2.subarray(14, 7);\
    function x() {}\
    gcslice(11); \
    m0.get(t1);\
    gc();\
    mjitChunkLimit(42);\
    t1.toSource = (function() {});\
    evaluate(\"gc()\", {\
        lineNumber: 42,\
        newContext: true,\
        compileAndGo: true,\
        noScriptRval: (x % 4 == 3),\
        catchTermination: false\
    });",
newGlobal(''))

Tested on changeset 6c23f41b0747 - pass in testcase as a CLI argument to a 64-bit debug shell.

Comment 2

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

Attachment: another stack

gc()
evalcx("\
    m0 = new WeakMap;\
    t2 = new Uint8ClampedArray;\
    Object.defineProperty(this, \"t1\", { \
        get: function() {\
            return t0.subarray(10, 3);\
        }\
    });\
    t0 = t2.subarray(14, 7);\
    function x() {}\
    gcslice(11); \
    m0.get(t1);\
    gc();\
    mjitChunkLimit(42);\
    t1.toSource = (function() {});\
    evaluate(\"gc()\", {\
        lineNumber: 42,\
        newContext: true,\
        noScriptRval: (x % 4 == 3),\
        catchTermination: false\
    });",
newGlobal(''))


Here's a variant that crashes 6c23f41b0747 at js::shadow::Object::numFixedSlots instead, on 64-bit debug shell.

Comment 4

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   114224:a65bfc6d1975
user:        Jon Coppeard
date:        Wed Oct 31 17:59:22 2012 +0000
summary:     Bug 790338 - Store the list of arraybufs per compartment rather than on the runtime r=billm

This is causing failures with all sorts of signatures (see duped bug 817365) so marking [fuzzblocker].

Comment 5

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

I also saw:

Assertion failure: JSVAL_IS_DOUBLE_IMPL(data),

during the course of reduction for the testcase in comment 1.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jon, could you take a look at this?

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

The problem occurs when ArrayBufferObject::addView updates the list of views for
a buffer by prepending the new view to the list.

The list of live buffers created during tracing stores the link pointer to the
next buffer on the buffer's first view, so addView sets the new view's next
buffer pointer to that of the previous first view.  But it doesn't unset the
next buffer pointer for that view.

This means that if this view subsequently becomes the first in the buffer, when
the buffer is traced, ArrayBufferObject::obj_trace thinks it's already on the
live buffer list and doesn't add it.  The views list is then not swept and may
therefore end up with pointers to dead views.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 688234 [details] [diff] [review]
Proposed fix

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

::: js/src/jit-test/tests/basic/bug817002.js
@@ +10,5 @@
> +    gc();\
> +    gc();\
> +    array.subarray(10, 3).a = 1;\
> +    gc();",
> +newGlobal(''))

Can you use this simplified form of the test, which avoids some distractions:

gc()
evalcx("\
  if (!('gcslice' in this))\
    gcslice = function() { };\
  array = new Uint8Array;\
  t0 = array.subarray();\
  gcslice(11);\
  array.subarray();\
  gc();\
  gc();\
  array.subarray().a;\
  gc();",
newGlobal())


I tried and failed to come up with something more robust (in particular, that would avoid the gcslice(11) magic number).

::: js/src/jstypedarray.cpp
@@ +374,5 @@
>  
>          // Move the multiview buffer list link into this view since we're
>          // prepending it to the list.
>          SetBufferLink(view, BufferLink(*views));
> +        SetBufferLink(*views, UNSET_BUFFER_LINK);

I think I might have even had that once, and billm made me take it out for reasons that were correct at the time. Or maybe I'm misremembering what that was.

At any rate, thanks. That must have been fun to track down.

Comment 9

[Steve Fink [:sfink] [:s:]]

Actually, do you mind adding something like this in?

diff --git a/js/src/jstypedarray.cpp b/js/src/jstypedarray.cpp
--- a/js/src/jstypedarray.cpp
+++ b/js/src/jstypedarray.cpp
@@ -538,16 +538,25 @@ ArrayBufferObject::obj_trace(JSTracer *t
         if (IS_GC_MARKING_TRACER(trc)) {
             // obj_trace may be called multiple times before sweep(), so avoid
             // adding this buffer to the list multiple times.
             if (BufferLink(firstView) == UNSET_BUFFER_LINK) {
                 JS_ASSERT(obj->compartment() == firstView->compartment());
                 JSObject **bufList = &obj->compartment()->gcLiveArrayBuffers;
                 SetBufferLink(firstView, *bufList);
                 *bufList = obj;
+            } else {
+#ifdef DEBUG
+                bool found = false;
+                for (JSObject *p = obj->compartment()->gcLiveArrayBuffers; p; p = BufferLink(p)) {
+                    if (p == obj)
+                        found = true;
+                }
+                JS_ASSERT(found);
+#endif
             }
         }
     }
 }
 
 void
 ArrayBufferObject::sweep(JSCompartment *compartment)
 {

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] from comment #9)

Re the test case, I had to add back this assignment to make it fail for me:

  array.subarray().a = 1;\

> Actually, do you mind adding something like this in?

Good idea, done.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e21b82f790f

Comment 12

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

I added Julian and rillian on the cc list because they were working on failures in Valgrind that are possibly related to this bug, along in bug 815931 (~ comment 19).

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9e21b82f790f

Comment 14

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

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.