Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 6 years ago

The following testcase crashes on mozilla-central revision 4303d49c5393 (build with --32 --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/ion/inlining/TypedObject-TypeDescrIsSimpleType.js
for (var i = 0; i < 99; i++) {
	    w = new TypedObject.ArrayType(TypedObject.int32, 100).build(function() {});
}
// jsfunfuzz-generated
relazifyFunctions();

Backtrace:

#0  0x56d48f16 in js::NativeObject::fixedData (this=0xf6a77070, nslots=4) at js/src/vm/NativeObject-inl.h:36
#1  0x56d2f2f0 in js::ArrayBufferObject::inlineDataPointer (this=0xf6a77070) at js/src/vm/ArrayBufferObject.cpp:952
#2  0x56b55359 in js::ArrayBufferObject::hasInlineData (this=0xf6a77070) at js/src/vm/ArrayBufferObject.h:372
#3  js::OutlineTypedObject::obj_trace (trc=0xffee093c, object=0xf6a90020) at js/src/builtin/TypedObject.cpp:1628
#4  0x56e2190b in js::Class::doTrace (this=<optimized out>, obj=0xf6a90020, trc=0xffee093c) at /home/ubuntu/shell-cache/js-dbg-32-linux-4303d49c5393/objdir-js/dist/include/js/Class.h:869
/snip

For detailed crash information, see attachment.

ArrayBuffers are on the stack, setting s-s as a start.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 2 • 6 years ago

This was compiled with:

'sh' '/home/ubuntu/trees/mozilla-central/js/src/configure' '--target=i686-pc-linux' '--enable-debug' '--with-ccache' '--enable-gczeal' '--enable-debug-symbols' '--disable-tests'

python -u -m funfuzz.js.compile_shell -b "--32 --enable-debug" -r 4303d49c5393

	Fuzzing Team
	Comment 3 • 6 years ago

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 4 • 6 years ago

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9b7cc103ce95
user:        Jon Coppeard
date:        Thu May 10 10:09:27 2018 +0100
summary:     Bug 1457703 - Don't fixup an associated object's shape when updating moved pointers in another object r=sfink a=abillings

Jon, is bug 1457703 a likely regressor?

	Jon Coppeard (:jonco) Assignee
	Comment 5 • 6 years ago

Yes, it is.

	Jon Coppeard (:jonco) Assignee
	Comment 6 • 6 years ago

Fix another place we can call numFixedSlots() on an associated object while traching during a moving GC.

	Steve Fink [:sfink] [:s:]
	Comment 7 • 6 years ago

Comment on attachment 8975440 [details] [diff] [review]
bug1461027-fixed-data

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

I used my callgraph traversal to look for more instances. Here's one:

  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
  #15471 = void* js::NativeObject::getPrivate() const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

similarly

  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
  #15475 = void js::NativeObject::setPrivateUnbarriered(void*)
  #15390 = uint32 js::NativeObject::numFixedSlots() const

and while we're there,

  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
  #15476 = void js::NativeObject::initPrivate(void*)
  #15390 = uint32 js::NativeObject::numFixedSlots() const

Then there's

  #165462 = void js::ScriptSourceObject::trace(JSTracer*, JSObject*)
  #15434 = JS::Value* js::NativeObject::getReservedSlot(uint32) const
  #15418 = JS::Value* js::NativeObject::getSlot(uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

paired with

  #178194 = void js::ArrayBufferObject::trace(JSTracer*, JSObject*)
  #15427 = void js::NativeObject::setSlot(uint32, JS::Value*)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

Hopping over to RegExps:

  #244301 = void js::RegExpObject::trace(JSTracer*)
  #18595 = js::ReadBarriered<js::RegExpShared*>& js::RegExpObject::sharedRef()
  #15468 = void** js::NativeObject::privateRef(uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

I hope the flow makes this one safe, but I didn't check:

  #307657 = void js::gc::StoreBuffer::SlotsEdge::trace(js::TenuringTracer*) const
  #307659 = void js::TenuringTracer::traceObjectSlots(js::NativeObject*, uint32, uint32)
  #15392 = void js::NativeObject::getSlotRange(uint32, uint32, js::HeapSlot**, js::HeapSlot**, js::HeapSlot**, js::HeapSlot**)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

I'll skip things similar to that. I'm not sure what to make of this:

  #33538 = void NoteWeakMapsTracer::trace(JSObject*, JS::GCCellPtr, JS::GCCellPtr)
  #307694 = void JS::TraceChildren(JSTracer*, JS::GCCellPtr)
  #105943 = void js::TraceChildren(JSTracer*, void*, int32)
  #313776 = void (TraceChildrenFunctor, int32, JSTracer**, void**)<JSObject>((Forward<Args>)(JS::DispatchTraceKindTyped::args)...)) JS::DispatchTraceKindTyped(F, JS::TraceKind, Args&& ...) [with F = TraceChildrenFunctor; Args = {JSTracer*&, void*&}; decltype (f.operator()<JSObject>((Forward<Args>)(JS::DispatchTraceKindTyped::args)...)) = void]
  #314678 = void TraceChildrenFunctor::operator(JSTracer*, void*)(JSTracer*, void*) [with T = JSObject]
  #162570 = void JSObject::traceChildren(JSTracer*)
  #15423 = js::HeapSlot* js::NativeObject::getSlotRef(uint32)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

Probably nothing or impossible:

  #169217 = void JS::GCHashSet<T, HashPolicy, AllocPolicy>::trace(JSTracer*) [with T = jsid; HashPolicy = js::DefaultHasher<jsid>; AllocPolicy = js::TempAllocPolicy]
  #169608 = js::detail::HashTable<T, HashPolicy, AllocPolicy>::Enum::~Enum() [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #169801 = js::detail::HashTable<T, HashPolicy, AllocPolicy>::Enum::~Enum() [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #169803 = void js::detail::HashTable<T, HashPolicy, AllocPolicy>::compactIfUnderloaded() [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #168049 = js::detail::HashTable<T, HashPolicy, AllocPolicy>::RebuildStatus js::detail::HashTable<T, HashPolicy, AllocPolicy>::changeTableSize(int, js::detail::HashTable<T, HashPolicy, AllocPolicy>::FailureBehavior) [with T = const jsid; HashPolicy = js::HashSet<jsid, js::DefaultHasher<jsid>, js::TempAllocPolicy>::SetOps; AllocPolicy = js::TempAllocPolicy]
  #13224 = void js::TempAllocPolicy::reportAllocOverflow() const
  #17966 = void js::ReportAllocationOverflow(JSContext*)
  (IN LIMITED 1) #29390 = void JS_ReportErrorNumberASCII(JSContext*, (JSErrorFormatString*)(void*,uint32)*, void*, uint32)
  #153327 = void JS_ReportErrorNumberASCIIVA(JSContext*, (JSErrorFormatString*)(void*,uint32)*, void*, uint32, __va_list_tag*)
  #153328 = uint8 js::ReportErrorNumberVA(JSContext*, uint32, (JSErrorFormatString*)(void*,uint32)*, void*, uint32, uint32, __va_list_tag*)
  #154214 = jscntxt.cpp:void ReportError(JSContext*, JSErrorReport*, (JSErrorFormatString*)(void*,uint32)*, void*)
  #154216 = void js::ErrorToException(JSContext*, JSErrorReport*, (JSErrorFormatString*)(void*,uint32)*, void*)
  #155890 = js::ErrorObject* js::ErrorObject::create(JSContext*, uint32, class JS::Handle<JSObject*>, class JS::Handle<JSString*>, uint32, uint32, struct js::ScopedJSFreePtr<JSErrorReport>*, class JS::Handle<JSString*>, class JS::Handle<JSObject*>)
  #239297 = uint8 js::ErrorObject::init(JSContext*, class JS::Handle<js::ErrorObject*>, uint32, struct js::ScopedJSFreePtr<JSErrorReport>*, class JS::Handle<JSString*>, class JS::Handle<JSObject*>, uint32, uint32, class JS::Handle<JSString*>)
  #15437 = void js::NativeObject::initReservedSlot(uint32, JS::Value*)
  #15428 = void js::NativeObject::initSlot(uint32, JS::Value*)
  #15393 = uint8 js::NativeObject::slotInRange(uint32, uint32) const
  #15390 = uint32 js::NativeObject::numFixedSlots() const

It doesn't report anything more if I suppress things resembling any of the above.

	Jon Coppeard (:jonco) Assignee
	Comment 8 • 6 years ago

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #7)
> I used my callgraph traversal to look for more instances. Here's one:

This looks really useful, is this available publicly somehow?

>  #178229 = void js::ArrayBufferViewObject::trace(JSTracer*, JSObject*)
>  #15471 = void* js::NativeObject::getPrivate() const
>  #15390 = uint32 js::NativeObject::numFixedSlots() const

It's OK to call numFixedSlots() on the object that's being traced, because it's already had its shape updated.  The problem comes when doing this for some other object (e.g. a view of an array buffer being traced).  So I think all of these are safe.

>   #165462 = void js::ScriptSourceObject::trace(JSTracer*, JSObject*)

This doesn't seem to exist any more.

>  #169217 = void JS::GCHashSet<T, HashPolicy, AllocPolicy>::trace(JSTracer*) [with T = jsid; HashPolicy = js::DefaultHasher<jsid>; AllocPolicy = js::TempAllocPolicy]

Hmm, hopefully that's impossible.  Oh, we should be using Range rather than Enum, which doesn't have this possibility.

	Steve Fink [:sfink] [:s:]
	Comment 9 • 6 years ago

Comment on attachment 8975440 [details] [diff] [review]
bug1461027-fixed-data

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

Oh right, that makes sense. I wonder if we could stick the object being traced into the JSTracer if DEBUG, so we could assert in numFixedSlots().

re: the callgraph tool, I thought I had sent you instructions for using it at some point in the past when I was going on PTO? I can look for it.

But quick instructions:
1. grab the script at https://bitbucket.org/sfink/sfink-tools/raw/default/bin/traverse.py?at=default&fileviewer=file-view-default
2. find a recent hazard build. It'll have a build artifact named callgraph.txt. (The one I used here was pretty old, as you noticed from the missing ScriptSourceObject::trace). You'll need to uncompress it.
3. python traverse.py callgraph.txt
4. run 'help'. Many of the commands are kind of cryptic. Some of them don't work since I keep changing stuff and I don't have tests. It'll take a while to load in the full callgraph (you can do a -p linux64-shell-haz push to get a much smaller shell-only one, but it's not *that* bad.)

I have a Rust version that loads the callgraph in < 1sec, but I haven't implemented any of the traversals for it yet.

As you point out, though, it's not really the right tool for this particular problem since it can't distinguish which object you're looking at. Though come to think of it, I wonder how huge it would get if I recorded every field access  in every method, so you could ask questions like "are all accesses to SomeClass.somefield done with this RAII lock held?" or simply "where are all the places we directly access this struct's field?" Hmm...

	Jon Coppeard (:jonco) Assignee
	Comment 10 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b176c63fc888b86d85ed5767802b433f7a8486

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/c1b176c63fc8