[dawn] Make the Instance simply RefCounted.

- Before, the Instance needed to be externally refcounted because
  there wasn't a way to get the Instance once the last external
  reference was dropped, hence it could cause any WaitAnyOnly or
  AllowProcessEvent events to be leaked. Since then, we have
  added a `GetInstance()` API on the Adapter which means that as
  long as we have a Device (which has `GetAdapter()`) on it, the
  instance can be reached. As a result, we only need to clear all
  events when the last device reference is dropped. This means
  that the Instance can just be a simple refcounted object now.
- This is also necessary because the `WaitAny()` and
  `ProcessEvents()` APIs implicitly assume that if they are called,
  that the Instance is externally alive. However, because users
  can drop the last external reference, then get a new one when
  they need it via the Device/Adapter, some teardown code in
  Chromium could cause a crash by calling those APIs on an
  Instance retrieved via the getter APIs after the original
  external reference was dropped.

Bug: 467732049, 474662276
Change-Id: Ib04c6243538983c2ba4c3060beb23487da66a499
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/283515
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Auto-Submit: Loko Kung <lokokung@google.com>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>

src/dawn/native/Instance.cpp

diff --git a/src/dawn/native/Instance.cpp b/src/dawn/native/Instance.cpp
index 045b3d9..35e9354 100644
--- a/src/dawn/native/Instance.cpp
+++ b/src/dawn/native/Instance.cpp
@@ -224,6 +224,10 @@
 InstanceBase::~InstanceBase() = default;
 
 void InstanceBase::DeleteThis() {
+    // Stop tracking events. See comment on ShutDown.
+    mEventManager.ShutDown();
+    mLoggingCallbackInfo = kEmptyLoggingCallbackInfo;
+
     // Flush all remaining callback tasks on all devices and on the instance.
     absl::flat_hash_set<DeviceBase*> devices;
     do {
@@ -242,19 +246,13 @@
         mCallbackTaskManager->Flush();
     } while (!mCallbackTaskManager->IsEmpty());
 
-    RefCountedWithExternalCount::DeleteThis();
+    RefCounted::DeleteThis();
 }
 
 void InstanceBase::DisconnectDawnPlatform() {
     SetPlatform(nullptr);
 }
 
-void InstanceBase::WillDropLastExternalRef() {
-    // Stop tracking events. See comment on ShutDown.
-    mEventManager.ShutDown();
-    mLoggingCallbackInfo = kEmptyLoggingCallbackInfo;
-}
-
 // TODO(crbug.com/dawn/832): make the platform an initialization parameter of the instance.
 MaybeError InstanceBase::Initialize(const UnpackedPtr<InstanceDescriptor>& descriptor) {
     // Initialize the platform to the default for now.

src/dawn/native/Instance.h

diff --git a/src/dawn/native/Instance.h b/src/dawn/native/Instance.h
index f1ee301..7a26657 100644
--- a/src/dawn/native/Instance.h
+++ b/src/dawn/native/Instance.h
@@ -38,7 +38,7 @@
 #include "absl/container/flat_hash_set.h"
 #include "dawn/common/MutexProtected.h"
 #include "dawn/common/Ref.h"
-#include "dawn/common/RefCountedWithExternalCount.h"
+#include "dawn/common/RefCounted.h"
 #include "dawn/common/ityp_array.h"
 #include "dawn/common/ityp_bitset.h"
 #include "dawn/native/Adapter.h"
@@ -74,7 +74,7 @@
 
 // This is called InstanceBase for consistency across the frontend, even if the backends don't
 // specialize this class.
-class InstanceBase final : public ErrorSink, public RefCountedWithExternalCount<RefCounted> {
+class InstanceBase final : public ErrorSink, public RefCounted {
   public:
     static ResultOrError<Ref<InstanceBase>> Create(const InstanceDescriptor* descriptor = nullptr);
 
@@ -162,7 +162,6 @@
     ~InstanceBase() override;
 
     void DeleteThis() override;
-    void WillDropLastExternalRef() override;
 
     InstanceBase(const InstanceBase& other) = delete;
     InstanceBase& operator=(const InstanceBase& other) = delete;

src/dawn/tests/end2end/EventTests.cpp

diff --git a/src/dawn/tests/end2end/EventTests.cpp b/src/dawn/tests/end2end/EventTests.cpp
index c2d60bf..efd6c19 100644
--- a/src/dawn/tests/end2end/EventTests.cpp
+++ b/src/dawn/tests/end2end/EventTests.cpp
@@ -451,43 +451,6 @@
     TestWaitAll(/*loopOnlyOnce=*/true);
 }
 
-TEST_P(EventCompletionTests, WorkDoneDropInstanceBeforeEvent) {
-    // TODO(crbug.com/dawn/1987): Wire does not implement instance destruction correctly yet.
-    DAWN_TEST_UNSUPPORTED_IF(UsesWire());
-    // Spontaneous events only make sense with WaitType::Spin since we are dropping the instance.
-    DAWN_TEST_UNSUPPORTED_IF(IsSpontaneous() && GetWaitType() != WaitType::Spin);
-
-    UseSecondInstance();
-    testInstance = nullptr;  // Drop the last external ref to the instance.
-
-    if (IsSpontaneous()) {
-        TrackForTest(OnSubmittedWorkDone(wgpu::QueueWorkDoneStatus::Success));
-        TestWaitAll();
-    } else {
-        TrackForTest(OnSubmittedWorkDone(wgpu::QueueWorkDoneStatus::CallbackCancelled));
-        EXPECT_EQ(mCallbacksCompletedCount, 1u);
-    }
-}
-
-TEST_P(EventCompletionTests, WorkDoneDropInstanceAfterEvent) {
-    // TODO(crbug.com/dawn/1987): Wire does not implement instance destruction correctly yet.
-    DAWN_TEST_UNSUPPORTED_IF(UsesWire());
-    // Spontaneous events only make sense with WaitType::Spin since we are dropping the instance.
-    DAWN_TEST_UNSUPPORTED_IF(IsSpontaneous() && GetWaitType() != WaitType::Spin);
-
-    UseSecondInstance();
-
-    if (IsSpontaneous()) {
-        TrackForTest(OnSubmittedWorkDone(wgpu::QueueWorkDoneStatus::Success));
-        testInstance = nullptr;  // Drop the last external ref to the instance.
-        TestWaitAll();
-    } else {
-        TrackForTest(OnSubmittedWorkDone(wgpu::QueueWorkDoneStatus::CallbackCancelled));
-        testInstance = nullptr;  // Drop the last external ref to the instance.
-        EXPECT_EQ(mCallbacksCompletedCount, 1u);
-    }
-}
-
 // TODO(crbug.com/dawn/1987):
 // - Test any reentrancy guarantees (for ProcessEvents or WaitAny inside a callback),
 //   to make sure things don't blow up and we don't attempt to hold locks recursively.