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 831144 e4a245dc212abd00595bb4e4489096c6daefc73c
parent 831143 5ff39f587a0de8a75ab92bc16b58c759cffdb42c
child 831145 49b70f7e6817131d79876ab88667bc6c2833e4a0
push id118868
push userbmo:zjz@zjz.name
push dateFri, 24 Aug 2018 07:04:39 +0000
reviewerslth, bbouvier
bugs1480998
milestone63.0a1
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();
 }