Add lock to SkPDFDict

Add lock to SkPDFDict

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

BUG=skia:2683
Committed: https://skia.googlesource.com/skia/+/1f8ed022226c9f960b9fc95af9297d5111a07ead

[hal.canary, 2014-06-27 14:57:57]

ptal

[mtklein, 2014-06-27 15:26:52]

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:39: this->insertDict(pdfStream);
Can either of these become private methods of SkPDFStream instead of protected
on SkPDFDict?

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (right):

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.cpp#newco...
src/pdf/SkPDFTypes.cpp:498: SkAutoMutexAcquire
lockOther(const_cast<SkMutex&>(other.fMutex));
fMutex should probably be mutable

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.cpp#newco...
src/pdf/SkPDFTypes.cpp:499: SkAutoMutexAcquire lock(fMutex);
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.

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h
File src/pdf/SkPDFTypes.h (right):

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode372
src/pdf/SkPDFTypes.h:372: int size() const { return fValue.count(); }
If fMutex guards fValue, why isn't this locked?

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode432
src/pdf/SkPDFTypes.h:432: /** Insert referecnes to all of the key-value pairs
from the other
spelling

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode435
src/pdf/SkPDFTypes.h:435: void insertDict(const SkPDFDict& other);
maybe mergeFrom?

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode449
src/pdf/SkPDFTypes.h:449: SkPDFObject* appendRec(SkPDFName* key, SkPDFObject*
value);
Just my opinion, but the Rec bit here doesn't really add any information. 
append(key, value) seems pretty clear to me.

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

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:39: this->insertDict(pdfStream);
On 2014/06/27 15:26:51, mtklein wrote:
> Can either of these become private methods of SkPDFStream instead
> of protected on SkPDFDict?

I don't see how.  At least this is an improvement over the iterator.

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (right):

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.cpp#newco...
src/pdf/SkPDFTypes.cpp:498: SkAutoMutexAcquire
lockOther(const_cast<SkMutex&>(other.fMutex));
On 2014/06/27 15:26:51, mtklein wrote:
> fMutex should probably be mutable

Done.

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.cpp#newco...
src/pdf/SkPDFTypes.cpp:499: SkAutoMutexAcquire lock(fMutex);
On 2014/06/27 15:26:51, mtklein wrote:
> 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.

Done.

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h
File src/pdf/SkPDFTypes.h (right):

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode372
src/pdf/SkPDFTypes.h:372: int size() const { return fValue.count(); }
On 2014/06/27 15:26:52, mtklein wrote:
> If fMutex guards fValue, why isn't this locked?

Done.

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode432
src/pdf/SkPDFTypes.h:432: /** Insert referecnes to all of the key-value pairs
from the other
On 2014/06/27 15:26:51, mtklein wrote:
> spelling

Done.

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode435
src/pdf/SkPDFTypes.h:435: void insertDict(const SkPDFDict& other);
On 2014/06/27 15:26:51, mtklein wrote:
> maybe mergeFrom?

Done.

https://codereview.chromium.org/360473005/diff/1/src/pdf/SkPDFTypes.h#newcode449
src/pdf/SkPDFTypes.h:449: SkPDFObject* appendRec(SkPDFName* key, SkPDFObject*
value);
On 2014/06/27 15:26:52, mtklein wrote:
> Just my opinion, but the Rec bit here doesn't really add any information. 
> append(key, value) seems pretty clear to me.

Done.

[mtklein, 2014-06-27 17:12:57]

lgtm

[hal.canary, 2014-06-27 17:22:22]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-06-27 17:22:34]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/360473005/20001

[commit-bot: I haz the power, 2014-06-27 17:37:31]

Change committed as 1f8ed022226c9f960b9fc95af9297d5111a07ead