1305099 - TSan: data race in updateShapeAfterMovingGC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Via IRC, TSAN found the following race while updating pointers after compacting GC:

WARNING: ThreadSanitizer: data race (pid=27085)
  Read of size 8 at 0x7f85b0ee0cd8 by thread T8:
    #0 unbarrieredGet /home/sewardj/MOZ/MC-RACE/js/src/gc/Barrier.h:457 (libxul.so+0x00000619d0b7)
    #1 updateShapeAfterMovingGC /home/sewardj/MOZ/MC-RACE/js/src/vm/NativeObject-inl.h:292 (libxul.so+0x00000619d0b7)
    #2 MakeAccessibleAfterMovingGC /home/sewardj/MOZ/MC-RACE/js/src/jsgcinlines.h:27 (libxul.so+0x00000619d0b7)
    #3 MaybeForwarded<js::NativeObject *> /home/sewardj/MOZ/MC-RACE/js/src/jsgc.h:1170 (libxul.so+0x00000619d0b7)
    #4 fixupAfterMovingGC /home/sewardj/MOZ/MC-RACE/js/src/jsobj.cpp:1864 (libxul.so+0x00000619d0b7)
    #5 UpdateCellPointers<JSObject> /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2209 (libxul.so+0x000006154e9b)
    #6 UpdateArenaPointersTyped<JSObject> /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2218 (libxul.so+0x000006154e9b)
    #7 UpdateArenaPointers /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2234 (libxul.so+0x000006154e9b)
    #8 updateArenas /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2358 (libxul.so+0x000006154e9b)
    #9 js::gc::UpdatePointersTask::run() /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2365 (libxul.so+0x0000061558e7)
    #10 runFromHelperThread /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1068 (libxul.so+0x000006317a43)
    #11 handleGCParallelWorkload /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1099 (libxul.so+0x000006317c63)
    #12 threadLoop /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1783 (libxul.so+0x0000063195aa)
    #13 js::HelperThread::ThreadMain(void*) /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1320 (libxul.so+0x0000063160e6)
    #14 callMain<0> /home/sewardj/MOZ/MC-RACE/js/src/threading/Thread.h:242 (libxul.so+0x00000631dd1b)
    #15 Start /home/sewardj/MOZ/MC-RACE/js/src/threading/Thread.h:235 (libxul.so+0x00000631dd1b)

  Previous write of size 8 at 0x7f85b0ee0cd8 by thread T6:
    #0 unsafeSet /home/sewardj/MOZ/MC-RACE/js/src/gc/Barrier.h:369 (libxul.so+0x00000619d0de)
    #1 updateShapeAfterMovingGC /home/sewardj/MOZ/MC-RACE/js/src/vm/NativeObject-inl.h:294 (libxul.so+0x00000619d0de)
    #2 MakeAccessibleAfterMovingGC /home/sewardj/MOZ/MC-RACE/js/src/jsgcinlines.h:27 (libxul.so+0x00000619d0de)
    #3 MaybeForwarded<js::NativeObject *> /home/sewardj/MOZ/MC-RACE/js/src/jsgc.h:1170 (libxul.so+0x00000619d0de)
    #4 fixupAfterMovingGC /home/sewardj/MOZ/MC-RACE/js/src/jsobj.cpp:1864 (libxul.so+0x00000619d0de)
    #5 UpdateCellPointers<JSObject> /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2209 (libxul.so+0x000006154f7b)
    #6 UpdateArenaPointersTyped<JSObject> /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2218 (libxul.so+0x000006154f7b)
    #7 UpdateArenaPointers /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2234 (libxul.so+0x000006154f7b)
    #8 updateArenas /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2358 (libxul.so+0x000006154f7b)
    #9 js::gc::UpdatePointersTask::run() /home/sewardj/MOZ/MC-RACE/js/src/jsgc.cpp:2365 (libxul.so+0x0000061558e7)
    #10 runFromHelperThread /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1068 (libxul.so+0x000006317a43)
    #11 handleGCParallelWorkload /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1099 (libxul.so+0x000006317c63)
    #12 threadLoop /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1783 (libxul.so+0x0000063195aa)
    #13 js::HelperThread::ThreadMain(void*) /home/sewardj/MOZ/MC-RACE/js/src/vm/HelperThreads.cpp:1320 (libxul.so+0x0000063160e6)
    #14 callMain<0> /home/sewardj/MOZ/MC-RACE/js/src/threading/Thread.h:242 (libxul.so+0x00000631dd1b)
    #15 Start /home/sewardj/MOZ/MC-RACE/js/src/threading/Thread.h:235 (libxul.so+0x00000631dd1b)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1305099-object-fixup-race

This is a race while updating pointers after a compacting GC, which happens across multiple threads.  The problem is that JSObject::fixupAfterMovingGC can call MaybeForwarded on the owner objects of any copy-on-write elements, which can update the shape pointer of the object (in order to make it's slots accessible via normal methods).  This means that the shape pointer can be accessed by two different threads at the same time.

The fix is to not use MaybeForwarded in this case but just to IsForwarded/Forwarded.  We don't need the shape to be updated here just access to the object's elements pointer, which is not moved.

Comment 2

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

Comment on attachment 8795266 [details] [diff] [review]
bug1305099-object-fixup-race

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

What's the invariant here? fixupAfterMovingGC should never modify anything that might be fixed up on another thread? Or it shouldn't read such things either?

I guess this is kind of what's documented in js/Class.h before JSObjectMovedOp. Is that op called on helper threads?

Comment 3

[Jon Coppeard (:jonco)]

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

fixupAfterMovingGC is called on multiple helper threads and should never access anything that might be written by another thread.  JSObjectMovedOp is called on the main thread only.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa080b7daae9
Fix race updating COW elements pointer after compacting GC r=sfink

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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