3 files changed, 44 insertions, 8 deletions

diff --git a/include/core/SkRWBuffer.h b/include/core/SkRWBuffer.h
index d054e0a5fd..ff807766d0 100644
--- a/include/core/SkRWBuffer.h
+++ b/include/core/SkRWBuffer.h
@@ -67,8 +67,9 @@ private:
 
 /**
  *  Accumulates bytes of memory that are "appended" to it, growing internal storage as needed.
- *  The growth is done such that at any time, a RBuffer or StreamAsset can be snapped off, which
- *  can see the previously stored bytes, but which will be unaware of any future writes.
+ *  The growth is done such that at any time in the writer's thread, an RBuffer or StreamAsset
+ *  can be snapped off (and safely passed to another thread). The RBuffer/StreamAsset snapshot
+ *  can see the previously stored bytes, but will be unaware of any future writes.
  */
 class SK_API SkRWBuffer {
 public:
diff --git a/src/core/SkRWBuffer.cpp b/src/core/SkRWBuffer.cpp
index f0b565edf3..bbf88ddfe3 100644
--- a/src/core/SkRWBuffer.cpp
+++ b/src/core/SkRWBuffer.cpp
@@ -42,6 +42,8 @@ struct SkBufferBlock {
         return amount;
     }
 
+    // Do not call in the reader thread, since the writer may be updating fUsed.
+    // (The assertion is still true, but TSAN still may complain about its raciness.)
     void validate() const {
 #ifdef SK_DEBUG
         SkASSERT(fCapacity > 0);
@@ -94,7 +96,7 @@ struct SkBufferHead {
         }
     }
 
-    void validate(size_t minUsed, SkBufferBlock* tail = nullptr) const {
+    void validate(size_t minUsed, const SkBufferBlock* tail = nullptr) const {
 #ifdef SK_DEBUG
         SkASSERT(fRefCnt > 0);
         size_t totalUsed = 0;
@@ -132,7 +134,6 @@ SkROBuffer::SkROBuffer(const SkBufferHead* head, size_t available)
 
 SkROBuffer::~SkROBuffer() {
     if (fHead) {
-        fHead->validate(fAvailable);
         fHead->unref();
     }
 }
@@ -142,7 +143,7 @@ SkROBuffer::Iter::Iter(const SkROBuffer* buffer) {
 }
 
 void SkROBuffer::Iter::reset(const SkROBuffer* buffer) {
-    if (buffer) {
+    if (buffer && buffer->fHead) {
         fBlock = &buffer->fHead->fBlock;
         fRemaining = buffer->fAvailable;
     } else {
diff --git a/tests/DataRefTest.cpp b/tests/DataRefTest.cpp
index 002abcbe89..4bfbff4638 100644
--- a/tests/DataRefTest.cpp
+++ b/tests/DataRefTest.cpp
@@ -245,7 +245,7 @@ static void check_abcs(skiatest::Reporter* reporter, const char buffer[], size_t
     }
 }
 
-// stream should contains an integral number of copies of gABC.
+// stream should contain an integral number of copies of gABC.
 static void check_alphabet_stream(skiatest::Reporter* reporter, SkStream* stream) {
     REPORTER_ASSERT(reporter, stream->hasLength());
     size_t size = stream->getLength();
@@ -284,6 +284,8 @@ static void check_alphabet_buffer(skiatest::Reporter* reporter, const SkROBuffer
     check_abcs(reporter, storage.get(), size);
 }
 
+#include "SkTaskGroup.h"
+
 DEF_TEST(RWBuffer, reporter) {
     // Knowing that the default capacity is 4096, choose N large enough so we force it to use
     // multiple buffers internally.
@@ -292,17 +294,31 @@ DEF_TEST(RWBuffer, reporter) {
     SkStream* streams[N];
 
     {
+        SkTaskGroup tasks;
         SkRWBuffer buffer;
         for (int i = 0; i < N; ++i) {
             buffer.append(gABC, 26);
             readers[i] = buffer.newRBufferSnapshot();
             streams[i] = buffer.newStreamSnapshot();
+            REPORTER_ASSERT(reporter, readers[i]->size() == buffer.size());
+            REPORTER_ASSERT(reporter, streams[i]->getLength() == buffer.size());
+
+            tasks.add([reporter, i, &readers, &streams] {
+                REPORTER_ASSERT(reporter, (i + 1) * 26U == readers[i]->size());
+                REPORTER_ASSERT(reporter, streams[i]->getLength() == readers[i]->size());
+                check_alphabet_buffer(reporter, readers[i]);
+                check_alphabet_stream(reporter, streams[i]);
+                REPORTER_ASSERT(reporter, streams[i]->rewind());
+            });
         }
         REPORTER_ASSERT(reporter, N*26 == buffer.size());
+        tasks.wait();
     }
 
+    // Test again, to verify that the snapshots work even after the SkRWBuffer is gone.
     for (int i = 0; i < N; ++i) {
         REPORTER_ASSERT(reporter, (i + 1) * 26U == readers[i]->size());
+        REPORTER_ASSERT(reporter, streams[i]->getLength() == readers[i]->size());
         check_alphabet_buffer(reporter, readers[i]);
         check_alphabet_stream(reporter, streams[i]);
         readers[i]->unref();
@@ -325,9 +341,27 @@ DEF_TEST(RWBuffer_size, r) {
     REPORTER_ASSERT(r, 0 == iter.size());
 }
 
-// Tests that it is safe to destruct an SkRWBuffer without appending
-// anything to it.
+// Tests that operations (including the destructor) are safe on an SkRWBuffer
+// without any data appended.
 DEF_TEST(RWBuffer_noAppend, r) {
     SkRWBuffer buffer;
     REPORTER_ASSERT(r, 0 == buffer.size());
+
+    sk_sp<SkROBuffer> roBuffer = sk_sp<SkROBuffer>(buffer.newRBufferSnapshot());
+    REPORTER_ASSERT(r, roBuffer);
+    if (roBuffer) {
+        REPORTER_ASSERT(r, roBuffer->size() == 0);
+        SkROBuffer::Iter iter(roBuffer.get());
+        REPORTER_ASSERT(r, iter.size() == 0);
+        REPORTER_ASSERT(r, !iter.data());
+        REPORTER_ASSERT(r, !iter.next());
+    }
+
+    SkAutoTDelete<SkStream> stream(buffer.newStreamSnapshot());
+    REPORTER_ASSERT(r, stream);
+    if (stream) {
+        REPORTER_ASSERT(r, stream->hasLength());
+        REPORTER_ASSERT(r, stream->getLength() == 0);
+        REPORTER_ASSERT(r, stream->skip(10) == 0);
+    }
 }