common: RefBase fixes and improvements

Fix missing Release() on RefBase::operator=(const T&).

Always acquire the new reference before dropping the existing reference.
Consider:
* Object A holds the only reference to object B.
* Ref of A is assigned B.
If A were released before referencing B, then B would be a dead pointer.

Remove RefBase::kNullValue, just use Traits::kNullValue. Avoids an unnecessary constexpr copy.

Add even more tests.

Change-Id: I111079d56cbf8c09162aa6e493078bead9ae97fa
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/55882
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/common/RefBase.h

@@ -33,17 +33,13 @@
 // RefBase supports
 template <typename T, typename Traits>
 class RefBase {
-  private:
-    static constexpr T kNullValue = Traits::kNullValue;
-
   public:
     // Default constructor and destructor.
-    RefBase() : mValue(kNullValue) {
+    RefBase() : mValue(Traits::kNullValue) {
     }
 
     ~RefBase() {
-        Release();
+        Release(mValue);
-        mValue = kNullValue;
     }
 
     // Constructors from nullptr.
@@ -51,49 +47,39 @@ class RefBase {
     }
 
     RefBase<T, Traits>& operator=(std::nullptr_t) {
-        Release();
+        Set(Traits::kNullValue);
-        mValue = kNullValue;
         return *this;
     }
 
     // Constructors from a value T.
     RefBase(T value) : mValue(value) {
-        Reference();
+        Reference(value);
     }
 
     RefBase<T, Traits>& operator=(const T& value) {
-        mValue = value;
+        Set(value);
-        Reference();
         return *this;
     }
 
     // Constructors from a RefBase<T>
     RefBase(const RefBase<T, Traits>& other) : mValue(other.mValue) {
-        Reference();
+        Reference(other.mValue);
     }
 
     RefBase<T, Traits>& operator=(const RefBase<T, Traits>& other) {
-        if (&other != this) {
+        Set(other.mValue);
-            other.Reference();
-            Release();
-            mValue = other.mValue;
-        }
-
         return *this;
     }
 
     RefBase(RefBase<T, Traits>&& other) {
-        mValue = other.mValue;
+        mValue = other.Detach();
-        other.mValue = kNullValue;
     }
 
     RefBase<T, Traits>& operator=(RefBase<T, Traits>&& other) {
         if (&other != this) {
-            Release();
+            Release(mValue);
-            mValue = other.mValue;
+            mValue = other.Detach();
-            other.mValue = kNullValue;
         }
-
         return *this;
     }
 
@@ -102,14 +88,12 @@ class RefBase {
     // operators defined with `other` == RefBase<T, Traits>.
     template <typename U, typename UTraits, typename = typename std::is_convertible<U, T>::type>
     RefBase(const RefBase<U, UTraits>& other) : mValue(other.mValue) {
-        Reference();
+        Reference(other.mValue);
     }
 
     template <typename U, typename UTraits, typename = typename std::is_convertible<U, T>::type>
     RefBase<T, Traits>& operator=(const RefBase<U, UTraits>& other) {
-        other.Reference();
+        Set(other.mValue);
-        Release();
-        mValue = other.mValue;
         return *this;
     }
 
@@ -120,7 +104,7 @@ class RefBase {
 
     template <typename U, typename UTraits, typename = typename std::is_convertible<U, T>::type>
     RefBase<T, Traits>& operator=(RefBase<U, UTraits>&& other) {
-        Release();
+        Release(mValue);
         mValue = other.Detach();
         return *this;
     }
@@ -150,18 +134,18 @@ class RefBase {
     }
 
     T Detach() DAWN_NO_DISCARD {
-        T value = mValue;
+        T value{std::move(mValue)};
-        mValue = kNullValue;
+        mValue = Traits::kNullValue;
         return value;
     }
 
     void Acquire(T value) {
-        Release();
+        Release(mValue);
         mValue = value;
     }
 
     T* InitializeInto() DAWN_NO_DISCARD {
-        ASSERT(mValue == kNullValue);
+        ASSERT(mValue == Traits::kNullValue);
         return &mValue;
     }
 
@@ -171,14 +155,24 @@ class RefBase {
     template <typename U, typename UTraits>
     friend class RefBase;
 
-    void Reference() const {
+    static void Reference(T value) {
-        if (mValue != kNullValue) {
+        if (value != Traits::kNullValue) {
-            Traits::Reference(mValue);
+            Traits::Reference(value);
         }
     }
-    void Release() const {
+    static void Release(T value) {
-        if (mValue != kNullValue) {
+        if (value != Traits::kNullValue) {
-            Traits::Release(mValue);
+            Traits::Release(value);
+        }
+    }
+
+    void Set(T value) {
+        if (mValue != value) {
+            // Ensure that the new value is referenced before the old is released to prevent any
+            // transitive frees that may affect the new value.
+            Reference(value);
+            Release(mValue);
+            mValue = value;
         }
     }
 

src/tests/BUILD.gn

@@ -174,6 +174,7 @@ test("dawn_unittests") {
     "unittests/PerStageTests.cpp",
     "unittests/PerThreadProcTests.cpp",
     "unittests/PlacementAllocatedTests.cpp",
+    "unittests/RefBaseTests.cpp",
     "unittests/RefCountedTests.cpp",
     "unittests/ResultTests.cpp",
     "unittests/RingBufferAllocatorTests.cpp",

src/tests/unittests/ObjectBaseTests.cpp

@@ -120,6 +120,24 @@ TEST(ObjectBase, CopyAssignment) {
     ASSERT_EQ(refcount, 2);
 }
 
+// Test the repeated copy assignment of C++ objects
+TEST(ObjectBase, RepeatedCopyAssignment) {
+    int refcount = 1;
+    Object source(&refcount);
+
+    Object destination;
+    for (int i = 0; i < 10; i++) {
+        destination = source;
+    }
+
+    ASSERT_EQ(source.Get(), &refcount);
+    ASSERT_EQ(destination.Get(), &refcount);
+    ASSERT_EQ(3, refcount);
+
+    destination = Object();
+    ASSERT_EQ(refcount, 2);
+}
+
 // Test the copy assignment of C++ objects onto themselves
 TEST(ObjectBase, CopyAssignmentSelf) {
     int refcount = 1;

src/tests/unittests/RefBaseTests.cpp

@@ -0,0 +1,319 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <gmock/gmock.h>
+
+#include "common/RefBase.h"
+
+namespace {
+    using Id = uint32_t;
+
+    enum class Action {
+        kReference,
+        kRelease,
+        kAssign,
+        kMarker,
+    };
+
+    struct Event {
+        Action action;
+        Id thisId = 0;
+        Id otherId = 0;
+    };
+
+    std::ostream& operator<<(std::ostream& os, const Event& event) {
+        switch (event.action) {
+            case Action::kReference:
+                os << "Reference " << event.thisId;
+                break;
+            case Action::kRelease:
+                os << "Release " << event.thisId;
+                break;
+            case Action::kAssign:
+                os << "Assign " << event.thisId << " <- " << event.otherId;
+                break;
+            case Action::kMarker:
+                os << "Marker " << event.thisId;
+                break;
+        }
+        return os;
+    }
+
+    bool operator==(const Event& a, const Event& b) {
+        return a.action == b.action && a.thisId == b.thisId && a.otherId == b.otherId;
+    }
+
+    using Events = std::vector<Event>;
+
+    struct RefTracker {
+        explicit constexpr RefTracker(nullptr_t) : mId(0), mEvents(nullptr) {
+        }
+
+        constexpr RefTracker(const RefTracker& other) = default;
+
+        RefTracker(Id id, Events* events) : mId(id), mEvents(events) {
+        }
+
+        void Reference() const {
+            mEvents->emplace_back(Event{Action::kReference, mId});
+        }
+
+        void Release() const {
+            mEvents->emplace_back(Event{Action::kRelease, mId});
+        }
+
+        RefTracker& operator=(const RefTracker& other) {
+            if (mEvents || other.mEvents) {
+                Events* events = mEvents ? mEvents : other.mEvents;
+                events->emplace_back(Event{Action::kAssign, mId, other.mId});
+            }
+            mId = other.mId;
+            mEvents = other.mEvents;
+            return *this;
+        }
+
+        bool operator==(const RefTracker& other) const {
+            return mId == other.mId;
+        }
+
+        bool operator!=(const RefTracker& other) const {
+            return mId != other.mId;
+        }
+
+        Id mId;
+        Events* mEvents;
+    };
+
+    struct RefTrackerTraits {
+        static constexpr RefTracker kNullValue{nullptr};
+
+        static void Reference(const RefTracker& handle) {
+            handle.Reference();
+        }
+
+        static void Release(const RefTracker& handle) {
+            handle.Release();
+        }
+    };
+
+    constexpr RefTracker RefTrackerTraits::kNullValue;
+
+    using Ref = RefBase<RefTracker, RefTrackerTraits>;
+}  // namespace
+
+TEST(RefBase, Acquire) {
+    Events events;
+    RefTracker tracker1(1, &events);
+    RefTracker tracker2(2, &events);
+    Ref ref(tracker1);
+
+    events.clear();
+    { ref.Acquire(tracker2); }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kRelease, 1},   // release ref
+                                             Event{Action::kAssign, 1, 2}  // acquire tracker2
+                                             ));
+}
+
+TEST(RefBase, Detach) {
+    Events events;
+    RefTracker tracker(1, &events);
+    Ref ref(tracker);
+
+    events.clear();
+    { DAWN_UNUSED(ref.Detach()); }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kAssign, 1, 0}  // nullify ref
+                                             ));
+}
+
+TEST(RefBase, Constructor) {
+    Ref ref;
+    EXPECT_EQ(ref.Get(), RefTrackerTraits::kNullValue);
+}
+
+TEST(RefBase, ConstructDestruct) {
+    Events events;
+    RefTracker tracker(1, &events);
+
+    events.clear();
+    {
+        Ref ref(tracker);
+        events.emplace_back(Event{Action::kMarker, 10});
+    }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1},  // reference tracker
+                                             Event{Action::kMarker, 10},    //
+                                             Event{Action::kRelease, 1}     // destruct ref
+                                             ));
+}
+
+TEST(RefBase, CopyConstruct) {
+    Events events;
+    RefTracker tracker(1, &events);
+    Ref refA(tracker);
+
+    events.clear();
+    {
+        Ref refB(refA);
+        events.emplace_back(Event{Action::kMarker, 10});
+    }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1},  // reference tracker
+                                             Event{Action::kMarker, 10},    //
+                                             Event{Action::kRelease, 1}     // destruct ref
+                                             ));
+}
+
+TEST(RefBase, RefCopyAssignment) {
+    Events events;
+    RefTracker tracker1(1, &events);
+    RefTracker tracker2(2, &events);
+    Ref refA(tracker1);
+    Ref refB(tracker2);
+
+    events.clear();
+    {
+        Ref ref;
+        events.emplace_back(Event{Action::kMarker, 10});
+        ref = refA;
+        events.emplace_back(Event{Action::kMarker, 20});
+        ref = refB;
+        events.emplace_back(Event{Action::kMarker, 30});
+        ref = refA;
+        events.emplace_back(Event{Action::kMarker, 40});
+    }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kMarker, 10},    //
+                                             Event{Action::kReference, 1},  // reference tracker1
+                                             Event{Action::kAssign, 0, 1},  // copy tracker1
+                                             Event{Action::kMarker, 20},    //
+                                             Event{Action::kReference, 2},  // reference tracker2
+                                             Event{Action::kRelease, 1},    // release tracker1
+                                             Event{Action::kAssign, 1, 2},  // copy tracker2
+                                             Event{Action::kMarker, 30},    //
+                                             Event{Action::kReference, 1},  // reference tracker1
+                                             Event{Action::kRelease, 2},    // release tracker2
+                                             Event{Action::kAssign, 2, 1},  // copy tracker1
+                                             Event{Action::kMarker, 40},    //
+                                             Event{Action::kRelease, 1}     // destruct ref
+                                             ));
+}
+
+TEST(RefBase, RefMoveAssignment) {
+    Events events;
+    RefTracker tracker1(1, &events);
+    RefTracker tracker2(2, &events);
+    Ref refA(tracker1);
+    Ref refB(tracker2);
+
+    events.clear();
+    {
+        Ref ref;
+        events.emplace_back(Event{Action::kMarker, 10});
+        ref = std::move(refA);
+        events.emplace_back(Event{Action::kMarker, 20});
+        ref = std::move(refB);
+        events.emplace_back(Event{Action::kMarker, 30});
+    }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kMarker, 10},    //
+                                             Event{Action::kAssign, 1, 0},  // nullify refA
+                                             Event{Action::kAssign, 0, 1},  // move into ref
+                                             Event{Action::kMarker, 20},    //
+                                             Event{Action::kRelease, 1},    // release tracker1
+                                             Event{Action::kAssign, 2, 0},  // nullify refB
+                                             Event{Action::kAssign, 1, 2},  // move into ref
+                                             Event{Action::kMarker, 30},    //
+                                             Event{Action::kRelease, 2}     // destruct ref
+                                             ));
+}
+
+TEST(RefBase, RefCopyAssignmentSelf) {
+    Events events;
+    RefTracker tracker(1, &events);
+    Ref ref(tracker);
+    Ref& self = ref;
+
+    events.clear();
+    {
+        ref = self;
+        ref = self;
+        ref = self;
+    }
+    EXPECT_THAT(events, testing::ElementsAre());
+}
+
+TEST(RefBase, RefMoveAssignmentSelf) {
+    Events events;
+    RefTracker tracker(1, &events);
+    Ref ref(tracker);
+    Ref& self = ref;
+
+    events.clear();
+    {
+        ref = std::move(self);
+        ref = std::move(self);
+        ref = std::move(self);
+    }
+    EXPECT_THAT(events, testing::ElementsAre());
+}
+
+TEST(RefBase, TCopyAssignment) {
+    Events events;
+    RefTracker tracker(1, &events);
+    Ref ref;
+
+    events.clear();
+    {
+        ref = tracker;
+        ref = tracker;
+        ref = tracker;
+    }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1},  //
+                                             Event{Action::kAssign, 0, 1}));
+}
+
+TEST(RefBase, TMoveAssignment) {
+    Events events;
+    RefTracker tracker(1, &events);
+    Ref ref;
+
+    events.clear();
+    { ref = std::move(tracker); }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1},  //
+                                             Event{Action::kAssign, 0, 1}));
+}
+
+TEST(RefBase, TCopyAssignmentAlternate) {
+    Events events;
+    RefTracker tracker1(1, &events);
+    RefTracker tracker2(2, &events);
+    Ref ref;
+
+    events.clear();
+    {
+        ref = tracker1;
+        events.emplace_back(Event{Action::kMarker, 10});
+        ref = tracker2;
+        events.emplace_back(Event{Action::kMarker, 20});
+        ref = tracker1;
+        events.emplace_back(Event{Action::kMarker, 30});
+    }
+    EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1},  // reference tracker1
+                                             Event{Action::kAssign, 0, 1},  // copy tracker1
+                                             Event{Action::kMarker, 10},    //
+                                             Event{Action::kReference, 2},  // reference tracker2
+                                             Event{Action::kRelease, 1},    // release tracker1
+                                             Event{Action::kAssign, 1, 2},  // copy tracker2
+                                             Event{Action::kMarker, 20},    //
+                                             Event{Action::kReference, 1},  // reference tracker1
+                                             Event{Action::kRelease, 2},    // release tracker2
+                                             Event{Action::kAssign, 2, 1},  // copy tracker1
+                                             Event{Action::kMarker, 30}));
+}