Fixes for SkRWBuffer

Do not call SkBufferHead::validate in SkROBuffer's destructor, which
may be called in a separate thread from SkRWBuffer::append. validate()
reads SkBufferBlock::fUsed, and append() writes to it, resulting in
a data race.

Update some comments to be more clear about how it is safe to use
these classes across threads.

Test the readers in separate threads.

In addition, make sure it is safe to create a reader even when no
data has been appended. Add tests for this case.

Mark a parameter to SkBufferHead::validate() as const, reflecting
its use.

BUG=chromium:601578
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1871953002

Review URL: https://codereview.chromium.org/1871953002

1 files changed, 37 insertions, 3 deletions

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);
+    }
 }