Bug 1480998 - Baldr: fix race in wasm signal handler if browser crashes during shutdown (r=lth,bbouvier)
authorLuke Wagner <luke@mozilla.com>
Thu, 23 Aug 2018 11:23:52 -0500
changeset 433143 e4a245dc212abd00595bb4e4489096c6daefc73c
parent 433142 5ff39f587a0de8a75ab92bc16b58c759cffdb42c
child 433144 49b70f7e6817131d79876ab88667bc6c2833e4a0
child 433194 62bf59351409430561fb8f8fcef08bdb1a7208cd
push id34499
push usercsabou@mozilla.com
push dateThu, 23 Aug 2018 21:40:51 +0000
treeherdermozilla-central@49b70f7e6817 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslth, bbouvier
bugs1480998
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1480998 - Baldr: fix race in wasm signal handler if browser crashes during shutdown (r=lth,bbouvier)
js/src/wasm/WasmProcess.cpp
--- a/js/src/wasm/WasmProcess.cpp
+++ b/js/src/wasm/WasmProcess.cpp
@@ -38,37 +38,38 @@ using mozilla::BinarySearchIf;
 // wasm compilation may be tiered, and the second tier doesn't have access to
 // any JSContext/JS::Compartment/etc lying around, we have to use a process-wide
 // map instead.
 
 typedef Vector<const CodeSegment*, 0, SystemAllocPolicy> CodeSegmentVector;
 
 Atomic<bool> wasm::CodeExists(false);
 
+// Because of profiling, the thread running wasm might need to know to which
+// CodeSegment the current PC belongs, during a call to lookup(). A lookup
+// is a read-only operation, and we don't want to take a lock then
+// (otherwise, we could have a deadlock situation if an async lookup
+// happened on a given thread that was holding mutatorsMutex_ while getting
+// sampled). Since the writer could be modifying the data that is getting
+// looked up, the writer functions use spin-locks to know if there are any
+// observers (i.e. calls to lookup()) of the atomic data.
+
+static Atomic<size_t> sNumObservers(0);
+static Atomic<bool> sShuttingDown(false);
+
 class ProcessCodeSegmentMap
 {
     // Since writes (insertions or removals) can happen on any background
     // thread at the same time, we need a lock here.
 
     Mutex mutatorsMutex_;
 
     CodeSegmentVector segments1_;
     CodeSegmentVector segments2_;
 
-    // Because of profiling, the thread running wasm might need to know to which
-    // CodeSegment the current PC belongs, during a call to lookup(). A lookup
-    // is a read-only operation, and we don't want to take a lock then
-    // (otherwise, we could have a deadlock situation if an async lookup
-    // happened on a given thread that was holding mutatorsMutex_ while getting
-    // sampled). Since the writer could be modifying the data that is getting
-    // looked up, the writer functions use spin-locks to know if there are any
-    // observers (i.e. calls to lookup()) of the atomic data.
-
-    Atomic<size_t> observers_;
-
     // Except during swapAndWait(), there are no lookup() observers of the
     // vector pointed to by mutableCodeSegments_
 
     CodeSegmentVector* mutableCodeSegments_;
     Atomic<const CodeSegmentVector*> readonlyCodeSegments_;
 
     struct CodeSegmentPC
     {
@@ -79,17 +80,17 @@ class ProcessCodeSegmentMap
                 return 0;
             if (pc < cs->base())
                 return -1;
             return 1;
         }
     };
 
     void swapAndWait() {
-        // Both vectors are consistent for look up at this point, although their
+        // Both vectors are consistent for lookup at this point although their
         // contents are different: there is no way for the looked up PC to be
         // in the code segment that is getting registered, because the code
         // segment is not even fully created yet.
 
         // If a lookup happens before this instruction, then the
         // soon-to-become-former read-only pointer is used during the lookup,
         // which is valid.
 
@@ -105,23 +106,22 @@ class ProcessCodeSegmentMap
         // - in case of removal, it means the new vector contains one less
         // entry, but it's fine since unregistering means the code segment
         // isn't used by any live instance anymore, thus PC can't be in the
         // to-be-removed code segment's range.
 
         // A lookup could have happened on any of the two vectors. Wait for
         // observers to be done using any vector before mutating.
 
-        while (observers_);
+        while (sNumObservers > 0) {}
     }
 
   public:
     ProcessCodeSegmentMap()
       : mutatorsMutex_(mutexid::WasmCodeSegmentMap),
-        observers_(0),
         mutableCodeSegments_(&segments1_),
         readonlyCodeSegments_(&segments2_)
     {
     }
 
     ~ProcessCodeSegmentMap()
     {
         MOZ_ASSERT(segments1_.empty());
@@ -187,65 +187,79 @@ class ProcessCodeSegmentMap
                                        CodeSegmentPC(cs->base()), &otherIndex));
         MOZ_ASSERT(index == otherIndex);
 #endif
 
         mutableCodeSegments_->erase(mutableCodeSegments_->begin() + index);
     }
 
     const CodeSegment* lookup(const void* pc) {
-        auto decObserver = mozilla::MakeScopeExit([&] {
-            observers_--;
-        });
-        observers_++;
-
-        // Once atomically-read, the readonly vector is valid as long as
-        // observers_ has been incremented (see swapAndWait()).
         const CodeSegmentVector* readonly = readonlyCodeSegments_;
 
         size_t index;
         if (!BinarySearchIf(*readonly, 0, readonly->length(), CodeSegmentPC(pc), &index))
             return nullptr;
 
         // It is fine returning a raw CodeSegment*, because we assume we are
         // looking up a live PC in code which is on the stack, keeping the
         // CodeSegment alive.
 
         return (*readonly)[index];
     }
 };
 
-static ProcessCodeSegmentMap processCodeSegmentMap;
+static ProcessCodeSegmentMap sProcessCodeSegmentMap;
 
 bool
 wasm::RegisterCodeSegment(const CodeSegment* cs)
 {
     MOZ_ASSERT(cs->codeTier().code().initialized());
-    return processCodeSegmentMap.insert(cs);
+    return sProcessCodeSegmentMap.insert(cs);
 }
 
 void
 wasm::UnregisterCodeSegment(const CodeSegment* cs)
 {
-    processCodeSegmentMap.remove(cs);
+    sProcessCodeSegmentMap.remove(cs);
 }
 
 const CodeSegment*
 wasm::LookupCodeSegment(const void* pc, const CodeRange** codeRange /*= nullptr */)
 {
-    if (const CodeSegment* found = processCodeSegmentMap.lookup(pc)) {
+    // Avoid accessing an uninitialized sProcessCodeSegmentMap if there is a
+    // crash early in startup. Returning null will allow the crash to propagate
+    // properly to breakpad.
+    if (!CodeExists)
+        return nullptr;
+
+    // Ensure the observer count is above 0 throughout the entire lookup to
+    // ensure swapAndWait() waits for the lookup to complete.
+    auto decObserver = mozilla::MakeScopeExit([&] {
+        MOZ_ASSERT(sNumObservers > 0);
+        sNumObservers--;
+    });
+    sNumObservers++;
+
+    // Check sShuttingDown with sNumObservers > 0 to ensure the spinloop in
+    // wasm::ShutDown() is effective.
+    if (sShuttingDown)
+        return nullptr;
+
+    if (const CodeSegment* found = sProcessCodeSegmentMap.lookup(pc)) {
         if (codeRange) {
             *codeRange = found->isModule()
                        ? found->asModule()->lookupRange(pc)
                        : found->asLazyStub()->lookupRange(pc);
         }
         return found;
     }
+
     if (codeRange)
         *codeRange = nullptr;
+
     return nullptr;
 }
 
 const Code*
 wasm::LookupCode(const void* pc, const CodeRange** codeRange /* = nullptr */)
 {
     const CodeSegment* found = LookupCodeSegment(pc, codeRange);
     MOZ_ASSERT_IF(!found && codeRange, !*codeRange);
@@ -256,11 +270,15 @@ void
 wasm::ShutDown()
 {
     // If there are live runtimes then we are already pretty much leaking the
     // world, so to avoid spurious assertions (which are valid and valuable when
     // there are not live JSRuntimes), don't bother releasing anything here.
     if (JSRuntime::hasLiveRuntimes())
         return;
 
+    // After signalling shutdown, wait for currently-active observers to finish.
+    sShuttingDown = true;
+    while (sNumObservers > 0) {}
+
     ReleaseBuiltinThunks();
-    processCodeSegmentMap.freeAll();
+    sProcessCodeSegmentMap.freeAll();
 }