828947 - bitfield race in struct nsTArrayHeader

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Julian Seward [:jseward]]

xpcom/glue/nsTArray.h defines nsTArrayHeader thusly:

  struct NS_COM_GLUE nsTArrayHeader
  {
    static nsTArrayHeader sEmptyHdr;
    uint32_t mLength;
    uint32_t mCapacity : 31;
    uint32_t mIsAutoArray : 1;
  };

The fields of concern are mCapacity and mIsAutoArray.  Because they
are bitfields, compilers cannot generate code which updates the fields
independently.  A thread assigning to obj->mCapacity may trash a
concurrent assignment to obj->mIsAutoArray done by a different thread,
and vice versa.  The C++11 standard makes this limitation explicit.

Comment 1

[Julian Seward [:jseward]]

Helgrind reports many conflicts between assignments of mCapacity and
mIsAutoArray as a result.  One such is shown below.  If I change
nsTArrayHeader to this

  struct NS_COM_GLUE nsTArrayHeader
  {
    static nsTArrayHeader sEmptyHdr;
    uint32_t mLength;
    uint32_t mCapacity;
    uint32_t mIsAutoArray;
    uint32_t __pad;
  };

then all such race reports disappear.


Possible data race during read of size 1 at 0x951090F by thread #1
Locks held: none
   at 0x6BBF29C: nsTArray_base<nsTArrayInfallibleAllocator>::EnsureCapacity(unsigned int, unsigned int) (nsTArray-inl.h:103)
   by 0x7C4D07A: nsObserverList::AddObserver(nsIObserver*, bool) (nsTArray.h:959)
   by 0x7C4D876: nsObserverService::AddObserver(nsIObserver*, char const*, bool) (nsObserverService.cpp:114)
   by 0x7C8B87E: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:164)
   by 0x766BA98: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3085)
   by 0x7672B54: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
   by 0x82C1866: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:373)
   by 0x82B38ED: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2368)
   by 0x82C0957: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:348)
   by 0x82C1A04: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:406)
   by 0x82C1FE9: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:112)
   by 0x82FC85C: js::BaseProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:266)

This conflicts with a previous write of size 1 by thread #12
Locks held: none
   at 0x6C2E475: nsTArray_base<nsTArrayInfallibleAllocator>::IsAutoArrayRestorer::~IsAutoArrayRestorer() (nsTArray-inl.h:297)
   by 0x6C2E543: bool nsTArray_base<nsTArrayInfallibleAllocator>::SwapArrayElements<nsTArrayInfallibleAllocator>(nsTArray_base<nsTArrayInfallibleAllocator>&, unsigned int, unsigned long) (nsTArray-inl.h:390)
   by 0x737C165: (anonymous namespace)::LoadAllScripts(JSContext*, mozilla::dom::workers::WorkerPrivate*, nsTArray<(anonymous namespace)::ScriptLoadInfo>&, bool) (nsTArray.h:1086)
   by 0x737C5B9: mozilla::dom::workers::scriptloader::LoadWorkerScript(JSContext*) (ScriptLoader.cpp:708)
   by 0x737DFF8: (anonymous namespace)::CompileScriptRunnable::WorkerRun(JSContext*, mozilla::dom::workers::WorkerPrivate*) (WorkerPrivate.cpp:704)
   by 0x738081E: mozilla::dom::workers::WorkerRunnable::Run() (WorkerPrivate.cpp:1693)
   by 0x73869BB: mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext*) (WorkerPrivate.cpp:2675)
   by 0x737757F: (anonymous namespace)::WorkerThreadRunnable::Run() (RuntimeService.cpp:503)

Comment 2

[Justin Lebar (not reading bugmail)]

mIsAutoArray tells us whether the header's TArray has an auto-array, not whether we're currently using that space.  So mIsAutoArray is set when we construct the header and changes only when the header is swapped with another array.

ISTM that we're either using nsTArray safely (i.e. only from one thread) or not.  Since nsTArray isn't threadsafe, the bitfield doesn't seem to add additional unsafety.  But perhaps we get into weird cases when swapping arrays between threads without a memory barrier.

bent and I don't make much sense out of the stack in comment 1 -- how is the worker's tarray supposed to get into nsObserverList?

Comment 3

[Julian Seward [:jseward]]

This invalid.  Thanks to jlebar for helping me figure out what's going
on.

What we now know is, there are unsynchronised accesses, but only to
sEmptyHdr, and only in very special circumstances, that (providing
nsTArray is working correctly) don't change sEmptyHeader:

* IncrementLength: adds zero to sEmptyHdr::mLength

* ~IsAutoArrayRestorer: sets sEmptyHdr::mIsAutoArray to zero, which
  is what it should always be anyway.