Add lock to SkPDFDict

src/pdf/SkPDFTypes.cpp

Index: src/pdf/SkPDFTypes.cpp
diff --git a/src/pdf/SkPDFTypes.cpp b/src/pdf/SkPDFTypes.cpp
index e3cc90c1de27e711a702e95f8982f42b562d1f76..8521dafcb84114204cc59b4f47b45e623bf91608 100644
--- a/src/pdf/SkPDFTypes.cpp
+++ b/src/pdf/SkPDFTypes.cpp
@@ -402,8 +402,14 @@ void SkPDFDict::emitObject(SkWStream* stream, SkPDFCatalog* catalog,
         return emitIndirectObject(stream, catalog);
     }
 
+    SkAutoMutexAcquire lock(fMutex); // If another thread triggers a
+                                     // resize while this thread is in
+                                     // the for-loop, we can be left
+                                     // with a bad fValue[i] reference.
     stream->writeText("<<");
     for (int i = 0; i < fValue.count(); i++) {
+        SkASSERT(fValue[i].key);
+        SkASSERT(fValue[i].value);
         fValue[i].key->emitObject(stream, catalog, false);
         stream->writeText(" ");
         fValue[i].value->emit(stream, catalog, false);
@@ -417,69 +423,82 @@ size_t SkPDFDict::getOutputSize(SkPDFCatalog* catalog, bool indirect) {
         return getIndirectOutputSize(catalog);
     }
 
+    SkAutoMutexAcquire lock(fMutex); // If another thread triggers a
+                                     // resize while this thread is in
+                                     // the for-loop, we can be left
+                                     // with a bad fValue[i] reference.
     size_t result = strlen("<<>>") + (fValue.count() * 2);
     for (int i = 0; i < fValue.count(); i++) {
+        SkASSERT(fValue[i].key);
+        SkASSERT(fValue[i].value);
         result += fValue[i].key->getOutputSize(catalog, false);
         result += fValue[i].value->getOutputSize(catalog, false);
     }
     return result;
 }
 
-SkPDFObject* SkPDFDict::insert(SkPDFName* key, SkPDFObject* value) {
-    key->ref();
-    value->ref();
-    struct Rec* newEntry = fValue.append();
-    newEntry->key = key;
-    newEntry->value = value;
+SkPDFObject*  SkPDFDict::appendRec(SkPDFName* key, SkPDFObject* value) {
+    SkASSERT(key);
+    SkASSERT(value);
+    SkAutoMutexAcquire lock(fMutex); // If the SkTDArray resizes while
+                                     // two threads access array, one
+                                     // is left with a bad pointer.
+    *(fValue.append()) = Rec(key, value);
     return value;
 }
 
+SkPDFObject* SkPDFDict::insert(SkPDFName* key, SkPDFObject* value) {
+    return this->appendRec(SkRef(key), SkRef(value));
+}
+
 SkPDFObject* SkPDFDict::insert(const char key[], SkPDFObject* value) {
-    value->ref();
-    struct Rec* newEntry = fValue.append();
-    newEntry->key = new SkPDFName(key);
-    newEntry->value = value;
-    return value;
+    return this->appendRec(new SkPDFName(key), SkRef(value));
 }
 
 void SkPDFDict::insertInt(const char key[], int32_t value) {
-    struct Rec* newEntry = fValue.append();
-    newEntry->key = new SkPDFName(key);
-    newEntry->value = new SkPDFInt(value);
+    (void)this->appendRec(new SkPDFName(key), new SkPDFInt(value));
 }
 
 void SkPDFDict::insertScalar(const char key[], SkScalar value) {
-    struct Rec* newEntry = fValue.append();
-    newEntry->key = new SkPDFName(key);
-    newEntry->value = new SkPDFScalar(value);
+    (void)this->appendRec(new SkPDFName(key), new SkPDFScalar(value));
 }
 
 void SkPDFDict::insertName(const char key[], const char name[]) {
-    struct Rec* newEntry = fValue.append();
-    newEntry->key = new SkPDFName(key);
-    newEntry->value = new SkPDFName(name);
+    (void)this->appendRec(new SkPDFName(key), new SkPDFName(name));
 }
 
 void SkPDFDict::clear() {
+    SkAutoMutexAcquire lock(fMutex);
     for (int i = 0; i < fValue.count(); i++) {
+        SkASSERT(fValue[i].key);
+        SkASSERT(fValue[i].value);
         fValue[i].key->unref();
         fValue[i].value->unref();
     }
     fValue.reset();
 }
 
-SkPDFDict::Iter::Iter(const SkPDFDict& dict)
-    : fIter(dict.fValue.begin()),
-      fStop(dict.fValue.end()) {
+void SkPDFDict::remove(const char key[]) {
+    SkASSERT(key);
+    SkPDFName name(key);
+    SkAutoMutexAcquire lock(fMutex);
+    for (int i = 0; i < fValue.count(); i++) {
+        SkASSERT(fValue[i].key);
+        if (*(fValue[i].key) == name) {
+            fValue[i].key->unref();
+            SkASSERT(fValue[i].value);
+            fValue[i].value->unref();
+            fValue.removeShuffle(i);
+            return;
+        }
+    }
 }
 
-SkPDFName* SkPDFDict::Iter::next(SkPDFObject** value) {
-    if (fIter != fStop) {
-        const Rec* cur = fIter;
-        fIter++;
-        *value = cur->value;
-        return cur->key;
+void SkPDFDict::insertDict(const SkPDFDict& other) {
+    SkAutoMutexAcquire lockOther(const_cast<SkMutex&>(other.fMutex));

[mtklein, 2014/06/27 15:26:51]

fMutex should probably be mutable

[hal.canary, 2014/06/27 16:03:27]

Done.

+    SkAutoMutexAcquire lock(fMutex);

[mtklein, 2014/06/27 15:26:51]

Locking more than one lock should terrify you.  We need to prove very carefully
there's no chance of deadlock.  As written, if one thread calls a.insertDict(b)
and another calls b.insertDict(a), we're hung.  As usual, we can be clever about
this, but I'd rather find a way that we don't have to be.

The more you can restrict where insertDict can be called from, the safer we will
be.  If it's private to SkPDFStream and only called from its copy constructor,
or better, inlined into its copy constructor, then there's no need to even lock
fMutex.

The way you've got it here, I can't see any way to locally prove this is safe. 
We'd need to carefully read all code that uses SkPDFDicts every time we want to
reassure ourselves it's safe, and so we'd need to think about these locks every
time we make a change to code that uses SkPDFDicts, even indirectly.  AFAIK,
that's all PDF code.

[hal.canary, 2014/06/27 16:03:27]

Done.

+    for (int i = 0; i < other.fValue.count(); i++) {
+        *(fValue.append())
+            = Rec(SkRef(other.fValue[i].key), SkRef(other.fValue[i].value));
     }
-    *value = NULL;
-    return NULL;
 }