Make error scope shutdown iterative instead of recursive.

This avoids a stack overflow when many error scopes are pushed on device
shutdown. It also changes the error scopes to return a Unknown error
type on shutdown instead of NoError.

A regression test is added.

Bug: chromium:1078438
Bug: chromium:1081063
Change-Id: Ibfab8dd19480414c1854ec2bd4928939663ba698
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/21440
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>

src/dawn_native/Device.cpp

@@ -152,6 +152,7 @@ namespace dawn_native {
         mState = State::Disconnected;
 
         mErrorScopeTracker = nullptr;
+        mCurrentErrorScope->UnlinkForShutdown();
         mFenceSignalTracker = nullptr;
         mDynamicUploader = nullptr;
 

src/dawn_native/ErrorScope.cpp

@@ -18,18 +18,18 @@
 
 namespace dawn_native {
 
-    ErrorScope::ErrorScope() = default;
+    ErrorScope::ErrorScope() : mIsRoot(true) {
+    }
 
     ErrorScope::ErrorScope(wgpu::ErrorFilter errorFilter, ErrorScope* parent)
-        : RefCounted(), mErrorFilter(errorFilter), mParent(parent) {
+        : RefCounted(), mErrorFilter(errorFilter), mParent(parent), mIsRoot(false) {
         ASSERT(mParent.Get() != nullptr);
     }
 
     ErrorScope::~ErrorScope() {
-        if (mCallback == nullptr || IsRoot()) {
+        if (!IsRoot()) {
-            return;
+            RunNonRootCallback();
         }
-        mCallback(static_cast<WGPUErrorType>(mErrorType), mErrorMessage.c_str(), mUserdata);
     }
 
     void ErrorScope::SetCallback(wgpu::ErrorCallback callback, void* userdata) {
@@ -42,13 +42,27 @@ namespace dawn_native {
     }
 
     bool ErrorScope::IsRoot() const {
-        return mParent.Get() == nullptr;
+        return mIsRoot;
+    }
+
+    void ErrorScope::RunNonRootCallback() {
+        ASSERT(!IsRoot());
+
+        if (mCallback != nullptr) {
+            // For non-root error scopes, the callback can run at most once.
+            mCallback(static_cast<WGPUErrorType>(mErrorType), mErrorMessage.c_str(), mUserdata);
+            mCallback = nullptr;
+        }
     }
 
     void ErrorScope::HandleError(wgpu::ErrorType type, const char* message) {
         HandleErrorImpl(this, type, message);
     }
 
+    void ErrorScope::UnlinkForShutdown() {
+        UnlinkForShutdownImpl(this);
+    }
+
     // static
     void ErrorScope::HandleErrorImpl(ErrorScope* scope, wgpu::ErrorType type, const char* message) {
         ErrorScope* currentScope = scope;
@@ -105,10 +119,24 @@ namespace dawn_native {
         }
     }
 
-    void ErrorScope::Destroy() {
+    // static
-        if (!IsRoot()) {
+    void ErrorScope::UnlinkForShutdownImpl(ErrorScope* scope) {
-            mErrorType = wgpu::ErrorType::Unknown;
+        Ref<ErrorScope> currentScope = scope;
-            mErrorMessage = "Error scope destroyed";
+        Ref<ErrorScope> parentScope = nullptr;
+        for (; !currentScope->IsRoot(); currentScope = parentScope.Get()) {
+            ASSERT(!currentScope->IsRoot());
+            ASSERT(currentScope.Get() != nullptr);
+            parentScope = std::move(currentScope->mParent);
+            ASSERT(parentScope.Get() != nullptr);
+
+            // On shutdown, error scopes that have yet to have a status get Unknown.
+            if (currentScope->mErrorType == wgpu::ErrorType::NoError) {
+                currentScope->mErrorType = wgpu::ErrorType::Unknown;
+                currentScope->mErrorMessage = "Error scope destroyed";
+            }
+
+            // Run the callback if it hasn't run already.
+            currentScope->RunNonRootCallback();
         }
     }
 

src/dawn_native/ErrorScope.h

@@ -44,16 +44,19 @@ namespace dawn_native {
         ErrorScope* GetParent();
 
         void HandleError(wgpu::ErrorType type, const char* message);
-
+        void UnlinkForShutdown();
-        void Destroy();
 
       private:
         ~ErrorScope() override;
         bool IsRoot() const;
+        void RunNonRootCallback();
+
         static void HandleErrorImpl(ErrorScope* scope, wgpu::ErrorType type, const char* message);
+        static void UnlinkForShutdownImpl(ErrorScope* scope);
 
         wgpu::ErrorFilter mErrorFilter = wgpu::ErrorFilter::None;
         Ref<ErrorScope> mParent = nullptr;
+        bool mIsRoot;
 
         wgpu::ErrorCallback mCallback = nullptr;
         void* mUserdata = nullptr;

src/dawn_native/ErrorScopeTracker.cpp

@@ -30,7 +30,7 @@ namespace dawn_native {
         // with UNKNOWN.
         constexpr Serial maxSerial = std::numeric_limits<Serial>::max();
         for (Ref<ErrorScope>& scope : mScopesInFlight.IterateUpTo(maxSerial)) {
-            scope->Destroy();
+            scope->UnlinkForShutdown();
         }
         Tick(maxSerial);
     }

src/tests/unittests/validation/ErrorScopeValidationTests.cpp

@@ -196,3 +196,11 @@ TEST_F(ErrorScopeValidationTest, DeviceDestroyedBeforeCallback) {
     EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_NoError, _, this)).Times(1);
     device = nullptr;
 }
+
+// Regression test that on device shutdown, we don't get a recursion in O(pushed error scope) that
+// would lead to a stack overflow
+TEST_F(ErrorScopeValidationTest, ShutdownStackOverflow) {
+    for (size_t i = 0; i < 1'000'000; i++) {
+        device.PushErrorScope(wgpu::ErrorFilter::Validation);
+    }
+}