From bc6acd5c5a1d7ee4b703fe9facfd8bf695b617d0 Mon Sep 17 00:00:00 2001
From: Mathias Agopian <mathias@google.com>
Date: Mon, 22 Feb 2021 23:24:31 -0800
Subject: [PATCH] GL backend: fix race condition when asserting a handle exists

---
 filament/backend/src/opengl/OpenGLDriver.cpp | 11 ++------
 filament/backend/src/opengl/OpenGLDriver.h   | 27 +++++++++++++++++---
 libs/utils/src/debug.cpp                     | 13 ++++++++--
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp
index 45d30d1593..bdee8845a4 100644
--- a/filament/backend/src/opengl/OpenGLDriver.cpp
+++ b/filament/backend/src/opengl/OpenGLDriver.cpp
@@ -361,9 +361,7 @@ template<typename D, typename ... ARGS>
 backend::Handle<D> OpenGLDriver::initHandle(ARGS&& ... args) noexcept {
     static_assert(sizeof(D) <= 208, "Handle<> too large");
     backend::Handle<D> h{ allocateHandle(sizeof(D)) };
-#ifndef NDEBUG
-    mHandleSet.insert(h.getId());
-#endif
+    registerHandleId(h.getId());
 
     D* addr = handle_cast<D *>(h);
 
@@ -396,9 +394,6 @@ template <typename B, typename D, typename>
 void OpenGLDriver::destruct(Handle<B>& handle, D const* p) noexcept {
     // allow to destroy the nullptr, similarly to operator delete
     if (p) {
-#ifndef NDEBUG
-        assert_invariant(mHandleSet.find(handle.getId()) != mHandleSet.end());
-#endif
 #if !defined(NDEBUG) && UTILS_HAS_RTTI
         if (UTILS_UNLIKELY(p->typeId != typeid(D).name())) {
             slog.e << "Destroying handle " << handle.getId() << ", type " << typeid(D).name()
@@ -409,9 +404,7 @@ void OpenGLDriver::destruct(Handle<B>& handle, D const* p) noexcept {
 #endif
         p->~D();
         mHandleArena.free(const_cast<D*>(p), sizeof(D));
-#ifndef NDEBUG
-        mHandleSet.erase(handle.getId());
-#endif
+        unregisterHandleId(handle.getId());
     }
 }
 
diff --git a/filament/backend/src/opengl/OpenGLDriver.h b/filament/backend/src/opengl/OpenGLDriver.h
index 8ab38f4517..7000374db9 100644
--- a/filament/backend/src/opengl/OpenGLDriver.h
+++ b/filament/backend/src/opengl/OpenGLDriver.h
@@ -254,11 +254,34 @@ private:
             utils::LockingPolicy::SpinLock,
             utils::TrackingPolicy::Debug>;
 
+    utils::SpinLock mHandleSetLock;
     tsl::robin_set<backend::HandleBase::HandleId> mHandleSet;
+    void registerHandleId(backend::HandleBase::HandleId id) noexcept {
+        mHandleSetLock.lock();
+        auto result = mHandleSet.insert(id);
+        assert_invariant(result.second);
+        mHandleSetLock.unlock();
+    }
+    void unregisterHandleId(backend::HandleBase::HandleId id) noexcept {
+        mHandleSetLock.lock();
+        assert_invariant(mHandleSet.find(id) != mHandleSet.cend() );
+        mHandleSet.erase(id);
+        mHandleSetLock.unlock();
+    }
+    void assertHandleId(backend::HandleBase::HandleId id) noexcept {
+        mHandleSetLock.lock();
+        assert_invariant(mHandleSet.find(id) != mHandleSet.cend() );
+        mHandleSetLock.unlock();
+    }
 
 #else
     using HandleArena = utils::Arena<HandleAllocator,
             utils::LockingPolicy::SpinLock>;
+
+    inline void registerHandleId(backend::HandleBase::HandleId id) noexcept {}
+    inline void unregisterHandleId(backend::HandleBase::HandleId id) noexcept {}
+    inline void assertHandleId(backend::HandleBase::HandleId id) noexcept {}
+
 #endif
 
     HandleArena mHandleArena;
@@ -291,9 +314,7 @@ private:
     handle_cast(backend::Handle<B>& handle) noexcept {
         assert_invariant(handle);
         if (!handle) return nullptr; // better to get a NPE than random behavior/corruption
-#ifndef NDEBUG
-        assert_invariant(mHandleSet.find(handle.getId()) != mHandleSet.end());
-#endif
+        assertHandleId(handle.getId());
         char* const base = (char *)mHandleArena.getArea().begin();
         size_t offset = handle.getId() << HandleAllocator::MIN_ALIGNMENT_SHIFT;
         // assert that this handle is even a valid one
diff --git a/libs/utils/src/debug.cpp b/libs/utils/src/debug.cpp
index 9900c3c02d..f033770a01 100644
--- a/libs/utils/src/debug.cpp
+++ b/libs/utils/src/debug.cpp
@@ -20,9 +20,18 @@
 
 namespace utils {
 
-void panic(const char *func, const char * file, int line, const char *assertion) noexcept {
-    PANIC_LOG("%s:%d: failed assertion `%s'\n", file, line, assertion);
+// we use a non-inlined, not marked as "no return" function for aborting so that we can set
+// a breakpoint on the call to abort() in panic() below and skip over it in the debugger if
+// needed.
+UTILS_NOINLINE
+void abort() noexcept {
     std::abort();
 }
 
+void panic(const char *func, const char * file, int line, const char *assertion) noexcept {
+    PANIC_LOG("%s:%d: failed assertion `%s'\n", file, line, assertion);
+    abort(); // set a breakpoint here
+    return;  // this line is needed to be able to move the cursor here in the debugger
+}
+
 } // namespace filament