Remove bug-prone SkIDChangeListener singleThreaded arg

A given object may be unique, but its owner may not be unique
and another thread may ref the object via the owner at any time.

This happens e.g. when sharing SkPictures between threads.

Bug: skia:10286
Change-Id: I51b5239338a81aaa4d67db05f01f2c7c24182096
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376619
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Adlai Holler <adlai@google.com>

include/private/SkIDChangeListener.h

@@ -49,23 +49,23 @@ public:
          * Add a new listener to the list. It must not already be deregistered. Also clears out
          * previously deregistered listeners.
          */
-        void add(sk_sp<SkIDChangeListener> listener, bool singleThreaded = false);
+        void add(sk_sp<SkIDChangeListener> listener) SK_EXCLUDES(fMutex);
 
         /**
          * The number of registered listeners (including deregisterd listeners that are yet-to-be
          * removed.
          */
-        int count();
+        int count() const SK_EXCLUDES(fMutex);
 
         /** Calls changed() on all listeners that haven't been deregistered and resets the list. */
-        void changed(bool singleThreaded = false);
+        void changed() SK_EXCLUDES(fMutex);
 
         /** Resets without calling changed() on the listeners. */
-        void reset(bool singleThreaded = false);
+        void reset() SK_EXCLUDES(fMutex);
 
     private:
-        SkMutex fMutex;
-        SkTDArray<SkIDChangeListener*> fListeners;  // pointers are reffed
+        mutable SkMutex fMutex;
+        SkTDArray<SkIDChangeListener*> fListeners SK_GUARDED_BY(fMutex);  // pointers are reffed
     };
 
 private:

src/core/SkIDChangeListener.cpp

@@ -34,61 +34,41 @@ List::~List() {
     }
 }
 
-void List::add(sk_sp<SkIDChangeListener> listener, bool singleThreaded) {
+void List::add(sk_sp<SkIDChangeListener> listener) {
     if (!listener) {
         return;
     }
     SkASSERT(!listener->shouldDeregister());
 
-    auto add = [&] {
-        // Clean out any stale listeners before we append the new one.
-        for (int i = 0; i < fListeners.count(); ++i) {
-            if (fListeners[i]->shouldDeregister()) {
-                fListeners[i]->unref();
-                fListeners.removeShuffle(i--);  // No need to preserve the order after i.
-            }
+    SkAutoMutexExclusive lock(fMutex);
+    // Clean out any stale listeners before we append the new one.
+    for (int i = 0; i < fListeners.count(); ++i) {
+        if (fListeners[i]->shouldDeregister()) {
+            fListeners[i]->unref();
+            fListeners.removeShuffle(i--);  // No need to preserve the order after i.
         }
-        *fListeners.append() = listener.release();
-    };
-
-    if (singleThreaded) {
-        add();
-    } else {
-        SkAutoMutexExclusive lock(fMutex);
-        add();
     }
+    *fListeners.append() = listener.release();
 }
 
-int List::count() {
+int List::count() const {
     SkAutoMutexExclusive lock(fMutex);
     return fListeners.count();
 }
 
-void List::changed(bool singleThreaded) {
-    auto visit = [this]() {
-        for (SkIDChangeListener* listener : fListeners) {
-            if (!listener->shouldDeregister()) {
-                listener->changed();
-            }
-            // Listeners get at most one shot, so whether these triggered or not, blow them away.
-            listener->unref();
+void List::changed() {
+    SkAutoMutexExclusive lock(fMutex);
+    for (SkIDChangeListener* listener : fListeners) {
+        if (!listener->shouldDeregister()) {
+            listener->changed();
         }
-        fListeners.reset();
-    };
-
-    if (singleThreaded) {
-        visit();
-    } else {
-        SkAutoMutexExclusive lock(fMutex);
-        visit();
+        // Listeners get at most one shot, so whether these triggered or not, blow them away.
+        listener->unref();
     }
+    fListeners.reset();
 }
 
-void List::reset(bool singleThreaded) {
-    if (singleThreaded) {
-        fListeners.unrefAll();
-    } else {
-        SkAutoMutexExclusive lock(fMutex);
-        fListeners.unrefAll();
-    }
+void List::reset() {
+    SkAutoMutexExclusive lock(fMutex);
+    fListeners.unrefAll();
 }

src/core/SkPathRef.cpp

@@ -490,16 +490,14 @@ void SkPathRef::addGenIDChangeListener(sk_sp<SkIDChangeListener> listener) {
     if (this == gEmpty) {
         return;
     }
-    bool singleThreaded = this->unique();
-    fGenIDChangeListeners.add(std::move(listener), singleThreaded);
+    fGenIDChangeListeners.add(std::move(listener));
 }
 
 int SkPathRef::genIDChangeListenerCount() { return fGenIDChangeListeners.count(); }
 
 // we need to be called *before* the genID gets changed or zerod
 void SkPathRef::callGenIDChangeListeners() {
-    bool singleThreaded = this->unique();
-    fGenIDChangeListeners.changed(singleThreaded);
+    fGenIDChangeListeners.changed();
 }
 
 SkRRect SkPathRef::getRRect() const {

src/core/SkPixelRef.cpp

@@ -79,23 +79,21 @@ void SkPixelRef::addGenIDChangeListener(sk_sp<SkIDChangeListener> listener) {
         return;
     }
     SkASSERT(!listener->shouldDeregister());
-    bool singleThreaded = this->unique();
-    fGenIDChangeListeners.add(std::move(listener), singleThreaded);
+    fGenIDChangeListeners.add(std::move(listener));
 }
 
 // we need to be called *before* the genID gets changed or zerod
 void SkPixelRef::callGenIDChangeListeners() {
-    bool singleThreaded = this->unique();
     // We don't invalidate ourselves if we think another SkPixelRef is sharing our genID.
     if (this->genIDIsUnique()) {
-        fGenIDChangeListeners.changed(singleThreaded);
+        fGenIDChangeListeners.changed();
         if (fAddedToCache.exchange(false)) {
             SkNotifyBitmapGenIDIsStale(this->getGenerationID());
         }
     } else {
         // Listeners get at most one shot, so even though these weren't triggered or not, blow them
         // away.
-        fGenIDChangeListeners.reset(singleThreaded);
+        fGenIDChangeListeners.reset();
     }
 }
 

src/image/SkImage_Lazy.cpp

@@ -503,7 +503,6 @@ GrColorType SkImage_Lazy::colorTypeOfLockTextureProxy(const GrCaps* caps) const
 }
 
 void SkImage_Lazy::addUniqueIDListener(sk_sp<SkIDChangeListener> listener) const {
-    bool singleThreaded = this->unique();
-    fUniqueIDListeners.add(std::move(listener), singleThreaded);
+    fUniqueIDListeners.add(std::move(listener));
 }
 #endif