Fix GrGpuBuffer::onRelease() crash problem.
The crash is because method GrGpuBuffer::onRelease() is called on wrong
thread. SkMessageBus::Post(const Message& m) takes a const ref of the
message, so there is a little possibility the GPU thread received and
handled the message before SkMessageBus::Post() is returned. In that
case, the caller of SkMessageBus::Post() (AsyncReadResult) still holds
the last ref of the GrGpuBuffer, and then GrGpuBuffer() will just be
released on the wrong thread.
Bug: chromium:1185489
Change-Id: I28665dbb1db7925d59ec574e9e26385e845ff4df
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/380696
Commit-Queue: Peng Huang <penghuang@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Auto-Submit: Peng Huang <penghuang@chromium.org>
diff --git a/src/core/SkMessageBus.h b/src/core/SkMessageBus.h
index 2a40b28..c6fe957 100644
--- a/src/core/SkMessageBus.h
+++ b/src/core/SkMessageBus.h
@@ -8,6 +8,9 @@
#ifndef SkMessageBus_DEFINED
#define SkMessageBus_DEFINED
+#include <type_traits>
+
+#include "include/core/SkRefCnt.h"
#include "include/core/SkTypes.h"
#include "include/private/SkMutex.h"
#include "include/private/SkNoncopyable.h"
@@ -23,12 +26,20 @@
* We may want to consider providing a default template implementation, to avoid this requirement by
* sending to all inboxes when the specialization for type 'Message' is not present.
*/
-template <typename Message>
-class SkMessageBus : SkNoncopyable {
+template <typename Message, bool AllowCopyableMessage = true> class SkMessageBus : SkNoncopyable {
public:
+ template <typename T> struct is_sk_sp : std::false_type {};
+ template <typename T> struct is_sk_sp<sk_sp<T>> : std::true_type {};
+
+ // We want to make sure the caller of Post() method will not keep a ref or copy of the message,
+ // so the message type must be sk_sp or non copyable.
+ static_assert(AllowCopyableMessage || is_sk_sp<Message>::value ||
+ !std::is_copy_constructible<Message>::value,
+ "The message type must be sk_sp or non copyable.");
+
// Post a message to be received by Inboxes for this Message type. Checks
// SkShouldPostMessageToBus() for each inbox. Threadsafe.
- static void Post(const Message& m);
+ static void Post(Message m);
class Inbox {
public:
@@ -43,10 +54,10 @@
private:
SkTArray<Message> fMessages;
SkMutex fMessagesMutex;
- uint32_t fUniqueID;
+ const uint32_t fUniqueID;
friend class SkMessageBus;
- void receive(const Message& m); // SkMessageBus is a friend only to call this.
+ void receive(Message m); // SkMessageBus is a friend only to call this.
};
private:
@@ -59,29 +70,30 @@
// This must go in a single .cpp file, not some .h, or we risk creating more than one global
// SkMessageBus per type when using shared libraries. NOTE: at most one per file will compile.
-#define DECLARE_SKMESSAGEBUS_MESSAGE(Message) \
- template <> \
- SkMessageBus<Message>* SkMessageBus<Message>::Get() { \
- static SkOnce once; \
- static SkMessageBus<Message>* bus; \
- once([] { bus = new SkMessageBus<Message>(); }); \
- return bus; \
+#define DECLARE_SKMESSAGEBUS_MESSAGE(Message, AllowCopyableMessage) \
+ template <> \
+ SkMessageBus<Message, AllowCopyableMessage>* \
+ SkMessageBus<Message, AllowCopyableMessage>::Get() { \
+ static SkOnce once; \
+ static SkMessageBus<Message, AllowCopyableMessage>* bus; \
+ once([] { bus = new SkMessageBus<Message, AllowCopyableMessage>(); }); \
+ return bus; \
}
// ----------------------- Implementation of SkMessageBus::Inbox -----------------------
-template<typename Message>
-SkMessageBus<Message>::Inbox::Inbox(uint32_t uniqueID) : fUniqueID(uniqueID) {
+template <typename Message, bool AllowCopyableMessage>
+SkMessageBus<Message, AllowCopyableMessage>::Inbox::Inbox(uint32_t uniqueID) : fUniqueID(uniqueID) {
// Register ourselves with the corresponding message bus.
- SkMessageBus<Message>* bus = SkMessageBus<Message>::Get();
+ auto* bus = SkMessageBus<Message, AllowCopyableMessage>::Get();
SkAutoMutexExclusive lock(bus->fInboxesMutex);
bus->fInboxes.push_back(this);
}
-template<typename Message>
-SkMessageBus<Message>::Inbox::~Inbox() {
+template <typename Message, bool AllowCopyableMessage>
+SkMessageBus<Message, AllowCopyableMessage>::Inbox::~Inbox() {
// Remove ourselves from the corresponding message bus.
- SkMessageBus<Message>* bus = SkMessageBus<Message>::Get();
+ auto* bus = SkMessageBus<Message, AllowCopyableMessage>::Get();
SkAutoMutexExclusive lock(bus->fInboxesMutex);
// This is a cheaper fInboxes.remove(fInboxes.find(this)) when order doesn't matter.
for (int i = 0; i < bus->fInboxes.count(); i++) {
@@ -92,14 +104,14 @@
}
}
-template<typename Message>
-void SkMessageBus<Message>::Inbox::receive(const Message& m) {
+template <typename Message, bool AllowCopyableMessage>
+void SkMessageBus<Message, AllowCopyableMessage>::Inbox::receive(Message m) {
SkAutoMutexExclusive lock(fMessagesMutex);
- fMessages.push_back(m);
+ fMessages.push_back(std::move(m));
}
-template<typename Message>
-void SkMessageBus<Message>::Inbox::poll(SkTArray<Message>* messages) {
+template <typename Message, bool AllowCopyableMessage>
+void SkMessageBus<Message, AllowCopyableMessage>::Inbox::poll(SkTArray<Message>* messages) {
SkASSERT(messages);
messages->reset();
SkAutoMutexExclusive lock(fMessagesMutex);
@@ -108,18 +120,31 @@
// ----------------------- Implementation of SkMessageBus -----------------------
-template <typename Message>
-SkMessageBus<Message>::SkMessageBus() {}
+template <typename Message, bool AllowCopyableMessage>
+SkMessageBus<Message, AllowCopyableMessage>::SkMessageBus() = default;
-template <typename Message>
-/*static*/ void SkMessageBus<Message>::Post(const Message& m) {
- SkMessageBus<Message>* bus = SkMessageBus<Message>::Get();
+template <typename Message, bool AllowCopyableMessage>
+/*static*/ void SkMessageBus<Message, AllowCopyableMessage>::Post(Message m) {
+ auto* bus = SkMessageBus<Message, AllowCopyableMessage>::Get();
SkAutoMutexExclusive lock(bus->fInboxesMutex);
for (int i = 0; i < bus->fInboxes.count(); i++) {
if (SkShouldPostMessageToBus(m, bus->fInboxes[i]->fUniqueID)) {
- bus->fInboxes[i]->receive(m);
+ if constexpr (AllowCopyableMessage) {
+ bus->fInboxes[i]->receive(m);
+ } else {
+ if constexpr (is_sk_sp<Message>::value) {
+ SkASSERT(m->unique());
+ }
+ bus->fInboxes[i]->receive(std::move(m));
+ break;
+ }
}
}
+
+ if constexpr (is_sk_sp<Message>::value && !AllowCopyableMessage) {
+ // Make sure sk_sp has been sent to an inbox.
+ SkASSERT(!m); // NOLINT(bugprone-use-after-move)
+ }
}
#endif // SkMessageBus_DEFINED
diff --git a/src/core/SkResourceCache.cpp b/src/core/SkResourceCache.cpp
index 167d81e..ae100a4 100644
--- a/src/core/SkResourceCache.cpp
+++ b/src/core/SkResourceCache.cpp
@@ -19,7 +19,7 @@
#include <stddef.h>
#include <stdlib.h>
-DECLARE_SKMESSAGEBUS_MESSAGE(SkResourceCache::PurgeSharedIDMessage)
+DECLARE_SKMESSAGEBUS_MESSAGE(SkResourceCache::PurgeSharedIDMessage, true)
static inline bool SkShouldPostMessageToBus(
const SkResourceCache::PurgeSharedIDMessage&, uint32_t) {
diff --git a/src/gpu/GrClientMappedBufferManager.cpp b/src/gpu/GrClientMappedBufferManager.cpp
index b927726..1dd3860 100644
--- a/src/gpu/GrClientMappedBufferManager.cpp
+++ b/src/gpu/GrClientMappedBufferManager.cpp
@@ -63,7 +63,7 @@
//////////////////////////////////////////////////////////////////////////////
-DECLARE_SKMESSAGEBUS_MESSAGE(GrClientMappedBufferManager::BufferFinishedMessage)
+DECLARE_SKMESSAGEBUS_MESSAGE(GrClientMappedBufferManager::BufferFinishedMessage, false)
bool SkShouldPostMessageToBus(const GrClientMappedBufferManager::BufferFinishedMessage& m,
uint32_t msgBusUniqueID) {
diff --git a/src/gpu/GrClientMappedBufferManager.h b/src/gpu/GrClientMappedBufferManager.h
index 3f3b067..20ef457 100644
--- a/src/gpu/GrClientMappedBufferManager.h
+++ b/src/gpu/GrClientMappedBufferManager.h
@@ -29,10 +29,17 @@
* Set fInboxID to inboxID(). fBuffer must have been previously passed to insert().
*/
struct BufferFinishedMessage {
+ BufferFinishedMessage(sk_sp<GrGpuBuffer> buffer, uint32_t indexId)
+ : fBuffer(std::move(buffer)), fInboxID(indexId) {}
+ BufferFinishedMessage(BufferFinishedMessage&& other) {
+ fBuffer = std::move(other.fBuffer);
+ fInboxID = other.fInboxID;
+ other.fInboxID = 0;
+ }
sk_sp<GrGpuBuffer> fBuffer;
uint32_t fInboxID;
};
- using BufferFinishedMessageBus = SkMessageBus<BufferFinishedMessage>;
+ using BufferFinishedMessageBus = SkMessageBus<BufferFinishedMessage, false>;
GrClientMappedBufferManager(uint32_t contextID);
GrClientMappedBufferManager(const GrClientMappedBufferManager&) = delete;
diff --git a/src/gpu/GrResourceCache.cpp b/src/gpu/GrResourceCache.cpp
index 82ec5b5..f0b09b9 100644
--- a/src/gpu/GrResourceCache.cpp
+++ b/src/gpu/GrResourceCache.cpp
@@ -25,9 +25,9 @@
#include "src/gpu/GrTracing.h"
#include "src/gpu/SkGr.h"
-DECLARE_SKMESSAGEBUS_MESSAGE(GrUniqueKeyInvalidatedMessage);
+DECLARE_SKMESSAGEBUS_MESSAGE(GrUniqueKeyInvalidatedMessage, true);
-DECLARE_SKMESSAGEBUS_MESSAGE(GrTextureFreedMessage);
+DECLARE_SKMESSAGEBUS_MESSAGE(GrTextureFreedMessage, true);
#define ASSERT_SINGLE_OWNER GR_ASSERT_SINGLE_OWNER(fSingleOwner)
diff --git a/src/gpu/ccpr/GrCCPathCache.cpp b/src/gpu/ccpr/GrCCPathCache.cpp
index 9a537a7..18c4e7f 100644
--- a/src/gpu/ccpr/GrCCPathCache.cpp
+++ b/src/gpu/ccpr/GrCCPathCache.cpp
@@ -13,7 +13,7 @@
static constexpr int kMaxKeyDataCountU32 = 256; // 1kB of uint32_t's.
-DECLARE_SKMESSAGEBUS_MESSAGE(sk_sp<GrCCPathCache::Key>);
+DECLARE_SKMESSAGEBUS_MESSAGE(sk_sp<GrCCPathCache::Key>, true);
static inline uint32_t next_path_cache_id() {
static std::atomic<uint32_t> gNextID(1);
diff --git a/src/gpu/text/GrTextBlobCache.cpp b/src/gpu/text/GrTextBlobCache.cpp
index d11a7f3..de94501 100644
--- a/src/gpu/text/GrTextBlobCache.cpp
+++ b/src/gpu/text/GrTextBlobCache.cpp
@@ -7,7 +7,7 @@
#include "src/gpu/text/GrTextBlobCache.h"
-DECLARE_SKMESSAGEBUS_MESSAGE(GrTextBlobCache::PurgeBlobMessage)
+DECLARE_SKMESSAGEBUS_MESSAGE(GrTextBlobCache::PurgeBlobMessage, true)
// This function is captured by the above macro using implementations from SkMessageBus.h
static inline bool SkShouldPostMessageToBus(
diff --git a/tests/MessageBusTest.cpp b/tests/MessageBusTest.cpp
index bc8c202..7d15e5d 100644
--- a/tests/MessageBusTest.cpp
+++ b/tests/MessageBusTest.cpp
@@ -22,17 +22,19 @@
}
} // namespace
-DECLARE_SKMESSAGEBUS_MESSAGE(TestMessage)
+
+DECLARE_SKMESSAGEBUS_MESSAGE(TestMessage, true)
DEF_TEST(MessageBus, r) {
+ using TestMessageBus = SkMessageBus<TestMessage>;
// Register two inboxes to receive all TestMessages.
- SkMessageBus<TestMessage>::Inbox inbox1, inbox2;
+ TestMessageBus::Inbox inbox1, inbox2;
// Send two messages.
const TestMessage m1 = { 5, 4.2f };
const TestMessage m2 = { 6, 4.3f };
- SkMessageBus<TestMessage>::Post(m1);
- SkMessageBus<TestMessage>::Post(m2);
+ TestMessageBus::Post(std::move(m1));
+ TestMessageBus::Post(std::move(m2));
// Make sure we got two.
SkTArray<TestMessage> messages;
@@ -43,7 +45,7 @@
// Send another; check we get just that one.
const TestMessage m3 = { 1, 0.3f };
- SkMessageBus<TestMessage>::Post(m3);
+ TestMessageBus::Post(m3);
inbox1.poll(&messages);
REPORTER_ASSERT(r, 1 == messages.count());
REPORTER_ASSERT(r, 1 == messages[0].x);
@@ -62,6 +64,60 @@
namespace {
+struct TestMessageRefCnt : public SkRefCnt {
+ TestMessageRefCnt(int i, float f) : x(i), y(f) {}
+
+ int x;
+ float y;
+};
+
+static inline bool SkShouldPostMessageToBus(const sk_sp<TestMessageRefCnt>&, uint32_t) {
+ return true;
+}
+
+} // namespace
+
+DECLARE_SKMESSAGEBUS_MESSAGE(sk_sp<TestMessageRefCnt>, false)
+
+DEF_TEST(MessageBusSp, r) {
+ // Register two inboxes to receive all TestMessages.
+ using TestMessageBus = SkMessageBus<sk_sp<TestMessageRefCnt>, false>;
+ TestMessageBus::Inbox inbox1;
+
+ // Send two messages.
+ auto m1 = sk_make_sp<TestMessageRefCnt>(5, 4.2f);
+ auto m2 = sk_make_sp<TestMessageRefCnt>(6, 4.3f);
+ TestMessageBus::Post(std::move(m1));
+ TestMessageBus::Post(std::move(m2));
+
+ // Make sure we got two.
+ SkTArray<sk_sp<TestMessageRefCnt>> messages;
+ inbox1.poll(&messages);
+ REPORTER_ASSERT(r, 2 == messages.count());
+ REPORTER_ASSERT(r, messages[0]->unique());
+ REPORTER_ASSERT(r, messages[1]->unique());
+ REPORTER_ASSERT(r, 5 == messages[0]->x);
+ REPORTER_ASSERT(r, 6 == messages[1]->x);
+
+ // Send another; check we get just that one.
+ auto m3 = sk_make_sp<TestMessageRefCnt>(1, 0.3f);
+ TestMessageBus::Post(std::move(m3));
+ inbox1.poll(&messages);
+ REPORTER_ASSERT(r, 1 == messages.count());
+ REPORTER_ASSERT(r, messages[0]->unique());
+ REPORTER_ASSERT(r, 1 == messages[0]->x);
+
+ // Send another without std::move(), it should trigger SkASSERT().
+ // auto m4 = sk_make_sp<TestMessageRefCnt>(1, 0.3f);
+ // TestMessageBus::Post(m4);
+
+ // Nothing was sent since the last read.
+ inbox1.poll(&messages);
+ REPORTER_ASSERT(r, 0 == messages.count());
+}
+
+namespace {
+
struct AddressedMessage {
uint32_t fInboxID;
};
@@ -76,15 +132,16 @@
} // namespace
-DECLARE_SKMESSAGEBUS_MESSAGE(AddressedMessage)
+DECLARE_SKMESSAGEBUS_MESSAGE(AddressedMessage, true)
DEF_TEST(MessageBus_SkShouldPostMessageToBus, r) {
- SkMessageBus<AddressedMessage>::Inbox inbox1(1), inbox2(2);
+ using AddressedMessageBus = SkMessageBus<AddressedMessage>;
+ AddressedMessageBus::Inbox inbox1(1), inbox2(2);
- SkMessageBus<AddressedMessage>::Post({0}); // Should go to both
- SkMessageBus<AddressedMessage>::Post({1}); // Should go to inbox1
- SkMessageBus<AddressedMessage>::Post({2}); // Should go to inbox2
- SkMessageBus<AddressedMessage>::Post({3}); // Should go nowhere
+ AddressedMessageBus::Post({0}); // Should go to both
+ AddressedMessageBus::Post({1}); // Should go to inbox1
+ AddressedMessageBus::Post({2}); // Should go to inbox2
+ AddressedMessageBus::Post({3}); // Should go nowhere
SkTArray<AddressedMessage> messages;
inbox1.poll(&messages);