fix multithread related crashes in skpdiff

tools/skpdiff/SkDiffContext.cpp

Index: tools/skpdiff/SkDiffContext.cpp
diff --git a/tools/skpdiff/SkDiffContext.cpp b/tools/skpdiff/SkDiffContext.cpp
index 07d304b64cb2e0fac003848c8cfa1e7d2bb80fed..7a1fb955fa0fd5b09e7b00127775e46acdc4afa5 100644
--- a/tools/skpdiff/SkDiffContext.cpp
+++ b/tools/skpdiff/SkDiffContext.cpp
@@ -21,21 +21,12 @@
 static const int kMaxPOI = 100;
 
 SkDiffContext::SkDiffContext() {
-    fRecords = NULL;
     fDiffers = NULL;
     fDifferCount = 0;
     fThreadCount = SkThreadPool::kThreadPerCore;
 }
 
 SkDiffContext::~SkDiffContext() {
-    // Delete the record linked list
-    DiffRecord* currentRecord = fRecords;
-    while (NULL != currentRecord) {
-        DiffRecord* nextRecord = currentRecord->fNext;
-        SkDELETE(currentRecord);
-        currentRecord = nextRecord;
-    }
-
     if (NULL != fDiffers) {
         SkDELETE_ARRAY(fDiffers);
     }
@@ -59,6 +50,7 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
     // Load the images at the paths
     SkBitmap baselineBitmap;
     SkBitmap testBitmap;
+    SkAutoMutexAcquire imageLock(fImageMutex);

[mtklein, 2013/11/06 14:24:36]

Put these in a {} block or just call .lock()?  It's sort of suggesting the wrong
thing to use an SkAutoMutexAcquire then unconditionally call release() later.

[djsollen, 2013/11/06 15:01:58]

The auto acquire is nice as it handles the cleanup in the event that we hit one
of the return statements.

[mtklein, 2013/11/06 15:09:41]

Ah.  Brain fart.  LGTM

     if (!SkImageDecoder::DecodeFile(baselinePath, &baselineBitmap)) {

[mtklein, 2013/11/06 14:24:36]

Does this bottleneck you now?  Is there a way to instantiate your own image
decoders instead of using the globals?

[djsollen, 2013/11/06 15:01:58]

I'm sure it bottlenecks the process a little, but I don't know how much faster
it could be because it crashes when decoding in parallel.  I could spend the
time to estimate what that cost is but I didn't want to add that to this CL,
which is why I filed the other bug.

         SkDebugf("Failed to load bitmap \"%s\"\n", baselinePath);
         return;
@@ -67,13 +59,14 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
         SkDebugf("Failed to load bitmap \"%s\"\n", testPath);
         return;
     }
+    imageLock.release();
 
     // Setup a record for this diff
-    DiffRecord* newRecord = SkNEW(DiffRecord);
+    SkAutoMutexAcquire recordsLock(fRecordMutex);
+    DiffRecord* newRecord = fRecords.addToHead(DiffRecord());
     newRecord->fBaselinePath = baselinePath;
     newRecord->fTestPath = testPath;
-    newRecord->fNext = fRecords;
-    fRecords = newRecord;
+    recordsLock.release();
 
     // Perform each diff
     for (int differIndex = 0; differIndex < fDifferCount; differIndex++) {
@@ -96,8 +89,6 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
             differ->deleteDiff(diffID);
         }
     }
-
-    // if we get a difference and we want the alpha mask then compute it here.
 }
 
 class SkThreadedDiff : public SkRunnable {
@@ -187,7 +178,9 @@ void SkDiffContext::diffPatterns(const char baselinePattern[], const char testPa
 }
 
 void SkDiffContext::outputRecords(SkWStream& stream, bool useJSONP) {
-    DiffRecord* currentRecord = fRecords;
+    SkTLList<DiffRecord>::Iter iter(fRecords, SkTLList<DiffRecord>::Iter::kHead_IterStart);
+    DiffRecord* currentRecord = iter.get();
+
     if (useJSONP) {
         stream.writeText("var SkPDiffRecords = {\n");
     } else {
@@ -270,12 +263,13 @@ void SkDiffContext::outputRecords(SkWStream& stream, bool useJSONP) {
 
         stream.writeText("        }");
 
+        currentRecord = iter.next();
+
         // JSON does not allow trailing commas
-        if (NULL != currentRecord->fNext) {
+        if (NULL != currentRecord) {
             stream.writeText(",");
         }
         stream.writeText("\n");
-        currentRecord = currentRecord->fNext;
     }
     stream.writeText("    ]\n");
     if (useJSONP) {
@@ -291,7 +285,8 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
 
     stream.writeText("key");
 
-    DiffRecord* currentRecord = fRecords;
+    SkTLList<DiffRecord>::Iter iter(fRecords, SkTLList<DiffRecord>::Iter::kHead_IterStart);
+    DiffRecord* currentRecord = iter.get();
 
     // Write CSV header and create a dictionary of all columns.
     while (NULL != currentRecord) {
@@ -304,14 +299,15 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
                 cntColumns++;
             }
         }
-        currentRecord = currentRecord->fNext;
+        currentRecord = iter.next();
     }
     stream.writeText("\n");
 
     double values[100];
     SkASSERT(cntColumns < 100);  // Make the array larger, if we ever have so many diff types.
 
-    currentRecord = fRecords;
+    SkTLList<DiffRecord>::Iter iter2(fRecords, SkTLList<DiffRecord>::Iter::kHead_IterStart);
+    currentRecord = iter2.get();
     while (NULL != currentRecord) {
         for (int i = 0; i < cntColumns; i++) {
             values[i] = -1;
@@ -340,6 +336,6 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
         }
         stream.writeText("\n");
 
-        currentRecord = currentRecord->fNext;
+        currentRecord = iter2.next();
     }
 }