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..7f919c62fdfe3f7618dcca57860665584f36de6e 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);
     if (!SkImageDecoder::DecodeFile(baselinePath, &baselineBitmap)) {
         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,10 @@ void SkDiffContext::diffPatterns(const char baselinePattern[], const char testPa
 }
 
 void SkDiffContext::outputRecords(SkWStream& stream, bool useJSONP) {
-    DiffRecord* currentRecord = fRecords;
+    SkAutoMutexAcquire recordsLock(fRecordMutex);

[scroggo, 2013/11/05 21:08:52]

I don't know this whole program, but I assume that addDiffs needs to be
protected by a mutex because it is called from multiple threads simultaneously.

Will addDiffs be called while outputRecords is running? It seems like if it is,
some records may not be output. If not, do we still need the mutex?

[djsollen, 2013/11/05 21:24:30]

Your right I don't need to try to make the whole class thread safe, just
addDiffs.  As a result I was able to remove the locks from the output methods.

+    SkTLList<DiffRecord>::Iter iter(fRecords, SkTLList<DiffRecord>::Iter::kHead_IterStart);
+    DiffRecord* currentRecord = iter.get();
+
     if (useJSONP) {
         stream.writeText("var SkPDiffRecords = {\n");
     } else {
@@ -270,12 +264,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 +286,9 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
 
     stream.writeText("key");
 
-    DiffRecord* currentRecord = fRecords;
+    SkAutoMutexAcquire recordsLock(fRecordMutex);

[scroggo, 2013/11/05 21:08:52]

Again, do we need the mutex here?

[djsollen, 2013/11/05 21:24:30]

Done.

+    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 +301,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 +338,6 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
         }
         stream.writeText("\n");
 
-        currentRecord = currentRecord->fNext;
+        currentRecord = iter2.next();
     }
 }