From 1f8ed022226c9f960b9fc95af9297d5111a07ead Mon Sep 17 00:00:00 2001 From: halcanary Date: Fri, 27 Jun 2014 10:37:27 -0700 Subject: 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 --- src/pdf/SkPDFStream.cpp | 12 ++----- src/pdf/SkPDFStream.h | 10 +++--- src/pdf/SkPDFTypes.cpp | 89 ++++++++++++++++++++++++++++++++----------------- src/pdf/SkPDFTypes.h | 32 +++++++++--------- 4 files changed, 84 insertions(+), 59 deletions(-) (limited to 'src/pdf') 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 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 fValue; + SkPDFObject* append(SkPDFName* key, SkPDFObject* value); + typedef SkPDFObject INHERITED; }; -- cgit v1.2.3