Closed Bug 1253461 Opened 4 years ago Closed 4 years ago

TSan: data race under compacting GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: terrence, Assigned: jonco)

Details

Attachments

(1 file, 1 obsolete file)

Just saw this when running jsapi-tests. Don't have time to look into it right this second.

testGCUID
==================
WARNING: ThreadSanitizer: data race (pid=30750)
  Write of size 8 at 0x7fb404f66018 by thread T11:
    #0 getNextArenaToUpdateAndUnlink /home/terrence/moz/branch/e/js/src/gc/Heap.h:653 (jsapi-tests+0x000000b5e58a)
    #1 updateArenas /home/terrence/moz/branch/e/js/src/jsgc.cpp:2638 (jsapi-tests+0x000000b5e58a)
    #2 run /home/terrence/moz/branch/e/js/src/jsgc.cpp:2650 (jsapi-tests+0x000000b5e627)
    #3 runFromHelperThread /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1083 (jsapi-tests+0x000000ccbbfb)
    #4 handleGCParallelWorkload /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1106 (jsapi-tests+0x000000cccd2e)
    #5 threadLoop /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1738 (jsapi-tests+0x000000cccd2e)
    #6 js::HelperThread::ThreadMain(void*) /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1345 (jsapi-tests+0x000000cca236)
    #7 ThreadRoutine /home/terrence/moz/branch/e/js/src/vm/PosixNSPR.cpp:45 (jsapi-tests+0x000000dc8a77)

  Previous read of size 8 at 0x7fb404f66018 by main thread:
    #0 getAllocKind /home/terrence/moz/branch/e/js/src/gc/Heap.h:550 (jsapi-tests+0x000000be24c8)
    #1 getAllocKind /home/terrence/moz/branch/e/js/src/gc/Heap.h:1227 (jsapi-tests+0x000000be24c8)
    #2 fixupDictionaryShapeAfterMovingGC /home/terrence/moz/branch/e/js/src/jspropertytree.cpp:242 (jsapi-tests+0x000000be24c8)
    #3 fixupAfterMovingGC /home/terrence/moz/branch/e/js/src/jspropertytree.cpp:307 (jsapi-tests+0x000000be24c8)
    #4 UpdateCellPointersTyped<js::Shape> /home/terrence/moz/branch/e/js/src/jsgc.cpp:2429 (jsapi-tests+0x000000b5e36c)
    #5 UpdateCellPointers /home/terrence/moz/branch/e/js/src/jsgc.cpp:2467 (jsapi-tests+0x000000b5e36c)
    #6 updateArenas /home/terrence/moz/branch/e/js/src/jsgc.cpp:2640 (jsapi-tests+0x000000b5e36c)
    #7 run /home/terrence/moz/branch/e/js/src/jsgc.cpp:2650 (jsapi-tests+0x000000b5e627)
    #8 js::GCParallelTask::runFromMainThread(JSRuntime*) /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1071 (jsapi-tests+0x000000ccbb79)
    #9 updateAllCellPointersParallel /home/terrence/moz/branch/e/js/src/jsgc.cpp:2689 (jsapi-tests+0x000000b5ed01)
    #10 updatePointersToRelocatedCells /home/terrence/moz/branch/e/js/src/jsgc.cpp:2774 (jsapi-tests+0x000000b5f612)
    #11 compactPhase /home/terrence/moz/branch/e/js/src/jsgc.cpp:5675 (jsapi-tests+0x000000b720a7)
    #12 incrementalCollectSlice /home/terrence/moz/branch/e/js/src/jsgc.cpp:6143 (jsapi-tests+0x000000b73dc7)
    #13 gcCycle /home/terrence/moz/branch/e/js/src/jsgc.cpp:6318 (jsapi-tests+0x000000b74836)
    #14 collect /home/terrence/moz/branch/e/js/src/jsgc.cpp:6424 (jsapi-tests+0x000000b75212)
    #15 gc /home/terrence/moz/branch/e/js/src/jsgc.cpp:6482 (jsapi-tests+0x000000b78e85)
    #16 GCForReason /home/terrence/moz/branch/e/js/src/jsgc.cpp:7385 (jsapi-tests+0x000000b78e85)
    #17 run /home/terrence/moz/branch/e/js/src/jsapi-tests/testGCUniqueId.cpp:114 (jsapi-tests+0x000000588d2e)
    #18 main /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.cpp:125 (jsapi-tests+0x00000064d206)

  Thread T11 'JS Helper' (tid=30762, running) created by main thread at:
    #0 pthread_create /home/terrence/moz/clang/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:954 (jsapi-tests+0x0000004606b6)
    #1 PR_CreateThread /home/terrence/moz/branch/e/js/src/vm/PosixNSPR.cpp:108 (jsapi-tests+0x000000dc8bc2)
    #2 ensureInitialized /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:607 (jsapi-tests+0x000000cc633c)
    #3 js::EnsureHelperThreadsInitialized() /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:59 (jsapi-tests+0x000000cc6212)
    #4 init /home/terrence/moz/branch/e/js/src/vm/Runtime.cpp:291 (jsapi-tests+0x000000dd9a68)
    #5 JS_NewRuntime /home/terrence/moz/branch/e/js/src/jsapi.cpp:471 (jsapi-tests+0x000000b0d8e6)
    #6 createRuntime /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.h:283 (jsapi-tests+0x0000004e29e1)
    #7 init /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.cpp:18 (jsapi-tests+0x00000064bd88)
    #8 main /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.cpp:118 (jsapi-tests+0x00000064d1e3)

SUMMARY: ThreadSanitizer: data race /home/terrence/moz/branch/e/js/src/gc/Heap.h:653 in getNextArenaToUpdateAndUnlink
==================
TEST-PASS | testGCUID | ok
Flags: needinfo?(jcoppeard)
This is because fixupDictionaryShapeAfterMovingGC is looking at the AllocKind in the arena header while a background thread is updating the linked list in auxNextLink.  These are separate fields but are all packed into the first 8 bytes of the arena header.

I don't think this is a 'real' race since the write will never change the alloc kind, but if we can do something less unpleasant in fixupDictionaryShapeAfterMovingGC that would be a win.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attached patch bug1253461-fix-shape-race (obsolete) — Splinter Review
Pending try results...
Try run is green.  This patch removes the checking the referent's arena header to tell what type of thing listp points to because we can tell by checking whether the base shape is owned.  As explained in the comments in Shape.h, the last shape in a dictionary mode list always has an owned base shape.

I kept the arena check in debug mode so we can assert that this is correct, but we could probably throw it away entirely.
Attachment #8727942 - Attachment is obsolete: true
Attachment #8728347 - Flags: review?(terrence)
Comment on attachment 8728347 [details] [diff] [review]
bug1253461-fix-shape-race v2

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

Sounds good to me. If we ever get our TSAN warnings down to near zero we can decide what to do to suppress this one in debug too.

::: js/src/jspropertytree.cpp
@@ +236,5 @@
>  
> +    // The listp field either points to the parent field of the next shape in
> +    // the list if there is one.  Otherwise if this shape is the last in the
> +    // list then it points to the shape_ field of the object the list is for.
> +    // We can tell which it is because the base shape is owend if this is the

owend => owned

@@ +238,5 @@
> +    // the list if there is one.  Otherwise if this shape is the last in the
> +    // list then it points to the shape_ field of the object the list is for.
> +    // We can tell which it is because the base shape is owend if this is the
> +    // last property and not otherwise.
> +    bool listpPointsIntoShape = !base()->isOwned();

\o/
Attachment #8728347 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/e9aba4255d34
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.