Add lock to SkPDFDict

src/pdf/SkPDFTypes.cpp

 
 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #include "SkPDFCatalog.h"
@@ skipping 384 matching lines @@
 SkPDFDict::~SkPDFDict() {
     clear();
 }
 
 void SkPDFDict::emitObject(SkWStream* stream, SkPDFCatalog* catalog,
                            bool indirect) {
     if (indirect) {
         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);
         stream->writeText("\n");
     }
     stream->writeText(">>");
 }
 
 size_t SkPDFDict::getOutputSize(SkPDFCatalog* catalog, bool indirect) {
     if (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;
 }