From a3760992c93ddb5512d96671831576907441605d Mon Sep 17 00:00:00 2001 From: bungeman Date: Thu, 14 Apr 2016 14:57:01 -0700 Subject: Revert of Fixes for SkRWBuffer (patchset #5 id:80001 of https://codereview.chromium.org/1871953002/ ) Reason for revert: Making MSAN and TSAN rather unhappy. https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-MSAN/builds/1586 https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-TSAN/builds/5922 Original issue's description: > 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 > > Committed: https://skia.googlesource.com/skia/+/d06920a29fe11c68bde2b93948ec99f277bb8459 TBR=mtklein@google.com,reed@google.com,scroggo@google.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:601578 Review URL: https://codereview.chromium.org/1882853004 --- tests/DataRefTest.cpp | 40 +++------------------------------------- 1 file changed, 3 insertions(+), 37 deletions(-) (limited to 'tests/DataRefTest.cpp') diff --git a/tests/DataRefTest.cpp b/tests/DataRefTest.cpp index 4bfbff4638..002abcbe89 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 contain an integral number of copies of gABC. +// stream should contains 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,8 +284,6 @@ 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. @@ -294,31 +292,17 @@ 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(); @@ -341,27 +325,9 @@ DEF_TEST(RWBuffer_size, r) { REPORTER_ASSERT(r, 0 == iter.size()); } -// Tests that operations (including the destructor) are safe on an SkRWBuffer -// without any data appended. +// Tests that it is safe to destruct an SkRWBuffer without appending +// anything to it. DEF_TEST(RWBuffer_noAppend, r) { SkRWBuffer buffer; REPORTER_ASSERT(r, 0 == buffer.size()); - - sk_sp roBuffer = sk_sp(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 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); - } } -- cgit v1.2.3