Assertion failure: obj->lastProperty() == templateObject->as<ArrayObject>().lastProperty(), at vm/Interpreter.cpp

RESOLVED FIXED in Firefox 47

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gkw, Assigned: jimb)

Tracking

(Blocks 2 bugs, {assertion, regression, testcase})

Trunk
mozilla47
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox45 affected, firefox47 fixed)

Details

(Whiteboard: [jsbugmon:update,ignore])

Attachments

(8 attachments, 2 obsolete attachments)

4.59 KB, text/plain
Details
10.65 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
7.93 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
2.42 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
1.39 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
1.62 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.14 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
1.85 KB, patch
Details | Diff | Splinter Review
// jsfunfuzz-generated
s = newGlobal();
// Adapted from randomly chosen test: js/src/jit-test/tests/ion/bug1054241.js
evalcx("enableShellObjectMetadataCallback(); Array;", s);
// jsfunfuzz-generated
evalcx("for (var w = 0; w < 99; w++) { this[0] = []; }", s);

asserts js debug shell on m-c changeset 451a18579143 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: obj->lastProperty() == templateObject->as<ArrayObject>().lastProperty(), at vm/Interpreter.cpp

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 451a18579143

autoBisect is running.
Posted file stack
(lldb) bt 5
* thread #1: tid = 0x6fc351, 0x000000010069ad4b js-dbg-64-dm-darwin-451a18579143`js::NewArrayOperationWithTemplate(cx=<unavailable>, templateObject=<unavailable>) + 555 at Interpreter.cpp:4729, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010069ad4b js-dbg-64-dm-darwin-451a18579143`js::NewArrayOperationWithTemplate(cx=<unavailable>, templateObject=<unavailable>) + 555 at Interpreter.cpp:4729
    frame #1: 0x00000001009593ab js-dbg-64-dm-darwin-451a18579143`js::jit::DoNewArray(cx=0x0000000102669400, frame=<unavailable>, stub=<unavailable>, length=<unavailable>, res=<unavailable>) + 123 at BaselineIC.cpp:980
    frame #2: 0x0000000101eebd47
    frame #3: 0x0000000101ee9dc4
    frame #4: 0x000000010017da29 js-dbg-64-dm-darwin-451a18579143`EnterBaseline(cx=<unavailable>, data=0xfff8800000000063) + 489 at BaselineJIT.cpp:126
(lldb)
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2d5da0d1ed25
user:        Jim Blandy
date:        Wed Jul 29 13:41:26 2015 -0700
summary:     Bug 1189059: Replace setObjectMetadataCallback with enableObjectMetadataCallback, fix callers. r=fitzgen

Jim/Nick, is bug 1189059 a likely regressor?

I replaced enableObjectMetadataCallback with setObjectMetadataCallback and ran it against the parent revision, but didn't seem to reproduce.
Blocks: 1189059
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
I can reproduce.
Assignee: nobody → jimb
Flags: needinfo?(jimb)
Flags: needinfo?(nfitzgerald)
Actually, I can reproduce this bug in the *parent* of my changeset, as well, if I rename the function being called. So there's a legitimate bug here, but it's not introduced by my changeset.

s = newGlobal();
evalcx("setObjectMetadataCallback(true); Array;", s);
evalcx("for (var w = 0; w < 99; w++) { this[0] = []; }", s);
(In reply to Jim Blandy :jimb from comment #4)
> Actually, I can reproduce this bug in the *parent* of my changeset, as well,
> if I rename the function being called. So there's a legitimate bug here, but
> it's not introduced by my changeset.

Hmmm, strange. I didn't seem to reproduce. Anyway, are you able to debug this further, or shall we punt to someone else?
Flags: needinfo?(jimb)
Here's the real regressor:

The first bad revision is:
changeset:   259739:020c6a559e3a
user:        Brian Hackett <bhackett1024@gmail.com>
date:        Sun May 03 08:14:04 2015 -0700
summary:     Bug 1146597 - Add unboxed arrays for JSOP_NEWARRAY arrays, and shell option for using them, r=jandem.
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> Hmmm, strange. I didn't seem to reproduce. Anyway, are you able to debug
> this further, or shall we punt to someone else?

You have to change the test code slightly.

I can debug this.
Flags: needinfo?(jimb)
OS: Mac OS X → All
Hardware: x86_64 → All
Hi, Brian! In changeset 599a8abf54a3 the test case given in comment 4 doesn't crash, but in the successor changeset 020c6a559e3a, it does hit the given assertion. That was a change you made (see comment 6).

I'm debugging this, but do you have any insight into what the problem might be?
Flags: needinfo?(bhackett1024)
The reason for the crash is that DoNewArray in BaselineIC takes the template object from the stub, but then NewArray finds a hit in the new object cache, and the cache entry's template object and the stub's template object have different shapes.

What's surprising is that the two shapes seem to be identical. Here is the shape from the new object cache's template object:

(gdb) p *templateObj->shape_.value
$37 = {
  <js::gc::TenuredCell> = {
    <js::gc::Cell> = {<No data fields>}, <No data fields>}, 
  members of js::Shape: 
  base_ = {
    <js::BarrieredBase<js::BaseShape*>> = {
      <js::BarrieredBaseMixins<js::BaseShape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::BaseShape*>: 
      value = 0x7f7fc2f74420
    }, <No data fields>}, 
  propid_ = {
    <js::BarrieredBase<jsid>> = {
      <js::BarrieredBaseMixins<jsid>> = {<No data fields>}, 
      members of js::BarrieredBase<jsid>: 
      value = {
        asBits = 140186708198376
      }
    }, <No data fields>}, 
  slotInfo = 16777215, 
  attrs = 196 '\304', 
  flags = 8 '\b', 
  parent = {
    <js::BarrieredBase<js::Shape*>> = {
      <js::BarrieredBaseMixins<js::Shape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::Shape*>: 
      value = 0x7f7fc2f8b830
    }, <No data fields>}, 
  {
    kids = {
      w = 0
    }, 
    listp = 0x0
  }
}

and here is its parent:

(gdb) p *$37->parent.value
$42 = {
  <js::gc::TenuredCell> = {
    <js::gc::Cell> = {<No data fields>}, <No data fields>}, 
  members of js::Shape: 
  base_ = {
    <js::BarrieredBase<js::BaseShape*>> = {
      <js::BarrieredBaseMixins<js::BaseShape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::BaseShape*>: 
      value = 0x7f7fc2f74420
    }, <No data fields>}, 
  propid_ = {
    <js::BarrieredBase<jsid>> = {
      <js::BarrieredBaseMixins<jsid>> = {<No data fields>}, 
      members of js::BarrieredBase<jsid>: 
      value = {
        asBits = 4
      }
    }, <No data fields>}, 
  slotInfo = 33554431, 
  attrs = 64 '@', 
  flags = 0 '\000', 
  parent = {
    <js::BarrieredBase<js::Shape*>> = {
      <js::BarrieredBaseMixins<js::Shape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::Shape*>: 
      value = 0x0
    }, <No data fields>}, 
  {
    kids = {
      w = 140186708629984
    }, 
    listp = 0x7f7fc2f859e0
  }
}

Whereas, here is the shape from the IC stub's template object:

(gdb) p *((NativeObject*)templateObject.ptr)->shape_.value
$40 = {
  <js::gc::TenuredCell> = {
    <js::gc::Cell> = {<No data fields>}, <No data fields>}, 
  members of js::Shape: 
  base_ = {
    <js::BarrieredBase<js::BaseShape*>> = {
      <js::BarrieredBaseMixins<js::BaseShape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::BaseShape*>: 
      value = 0x7f7fc2f74420
    }, <No data fields>}, 
  propid_ = {
    <js::BarrieredBase<jsid>> = {
      <js::BarrieredBaseMixins<jsid>> = {<No data fields>}, 
      members of js::BarrieredBase<jsid>: 
      value = {
        asBits = 140186708198376
      }
    }, <No data fields>}, 
  slotInfo = 16777215, 
  attrs = 196 '\304', 
  flags = 8 '\b', 
  parent = {
    <js::BarrieredBase<js::Shape*>> = {
      <js::BarrieredBaseMixins<js::Shape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::Shape*>: 
      value = 0x7f7fc2f8b8f8
    }, <No data fields>}, 
  {
    kids = {
      w = 0
    }, 
    listp = 0x0
  }
}

and its parent:

(gdb) p *$40->parent.value
$43 = {
  <js::gc::TenuredCell> = {
    <js::gc::Cell> = {<No data fields>}, <No data fields>}, 
  members of js::Shape: 
  base_ = {
    <js::BarrieredBase<js::BaseShape*>> = {
      <js::BarrieredBaseMixins<js::BaseShape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::BaseShape*>: 
      value = 0x7f7fc2f74420
    }, <No data fields>}, 
  propid_ = {
    <js::BarrieredBase<jsid>> = {
      <js::BarrieredBaseMixins<jsid>> = {<No data fields>}, 
      members of js::BarrieredBase<jsid>: 
      value = {
        asBits = 4
      }
    }, <No data fields>}, 
  slotInfo = 33554431, 
  attrs = 64 '@', 
  flags = 0 '\000', 
  parent = {
    <js::BarrieredBase<js::Shape*>> = {
      <js::BarrieredBaseMixins<js::Shape*>> = {<No data fields>}, 
      members of js::BarrieredBase<js::Shape*>: 
      value = 0x0
    }, <No data fields>}, 
  {
    kids = {
      w = 140186708630040
    }, 
    listp = 0x7f7fc2f85a18
  }
}

The two chains are identical, except for first shapes' parent links (obviously) and then the kids table (which I believe isn't considered in shape identity). Since these aren't owned shapes, I'm not sure why we have two of them.

My understanding of what's going on here is pretty weak.
The 'Array' in the test case is critical, because it causes us to call CreateArrayPrototype while the metadata callback is active. The metadata callback is invoked for Array.prototype. ShellObjectMetadataCallback allocates another array. Very suspicious.
Nested calls to CreateArrayPrototype. Seems bad.

(gdb) where
#0  CreateArrayPrototype (cx=0x7feae521b400, key=JSProto_Array) at /home/jimb/moz/dbg/js/src/jsarray.cpp:3230
#1  0x0000000000b5ceb3 in js::GlobalObject::resolveConstructor (cx=0x7feae521b400, global=..., key=JSProto_Array) at /home/jimb/moz/dbg/js/src/vm/GlobalObject.cpp:158
#2  0x0000000000a02ea9 in MaybeResolveConstructor (cxArg=0x7feae521b400, global=..., key=JSProto_Array) at /home/jimb/moz/dbg/js/src/jsobj.cpp:2005
#3  0x0000000000a03051 in js::GetBuiltinPrototype (cx=0x7feae521b400, key=JSProto_Array, protop=...) at /home/jimb/moz/dbg/js/src/jsobj.cpp:2025
#4  0x0000000000501a7c in NewArray<0u> (cxArg=0x7feae521b400, length=0, protoArg=..., newKind=js::GenericObject) at /home/jimb/moz/dbg/js/src/jsarray.cpp:3354
#5  0x00000000004fe9c2 in js::NewDenseEmptyArray (cx=0x7feae521b400, proto=..., newKind=js::GenericObject) at /home/jimb/moz/dbg/js/src/jsarray.cpp:3407
#6  0x00000000009051dc in ShellObjectMetadataCallback (cx=0x7feae521b400) at /home/jimb/moz/dbg/js/src/builtin/TestingFunctions.cpp:1523
#7  0x000000000099bace in JSCompartment::setNewObjectMetadata (this=0x7feae5256000, cx=0x7feae521b400, obj=0x7feadc6850c0) at /home/jimb/moz/dbg/js/src/jscompartment.cpp:906
#8  0x0000000000994a41 in js::SetNewObjectMetadata (cxArg=0x7feae521b400, obj=0x7feadc6850c0) at /home/jimb/moz/dbg/js/src/jsobjinlines.h:294
#9  0x000000000099ca89 in js::AutoSetNewObjectMetadata::~AutoSetNewObjectMetadata (this=0x7ffda92d77d0, __in_chrg=<optimized out>) at /home/jimb/moz/dbg/js/src/jscompartment.cpp:1195
#10 0x00000000004fe85d in CreateArrayPrototype (cx=0x7feae521b400, key=JSProto_Array) at /home/jimb/moz/dbg/js/src/jsarray.cpp:3244
#11 0x0000000000b5ceb3 in js::GlobalObject::resolveConstructor (cx=0x7feae521b400, global=..., key=JSProto_Array) at /home/jimb/moz/dbg/js/src/vm/GlobalObject.cpp:158
#12 0x0000000000b5cc38 in js::GlobalObject::ensureConstructor (cx=0x7feae521b400, global=..., key=JSProto_Array) at /home/jimb/moz/dbg/js/src/vm/GlobalObject.cpp:98
#13 0x0000000000971063 in JS_ResolveStandardClass (cx=0x7feae521b400, obj=..., id=..., resolved=0x7ffda92d7c4f) at /home/jimb/moz/dbg/js/src/jsapi.cpp:1094
#14 0x000000000044163c in global_resolve (cx=0x7feae521b400, obj=..., id=..., resolvedp=0x7ffda92d7c4f) at /home/jimb/moz/dbg/js/src/shell/js.cpp:5325
#15 0x00000000009ceae8 in js::CallResolveOp (cx=0x7feae521b400, obj=..., id=..., propp=..., recursedp=0x7ffda92d7d1f) at /home/jimb/moz/dbg/js/src/vm/NativeObject-inl.h:391
#16 0x0000000000a10171 in js::LookupOwnPropertyInline<(js::AllowGC)1> (cx=0x7feae521b400, obj=..., id=..., propp=..., donep=0x7ffda92d7dbf) at /home/jimb/moz/dbg/js/src/vm/NativeObject-inl.h:480
#17 0x0000000000a0eb31 in js::LookupPropertyInline<(js::AllowGC)1> (cx=0x7feae521b400, obj=..., id=..., objp=..., propp=...) at /home/jimb/moz/dbg/js/src/vm/NativeObject-inl.h:554
#18 0x0000000000a03550 in js::LookupProperty (cx=0x7feae521b400, obj=..., id=..., objp=..., propp=...) at /home/jimb/moz/dbg/js/src/jsobj.cpp:2148
#19 0x0000000000a03654 in js::LookupName (cx=0x7feae521b400, name=..., scopeChain=..., objp=..., pobjp=..., propp=...) at /home/jimb/moz/dbg/js/src/jsobj.cpp:2158
#20 0x0000000000b684a0 in GetNameOperation (cx=0x7feae521b400, fp=0x7feadbf630d8, pc=0x7feae52b5cd7 "\232", vp=...) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:289
#21 0x0000000000b79374 in Interpret (cx=0x7feae521b400, state=...) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:2925
#22 0x0000000000b69154 in js::RunScript (cx=0x7feae521b400, state=...) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:430
#23 0x0000000000b6a57f in js::ExecuteKernel (cx=0x7feae521b400, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=js::EXECUTE_INDIRECT_EVAL, evalInFrame=..., result=0x7ffda92da3a8) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:703
#24 0x00000000005fb56a in EvalKernel (cx=0x7feae521b400, args=..., evalType=INDIRECT_EVAL, caller=..., scopeobj=..., pc=0x0) at /home/jimb/moz/dbg/js/src/builtin/Eval.cpp:353
#25 0x00000000005fc09f in js::IndirectEval (cx=0x7feae521b400, argc=1, vp=0x7ffda92da3a8) at /home/jimb/moz/dbg/js/src/builtin/Eval.cpp:456
#26 0x0000000000b8caea in js::CallJSNative (cx=0x7feae521b400, native=0x5fbfa7 <js::IndirectEval(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/jimb/moz/dbg/js/src/jscntxtinlines.h:235
#27 0x0000000000b69539 in js::Invoke (cx=0x7feae521b400, args=..., construct=js::NO_CONSTRUCT) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:489
#28 0x0000000000b6992d in js::Invoke (cx=0x7feae521b400, thisv=..., fval=..., argc=1, argv=0x7feadbf630b8, rval=...) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:542
#29 0x0000000000ac56c3 in js::DirectProxyHandler::call (this=0x1350650 <js::CrossCompartmentWrapper::singleton>, cx=0x7feae521b400, proxy=..., args=...) at /home/jimb/moz/dbg/js/src/proxy/DirectProxyHandler.cpp:77
#30 0x0000000000ab8180 in js::CrossCompartmentWrapper::call (this=0x1350650 <js::CrossCompartmentWrapper::singleton>, cx=0x7feae521b400, wrapper=..., args=...) at /home/jimb/moz/dbg/js/src/proxy/CrossCompartmentWrapper.cpp:289
#31 0x0000000000ac8cee in js::Proxy::call (cx=0x7feae521b400, proxy=..., args=...) at /home/jimb/moz/dbg/js/src/proxy/Proxy.cpp:412
#32 0x0000000000ac9fb7 in js::proxy_Call (cx=0x7feae521b400, argc=1, vp=0x7feadbf630a8) at /home/jimb/moz/dbg/js/src/proxy/Proxy.cpp:710
#33 0x0000000000b8caea in js::CallJSNative (cx=0x7feae521b400, native=0xac9ee9 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/jimb/moz/dbg/js/src/jscntxtinlines.h:235
#34 0x0000000000b6943d in js::Invoke (cx=0x7feae521b400, args=..., construct=js::NO_CONSTRUCT) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:477
#35 0x0000000000b785f0 in Interpret (cx=0x7feae521b400, state=...) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:2798
#36 0x0000000000b69154 in js::RunScript (cx=0x7feae521b400, state=...) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:430
#37 0x0000000000b6a57f in js::ExecuteKernel (cx=0x7feae521b400, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=js::EXECUTE_GLOBAL, evalInFrame=..., result=0x0) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:703
#38 0x0000000000b6a94b in js::Execute (cx=0x7feae521b400, script=..., scopeChainArg=..., rval=0x0) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:742
#39 0x000000000097d768 in ExecuteScript (cx=0x7feae521b400, scope=..., script=..., rval=0x0) at /home/jimb/moz/dbg/js/src/jsapi.cpp:4389
#40 0x000000000097db36 in JS_ExecuteScript (cx=0x7feae521b400, scriptArg=...) at /home/jimb/moz/dbg/js/src/jsapi.cpp:4422
#41 0x000000000042fb5e in RunFile (cx=0x7feae521b400, filename=0x7ffda92de02e "/home/jimb/moz/fuzz.js", file=0x7feadbe06800, compileOnly=false) at /home/jimb/moz/dbg/js/src/shell/js.cpp:510
#42 0x0000000000430348 in Process (cx=0x7feae521b400, filename=0x7ffda92de02e "/home/jimb/moz/fuzz.js", forceTTY=false) at /home/jimb/moz/dbg/js/src/shell/js.cpp:629
#43 0x0000000000443a09 in ProcessArgs (cx=0x7feae521b400, op=0x7ffda92dc8f0) at /home/jimb/moz/dbg/js/src/shell/js.cpp:5995
#44 0x0000000000444c31 in Shell (cx=0x7feae521b400, op=0x7ffda92dc8f0, envp=0x7ffda92dcb58) at /home/jimb/moz/dbg/js/src/shell/js.cpp:6320
#45 0x0000000000445fce in main (argc=3, argv=0x7ffda92dcb38, envp=0x7ffda92dcb58) at /home/jimb/moz/dbg/js/src/shell/js.cpp:6677
(gdb)
Indeed, GlobalObject.cpp:163 in GlobalObject::resolveConstructor is getting executed twice for the same global object, because of the nested prototype resolution caused by ShellObjectMetadataCallback:

        global->setPrototype(key, ObjectValue(*proto));

Surely this is a problem...
Here's a patch that fixes the crash. I don't know if it's kosher for us to keep abusing the activeAnalysis flag to prevent collection of object metadata.
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 85cf2e720a84).
Jim, is the patch in comment 13 ready for review/landing? Bug 1228575 might be the same issue/a dupe.
Flags: needinfo?(jimb)
No; I want to replace the AutoEnterAnalysis with something more lightweight. Nick suggested an adequate compromise the other day; I'm just waiting for a spare moment to implement it.
Flags: needinfo?(jimb)
Duplicate of this bug: 1228575
Attachment #8684693 - Attachment is obsolete: true
Attachment #8703991 - Flags: review?(nfitzgerald)
Comment on attachment 8703991 [details] [diff] [review]
Properly root object passed to the allocation metadata callback.

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

Should we check-in the fuzzer's test case as part of this patch or the other one?

::: js/src/jsobjinlines.h
@@ +283,5 @@
> +// object unconditionally, even though collecting metadata is rare. Instead,
> +// SetNewObjectMetadata's contract is that the caller must use the pointer
> +// returned in place of the pointer passed. If a GC occurs, the returned pointer
> +// may be the passed pointer, relocated by GC. If no GC could occur, it's just
> +// passed through. We root nothing unless necessary.

Thanks for this comment, this is very elucidating.

@@ +284,5 @@
> +// SetNewObjectMetadata's contract is that the caller must use the pointer
> +// returned in place of the pointer passed. If a GC occurs, the returned pointer
> +// may be the passed pointer, relocated by GC. If no GC could occur, it's just
> +// passed through. We root nothing unless necessary.
> +static MOZ_ALWAYS_INLINE MOZ_WARN_UNUSED_RESULT JSObject*

MOZ_WARN_UNUSED_RESULT \o/

::: js/src/vm/Runtime-inl.h
@@ +70,5 @@
>      if (group->clasp()->shouldDelayMetadataCallback())
>          cx->compartment()->setObjectPendingMetadata(cx, obj);
> +    else {
> +        obj = static_cast<NativeObject*>(SetNewObjectMetadata(cx, obj));
> +    }

I don't think we want the {} here, unless my understanding of SM style has been wrong for a long time (very possible!)

Or, if we do want them, then I think the consequent should get them some as well.
Attachment #8703991 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8703992 [details] [diff] [review]
Use a dedicated flag on JS::Zone to disable allocatation metadata collection, instead of abusing AutoEnterAnalysis.

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

Just as we discussed in person -- nice!
Attachment #8703992 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #21)
> Comment on attachment 8703991 [details] [diff] [review]
> Properly root object passed to the allocation metadata callback.
> 
> Review of attachment 8703991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Should we check-in the fuzzer's test case as part of this patch or the other
> one?

I'm working on a test that reproduces the problem on tip efficiently.


> ::: js/src/vm/Runtime-inl.h
> @@ +70,5 @@
> >      if (group->clasp()->shouldDelayMetadataCallback())
> >          cx->compartment()->setObjectPendingMetadata(cx, obj);
> > +    else {
> > +        obj = static_cast<NativeObject*>(SetNewObjectMetadata(cx, obj));
> > +    }
> 
> I don't think we want the {} here, unless my understanding of SM style has
> been wrong for a long time (very possible!)

You're right. I have no idea why they're there. Fixed here and elsewhere.
Just asking for feedback here, since I don't have a good test case yet.
Attachment #8704283 - Flags: feedback?(nfitzgerald)
Comment on attachment 8704283 [details] [diff] [review]
Don't collect allocation metadata when lazily creating standard prototypes.

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

Didn't I just do this in another bug?
Attachment #8704283 - Flags: feedback?(nfitzgerald) → feedback+
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #25)
> Didn't I just do this in another bug?

MAYBE YOU DID... I was sure I'd attached this patch and requested feedback, but then I couldn't find it, so I assumed Bugzilla had just eaten. Maybe I attached it to the wrong bug... :(
I was trying to come up with a more direct test case than the fuzzer test, because ShellObjectMetadataCallback may well go away soon (see bug 1236748). But it's pretty hard to trigger.

When the fuzzer test evaluates the reference to Array, we have to resolve Array.prototype for the global s. Call this the "first prototype". The creation of the first prototype itself triggers the metadata callback. The ShellObjectMetadataCallback itself allocates an array, so we re-enter the Array.prototype resolution, and create the "second prototype".

ShellObjectMetadataCallback then actually allocates an array instance, whose guts get preserved in the new object cache. This cache entry uses the second prototype.

As we finish the inner Array.prototype resolution, the second prototype gets saved in the global object's reserved slot for JSProto_Array. But then as we finish the outer Array.prototype resolution, we overwrite that with the first prototype.

So now we've got a new object cache entry referring to the second prototype, but the global object's JSProto_Array slot referring to the first prototype, and we're ready to have a good time.

BaselineIC.cpp's DoNewArray allocates a template object if its stub doesn't have one yet. This is the case the first time we call it. However, the template object is allocated with TenuredObject, which makes us skip the new object cache; we get a template object that uses the first prototype.

On the second call to DoNewArray, the stub has a template object, so we call NewArrayOperationWithTemplate. This allocation, however, is cacheable, so we hit the new object cache, and get an Array with the second prototype. Since the template object uses the first prototype, we crash.

If we assert that globals' JSProto_ slots are empty before we populate them, that should catch all nested prototype resolutions, no IC magic needed. I'm a little concerned that nested resolutions might be so common that this won't be practical; we'll cross that bridge if we come to it.

For regression testing, I think I'll add a mysterious debug-only option that causes Debugger's metadata hook to allocate an array, to verify that metadata hooks can allocate whatever they like without incurring the wrath of demons that were supposed to remain invisible.
(In reply to Jim Blandy :jimb from comment #27)
> If we assert that globals' JSProto_ slots are empty before we populate them,
> that should catch all nested prototype resolutions, no IC magic needed. I'm
> a little concerned that nested resolutions might be so common that this
> won't be practical; we'll cross that bridge if we come to it.

I'd rather not be proud of being cynical, but indeed, asserting that we make only one prototype per class per global catches a lot of bugs. Looking into ways to weaken this.
Now, with a test case.
Attachment #8704283 - Attachment is obsolete: true
Attachment #8708159 - Flags: review?(nfitzgerald)
This is the assertion that makes the test case in the previous patch crash.
Attachment #8708160 - Flags: review?(nfitzgerald)
Attachment #8708159 - Flags: review?(nfitzgerald) → review+
Attachment #8708160 - Flags: review?(nfitzgerald) → review+
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6593a4c57c9a&selectedJob=15521506

I just know this isn't going to be smooth.
I wrote some implicit constructors by accident.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f0d0b8f63b
Jim, just wondering if this fell off your radar?
Flags: needinfo?(jimb)
Sorry, was distracted by other stuff. I have a CGC regression which is probably my fault in comment 32's try push. Working on it.
Flags: needinfo?(jimb)
Seems to be a problem with the rooting changes. I can reproduce the CGC failure under RR.
Okay, I know more about this now. I think the problem is that UnboxedPlainObject needs to set the JSCLASS_DELAY_METADATA_CALLBACK flag, because it doesn't initialize its expando_ field until the call to initExpando in UnboxedPlainObject::create, whereas JSObject::create will call SetNewObjectMetadata before that.

The crash occurs because the call to SetNewObjectMetadata causes a compacting GC, which causes RelocateCell to try to relocate the object in its partially-initialized state. In this particular case, the call to setInitialShapeMaybeNonNative has dropped a shape in there, even though in the case of an UnboxedPlainObject, that word holds the expando_ field, not a shape field.

Now I'm getting kind of worried about this patch. If there are a lot of classes out there that are missing their JSCLASS_DELAY_METADATA_CALLBACK flags, then allowing GC's to occur during metadata callbacks under some circumstances (as the patch does now) will cause this kind of problem for all of them.

I guess if classes are missing that flag, we've potentially got crasher bugs already; is that correct, Nick? Can you think of ways to make omitting the flag crashier?
Flags: needinfo?(nfitzgerald)
New try push with that fix in place: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5165669b191
(In reply to Jim Blandy :jimb from comment #36)
> The crash occurs because the call to SetNewObjectMetadata causes a
> compacting GC, which causes RelocateCell to try to relocate the object in
> its partially-initialized state.

SetNewObjectMetadata has an AutoEnterAnalysis, which suppresses GC. Is the GC triggered somewhere else?
Flags: needinfo?(jimb)
My patch removes that AutoEnterAnalysis, and re-roots the metadata paths. With the patch, it's only necessary to forbid when invoking a metadata callback from AutoSetNewObjectMetadata.
Flags: needinfo?(jimb)
Comment on attachment 8715112 [details] [diff] [review]
Add JSCLASS_DELAY_METADATA_CALLBACK flag to UnboxedPlainObject.

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

LGTM.

Let's hope not too many Classes will need this treatment.
Attachment #8715112 - Flags: review?(jdemooij) → review+
(In reply to Jim Blandy :jimb from comment #36)
> Now I'm getting kind of worried about this patch. If there are a lot of
> classes out there that are missing their JSCLASS_DELAY_METADATA_CALLBACK
> flags, then allowing GC's to occur during metadata callbacks under some
> circumstances (as the patch does now) will cause this kind of problem for
> all of them.

We should probably do a manual census of FooObject::Create methods.

> I guess if classes are missing that flag, we've potentially got crasher bugs
> already; is that correct, Nick? Can you think of ways to make omitting the
> flag crashier?

We could cx_->runtime()->gc.gcIfRequested() in ~AutoSetNewObjectMetadata, which should surface some of these things a little earlier. Well, actually nvm -- the issue is /missing/ an AutoSetNewObjectMetadata so changing the dtor wouldn't help catch more of this bug because the changed code still wouldn't get called.

I guess we could add a new metadata callback for the fuzzers that plays with the object being created, and hopefully trips some existing I-had-better-be-initialized assertions? Enumerate an object's properties in the metadata callback?

Add an initialized() method to JSObject and assert that in the metadata callback? Although, then we would have to add a whole lot of overrides and remember to add them in the future -- same problem as remembering to add the JSCLASS_DELAY_METADATA_CALLBACK flag.
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #42)
> We should probably do a manual census of FooObject::Create methods.

Filed bug 1245539
Another possibility would be, in DEBUG builds, to have the metadata hook call JS::TraceChildren. I think that would have caught this bug (I can check)
Current patch stack fails CGC builds:
js/src/jit-test/tests/debug/Memory-tenurePromotionsLog-01.js

I'm working on this.
Okay, so the Memory-tenurePromotionsLog-01 failure is kind of interesting.

We allocate a Date object. We decide to attach allocation metadata for it. We allocate a stack frame object. That allocation triggers a GC which tenures the Date object. We log that tenuring. At the time we log the tenuring, we have not yet attached metadata to the Date object, because we haven't finished allocating the stack frame yet. The tenure log entry for the Data is created with a null allocation site stack.

The test tries to use the stack to pair up objects we expect to have been tenured with tenure log entries. Since the tenure log entry for the Date object has no stack, the test fails.

This is a pretty fundamental problem in the way we've structured things. The tenure log can observe objects at surprising times - in this case, before they've had their allocation metadata attached.

Nick says we're not actually using the tenuring log for anything at the moment.
Depends on: 1247126
$ JS_GC_ZEAL=14 python ../js/src/jit-test/jit_test.py --show-cmd js/src/js testBug1236552
/home/jimb/moz/dbg/obj-spider/js/src/js -f /home/jimb/moz/dbg/js/src/jit-test/lib/prologue.js --js-cache /home/jimb/moz/dbg/js/src/jit-test/.js-cache -e 'const platform='"'"'linux2'"'"'; const libdir='"'"'/home/jimb/moz/dbg/js/src/jit-test/lib/'"'"'; const scriptdir='"'"'/home/jimb/moz/dbg/js/src/jit-test/tests/asm.js/'"'"'' -f /home/jimb/moz/dbg/js/src/jit-test/tests/asm.js/testBug1236552.js
Assertion failure: !mEntered, at /home/jimb/moz/dbg/obj-spider/dist/include/mozilla/ReentrancyGuard.h:39
Exit code: -11
FAIL - asm.js/testBug1236552.js
FAILURES:
    /home/jimb/moz/dbg/js/src/jit-test/tests/asm.js/testBug1236552.js
TIMEOUTS:
$
The bug here is that js::Debugger::appendAllocationSite can cause a GC (via the call to JSCompartment::wrap), but js::Debugger::slowPathOnLogAllocationSite doesn't prevent debuggers from being GC'd (nor protect the DebuggerVector it's iterating over). So the `wrap` call may cause a GC which frees the Debugger whose allocationsLog we then try to enqueue data to.
One failure remaining: this one times out:

$ JS_GC_ZEAL=14 python ../js/src/jit-test/jit_test.py --show-cmd js/src/js bug-1006876-too-much-recursion.js
/home/jimb/moz/dbg/obj-spider/js/src/js -f /home/jimb/moz/dbg/js/src/jit-test/lib/prologue.js --js-cache /home/jimb/moz/dbg/js/src/jit-test/.js-cache -e 'const platform='"'"'linux2'"'"'; const libdir='"'"'/home/jimb/moz/dbg/js/src/jit-test/lib/'"'"'; const scriptdir='"'"'/home/jimb/moz/dbg/js/src/jit-test/tests/saved-stacks/'"'"'' -f /home/jimb/moz/dbg/js/src/jit-test/tests/saved-stacks/bug-1006876-too-much-recursion.js
^CTEST-UNEXPECTED-FAIL | jit_test.py : Test execution interrupted by user
PASSED ALL (partial run -- interrupted by user before starting)

It used to be the case that capturing a stack could not cause garbage collection, but now the JS_GC_ZEAL settings can affect the capture of the stack for the error. So we're trying to do 64722 compacting garbage collections.
(In reply to Jim Blandy :jimb from comment #49)
> One failure remaining: this one times out:
> 
> $ JS_GC_ZEAL=14 python ../js/src/jit-test/jit_test.py --show-cmd js/src/js
> bug-1006876-too-much-recursion.js
> /home/jimb/moz/dbg/obj-spider/js/src/js -f
> /home/jimb/moz/dbg/js/src/jit-test/lib/prologue.js --js-cache
> /home/jimb/moz/dbg/js/src/jit-test/.js-cache -e 'const
> platform='"'"'linux2'"'"'; const
> libdir='"'"'/home/jimb/moz/dbg/js/src/jit-test/lib/'"'"'; const
> scriptdir='"'"'/home/jimb/moz/dbg/js/src/jit-test/tests/saved-stacks/'"'"''
> -f
> /home/jimb/moz/dbg/js/src/jit-test/tests/saved-stacks/bug-1006876-too-much-
> recursion.js
> ^CTEST-UNEXPECTED-FAIL | jit_test.py : Test execution interrupted by user
> PASSED ALL (partial run -- interrupted by user before starting)
> 
> It used to be the case that capturing a stack could not cause garbage
> collection, but now the JS_GC_ZEAL settings can affect the capture of the
> stack for the error. So we're trying to do 64722 compacting garbage
> collections.

https://dxr.mozilla.org/mozilla-central/source/js/src/devtools/automation/cgc-jittest-timeouts.txt
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #50)
> https://dxr.mozilla.org/mozilla-central/source/js/src/devtools/automation/
> cgc-jittest-timeouts.txt

PERFECT

I've added the test above to the list.
Here are a few more changes I had to add.
Attachment #8721557 - Flags: review?(nfitzgerald)
Comment on attachment 8721557 [details] [diff] [review]
Additional changes to fix various SM(cgc) failures

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

Thanks!

r=me with an investigation into how feasible it is to take a `Handle<GCVector<Debugger*>>` param.

::: js/src/vm/Debugger.cpp
@@ +1734,5 @@
> +    Rooted<GCVector<JSObject*>> activeDebuggers(cx, GCVector<JSObject*>(cx));
> +    for (Debugger** dbgp = dbgs.begin(); dbgp < dbgs.end(); dbgp++) {
> +        if (!activeDebuggers.append((*dbgp)->object))
> +            return false;
> +    }

This totally makes sense but is sort of gross in the sense that the use of the Debuggers and the rooting of them are relatively separate. Ideally, we would want callers to prove to us that they have rooted the debuggers, the same way we do throughout the rest of SpiderMonkey with HandleObject/etc parameters.

Good news: I think we can do this these days by taking a `Handle<GCVector<Debugger>>` as a parameter for this method instead of a `GlobalObject::DebuggerVector&` (also, that definition feels like it should be in Debugger.h and not under GlobalObject's namespace). I *think* that this approach should mostly Just Work, since we already have the appropriate `js::Debugger::trace` method, although we may have to do some small boilerplate for `GCPolicy<Debugger*>` since it is `GCVector<Debugger*>` not `GCVector<Debugger>`. We may also need to add an `explicit GCVector<T>(const Vector<T>&)` constructor.

If this is too much ocean-boiling, or too many hoops to jump through, then I'm fine landing this patch as-is (perhaps with a follow up to clean this up sometime down the line?)
Attachment #8721557 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #55)
> Good news: I think we can do this these days by taking a
> `Handle<GCVector<Debugger>>` as a parameter

That should be `Handle<GCVector<Debugger*>>`; note the "*" that was missing.
oh what a glorious try push
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #55)
> This totally makes sense but is sort of gross in the sense that the use of
> the Debuggers and the rooting of them are relatively separate. Ideally, we
> would want callers to prove to us that they have rooted the debuggers, the
> same way we do throughout the rest of SpiderMonkey with HandleObject/etc
> parameters.

That is a totally great observation. I had lost myself in the details. I'll give it a shot.
Per conversation on IRC, we decided that rooting the Debuggers in slowPathOnLogAllocationSite is actually the right thing to do, since the caller has only a weak reference to them.
Revision of js::Debugger::slowPathOnLogAllocationSite Debugger rooting patch.
Flags: in-testsuite+
Target Milestone: --- → mozilla47
Depends on: 1252154
You need to log in before you can comment on or make changes to this bug.