Fix race condition in SkROBuffer.

Fix race condition in SkROBuffer.

SkBufferBlock::fUsed may be updated by the writer while a reader is
attempting to read it.

BUG=chromium:601578
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1872853002

Committed: https://skia.googlesource.com/skia/+/377add74267e5e0c94521858fb1f9ac5cf299667

[reed1, 2016-04-08 18:02:27]

Description was changed from

==========
alt way to fix race condition

BUG=skia:
==========

to

==========
alt way to fix race condition

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-04-08 18:02:48]

reed@google.com changed reviewers:
+ bungeman@google.com, mtklein@google.com, scroggo@google.com

[reed1, 2016-04-08 18:02:55]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 18:02:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872853002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872853002/1

[commit-bot: I haz the power, 2016-04-08 18:17:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 18:17:29]

Dry run: This issue passed the CQ dry run.

[scroggo, 2016-04-08 18:18:52]

This seems like a fine approach to me. Ben or mtklein, did I miss anything?

https://codereview.chromium.org/1872853002/diff/1/src/core/SkRWBuffer.cpp
File src/core/SkRWBuffer.cpp (right):

https://codereview.chromium.org/1872853002/diff/1/src/core/SkRWBuffer.cpp#new...
src/core/SkRWBuffer.cpp:17: size_t          fCapacity;  // never changes after
the block is created
const? I guess we need a constructor to do that...

[bungeman-skia, 2016-04-08 18:25:34]

Description was changed from

==========
alt way to fix race condition

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix race condition in SkROBuffer.

SkBufferBlock::fUsed may be updated by the writer while a reader is
attempting to read it.

BUG=chromium:601578
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bungeman-skia, 2016-04-08 18:39:29]

https://codereview.chromium.org/1872853002/diff/1/src/core/SkRWBuffer.cpp
File src/core/SkRWBuffer.cpp (right):

https://codereview.chromium.org/1872853002/diff/1/src/core/SkRWBuffer.cpp#new...
src/core/SkRWBuffer.cpp:17: size_t          fCapacity;  // never changes after
the block is created
On 2016/04/08 18:18:52, scroggo wrote:
> const? I guess we need a constructor to do that...

It would actually be nice to write a private constructor and then call placement
new to call it in Alloc below. It would make it more obvious that we're
constructing an SkBufferBlock right there (and then we can mark this const).

[reed1, 2016-04-08 19:18:11]

https://codereview.chromium.org/1872853002/diff/1/src/core/SkRWBuffer.cpp
File src/core/SkRWBuffer.cpp (right):

https://codereview.chromium.org/1872853002/diff/1/src/core/SkRWBuffer.cpp#new...
src/core/SkRWBuffer.cpp:17: size_t          fCapacity;  // never changes after
the block is created
On 2016/04/08 18:39:29, bungeman1 wrote:
> On 2016/04/08 18:18:52, scroggo wrote:
> > const? I guess we need a constructor to do that...
> 
> It would actually be nice to write a private constructor and then call
placement
> new to call it in Alloc below. It would make it more obvious that we're
> constructing an SkBufferBlock right there (and then we can mark this const).

Done.

[reed1, 2016-04-08 19:18:38]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-08 19:18:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872853002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872853002/20001

[reed1, 2016-04-08 19:19:41]

ok to land this, and then build upon it in a 2nd CL, or do you want to merge w/
it directly (or just replicate its idea)?

[reed1, 2016-04-08 19:21:02]

note: this does not address validate() being called from the reader-thread, and
does not try to have Head inherit from SkRefCnt...

[mtklein, 2016-04-08 19:33:11]

On 2016/04/08 at 19:19:41, reed wrote:
> ok to land this, and then build upon it in a 2nd CL, or do you want to merge
w/ it directly (or just replicate its idea)?

(lgtm)

[commit-bot: I haz the power, 2016-04-08 19:35:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-08 19:35:19]

Dry run: This issue passed the CQ dry run.

[scroggo, 2016-04-08 19:44:25]

lgtm

[reed1, 2016-04-08 19:46:09]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-04-08 19:46:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872853002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872853002/20001

[commit-bot: I haz the power, 2016-04-08 19:47:16]

Description was changed from

==========
Fix race condition in SkROBuffer.

SkBufferBlock::fUsed may be updated by the writer while a reader is
attempting to read it.

BUG=chromium:601578
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix race condition in SkROBuffer.

SkBufferBlock::fUsed may be updated by the writer while a reader is
attempting to read it.

BUG=chromium:601578
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/377add74267e5e0c94521858fb1f9ac5cf299667
==========

[commit-bot: I haz the power, 2016-04-08 19:47:17]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/377add74267e5e0c94521858fb1f9ac5cf299667