From a6cf7b5b1da4eee6c30803f77d458f5972cf6a99 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Mon, 11 May 2020 20:26:12 +0000 Subject: [PATCH] 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 Reviewed-by: Austin Eng --- src/dawn_native/Device.cpp | 1 + src/dawn_native/ErrorScope.cpp | 48 +++++++++++++++---- src/dawn_native/ErrorScope.h | 7 ++- src/dawn_native/ErrorScopeTracker.cpp | 2 +- .../validation/ErrorScopeValidationTests.cpp | 8 ++++ 5 files changed, 53 insertions(+), 13 deletions(-) diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 4bbca1d02f..c75eed7c04 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -152,6 +152,7 @@ namespace dawn_native { mState = State::Disconnected; mErrorScopeTracker = nullptr; + mCurrentErrorScope->UnlinkForShutdown(); mFenceSignalTracker = nullptr; mDynamicUploader = nullptr; diff --git a/src/dawn_native/ErrorScope.cpp b/src/dawn_native/ErrorScope.cpp index 2facb8be5c..daa0e48f2a 100644 --- a/src/dawn_native/ErrorScope.cpp +++ b/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()) { - return; + if (!IsRoot()) { + RunNonRootCallback(); } - mCallback(static_cast(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(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() { - if (!IsRoot()) { - mErrorType = wgpu::ErrorType::Unknown; - mErrorMessage = "Error scope destroyed"; + // static + void ErrorScope::UnlinkForShutdownImpl(ErrorScope* scope) { + Ref currentScope = scope; + Ref 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(); } } diff --git a/src/dawn_native/ErrorScope.h b/src/dawn_native/ErrorScope.h index eea066def4..75327d5569 100644 --- a/src/dawn_native/ErrorScope.h +++ b/src/dawn_native/ErrorScope.h @@ -44,16 +44,19 @@ namespace dawn_native { ErrorScope* GetParent(); void HandleError(wgpu::ErrorType type, const char* message); - - void Destroy(); + void UnlinkForShutdown(); 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 mParent = nullptr; + bool mIsRoot; wgpu::ErrorCallback mCallback = nullptr; void* mUserdata = nullptr; diff --git a/src/dawn_native/ErrorScopeTracker.cpp b/src/dawn_native/ErrorScopeTracker.cpp index ea5d5a48cd..409b36b696 100644 --- a/src/dawn_native/ErrorScopeTracker.cpp +++ b/src/dawn_native/ErrorScopeTracker.cpp @@ -30,7 +30,7 @@ namespace dawn_native { // with UNKNOWN. constexpr Serial maxSerial = std::numeric_limits::max(); for (Ref& scope : mScopesInFlight.IterateUpTo(maxSerial)) { - scope->Destroy(); + scope->UnlinkForShutdown(); } Tick(maxSerial); } diff --git a/src/tests/unittests/validation/ErrorScopeValidationTests.cpp b/src/tests/unittests/validation/ErrorScopeValidationTests.cpp index 4672247876..5bcbac1f05 100644 --- a/src/tests/unittests/validation/ErrorScopeValidationTests.cpp +++ b/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); + } +}