Add lock to SkPDFDict

Add mutex lock to all functions. Remove dictionary iterator, and
replace with new thread-safe functions.

BUG=skia:2683
R=mtklein@google.com, djsollen@google.com

Author: halcanary@google.com

Review URL: https://codereview.chromium.org/360473005

4 files changed, 84 insertions, 59 deletions

diff --git a/src/pdf/SkPDFStream.cpp b/src/pdf/SkPDFStream.cpp
index e570976403..815ef481ee 100644
--- a/src/pdf/SkPDFStream.cpp
+++ b/src/pdf/SkPDFStream.cpp
@@ -36,15 +36,9 @@ SkPDFStream::SkPDFStream(const SkPDFStream& pdfStream)
         fState = kCompressed_State;
         removeLength = false;
     }
-    SkPDFDict::Iter dict(pdfStream);
-    SkPDFName* key;
-    SkPDFObject* value;
-    SkPDFName lengthName("Length");
-    for (key = dict.next(&value); key != NULL; key = dict.next(&value)) {
-        if (removeLength && *key == lengthName) {
-            continue;
-        }
-        this->insert(key, value);
+    this->mergeFrom(pdfStream);
+    if (removeLength) {
+        this->remove("Length");
     }
 }
 
diff --git a/src/pdf/SkPDFStream.h b/src/pdf/SkPDFStream.h
index 6371bc187d..636ea984e3 100644
--- a/src/pdf/SkPDFStream.h
+++ b/src/pdf/SkPDFStream.h
@@ -35,10 +35,7 @@ public:
     explicit SkPDFStream(SkData* data);
     /** Deprecated constructor. */
     explicit SkPDFStream(SkStream* stream);
-    /** Create a PDF stream with the same content and dictionary entries
-     *  as the passed one.
-     */
-    explicit SkPDFStream(const SkPDFStream& pdfStream);
+
     virtual ~SkPDFStream();
 
     // The SkPDFObject interface.
@@ -54,6 +51,11 @@ protected:
         kCompressed_State,     //!< The stream's already been compressed.
     };
 
+    /** Create a PDF stream with the same content and dictionary entries
+     *  as the passed one.
+     */
+    explicit SkPDFStream(const SkPDFStream& pdfStream);
+
     /* Create a PDF stream with no data.  The setData method must be called to
      * set the data.
      */
diff --git a/src/pdf/SkPDFTypes.cpp b/src/pdf/SkPDFTypes.cpp
index e3cc90c1de..7b79e411b6 100644
--- a/src/pdf/SkPDFTypes.cpp
+++ b/src/pdf/SkPDFTypes.cpp
@@ -396,14 +396,26 @@ SkPDFDict::~SkPDFDict() {
     clear();
 }
 
+int SkPDFDict::size() const {
+    SkAutoMutexAcquire lock(fMutex);
+    return fValue.count();
+}
+
+
 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);
@@ -417,69 +429,84 @@ 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::append(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->append(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->append(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->append(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->append(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->append(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::mergeFrom(const SkPDFDict& other) {
+    SkAutoMutexAcquire lockOther(other.fMutex);
+    SkTDArray<Rec> copy(other.fValue);
+    lockOther.release();  // Do not hold both mutexes at once.
+
+    SkAutoMutexAcquire lock(fMutex);
+    for (int i = 0; i < copy.count(); i++) {
+        *(fValue.append()) = Rec(SkRef(copy[i].key), SkRef(copy[i].value));
     }
-    *value = NULL;
-    return NULL;
 }
diff --git a/src/pdf/SkPDFTypes.h b/src/pdf/SkPDFTypes.h
index 1f06c4c5fe..004c767843 100644
--- a/src/pdf/SkPDFTypes.h
+++ b/src/pdf/SkPDFTypes.h
@@ -369,7 +369,7 @@ public:
 
     /** The size of the dictionary.
      */
-    int size() { return fValue.count(); }
+    int size() const;
 
     /** Add the value to the dictionary with the given key.  Refs value.
      *  @param key   The key for this dictionary entry.
@@ -424,28 +424,30 @@ public:
      */
     void clear();
 
+protected:
+    /** Use to remove a single key from the dictionary.
+     */
+    void remove(const char key[]);
+
+    /** Insert references to all of the key-value pairs from the other
+     *  dictionary into this one.
+     */
+    void mergeFrom(const SkPDFDict& other);
+
 private:
     struct Rec {
-      SkPDFName* key;
-      SkPDFObject* value;
+        SkPDFName* key;
+        SkPDFObject* value;
+        Rec(SkPDFName* k, SkPDFObject* v) : key(k), value(v) {}
     };
 
-public:
-    class Iter {
-    public:
-        explicit Iter(const SkPDFDict& dict);
-        SkPDFName* next(SkPDFObject** value);
-
-    private:
-        const Rec* fIter;
-        const Rec* fStop;
-    };
-
-private:
     static const int kMaxLen = 4095;
 
+    mutable SkMutex fMutex;  // protects modifications to fValue
     SkTDArray<struct Rec> fValue;
 
+    SkPDFObject* append(SkPDFName* key, SkPDFObject* value);
+
     typedef SkPDFObject INHERITED;
 };