Avoid data races on initializing GCScope.

Avoid data races on initializing GCScope.

Fully initialize the stack-allocated GCScope object before having it
enter a safe point, and attempt to have all the attached threads do
the same ("parking" them.)

By arranging for all updates to the GCScope object to happen before
safe points are entered, we avoid overlapping r/w access of the
thread stack regions that marking will proceed to conservatively scan
for heap references. Such overlaps can happen if two threads concurrently
attempt to initiate a GC.

R=haraken
BUG=527338
Committed: https://crrev.com/ce4a22fd3ac69b9648e14d2c91c75ed4c14e63f2
Cr-Commit-Position: refs/heads/master@{#358573}

[sof, 2015-11-09 05:57:31]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-11-09 05:57:31]

please take a look.

[haraken, 2015-11-09 06:20:11]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-11-09 06:20:12]

Thanks for looking into this! (I won't have time to write this CL this week...)

Mostly looks good.

https://codereview.chromium.org/1411643006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1411643006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.cpp:80: ,
m_safePointScope(stackState, gcType != BlinkGC::ThreadTerminationGC ? state :
nullptr, true)

It seems that it's no longer worth using a SafePoint"Scope" (since we need to
delay entering the safe point). Can we just directly call
state->enterSafePoint() and state->leaveSafePoint()?

[sof, 2015-11-09 06:33:56]

https://codereview.chromium.org/1411643006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1411643006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.cpp:80: ,
m_safePointScope(stackState, gcType != BlinkGC::ThreadTerminationGC ? state :
nullptr, true)
On 2015/11/09 06:20:11, haraken wrote:
> 
> It seems that it's no longer worth using a SafePoint"Scope" (since we need to
> delay entering the safe point). Can we just directly call
> state->enterSafePoint() and state->leaveSafePoint()?
> 
> 

That's better, delineating the two scope objects. Done.

[haraken, 2015-11-09 06:35:53]

LGTM

[sof, 2015-11-09 06:40:53]

Description was changed from

==========
Avoid data races on initializing GCScope.

Fully initialize the stack-allocated GCScope object before having it
enter a safe point, and attempt to have all the attached threads do
the same ("parking" them.)

By arranging for all updates to the GCScope object to happen before
safe points are entered, we avoid overlapping r/w access of the
thread stack regions that marking will proceed to conservatively scan
for heap references. Such overlaps can happen if two threads concurrently
attempt to initiate a GC.

R=
BUG=527338
==========

to

==========
Avoid data races on initializing GCScope.

Fully initialize the stack-allocated GCScope object before having it
enter a safe point, and attempt to have all the attached threads do
the same ("parking" them.)

By arranging for all updates to the GCScope object to happen before
safe points are entered, we avoid overlapping r/w access of the
thread stack regions that marking will proceed to conservatively scan
for heap references. Such overlaps can happen if two threads concurrently
attempt to initiate a GC.

R=haraken
BUG=527338
==========

[sof, 2015-11-09 06:41:04]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-09 06:41:09]

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

[commit-bot: I haz the power, 2015-11-09 09:04:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-09 09:05:14]

Patchset 3 (id:??) landed as
https://crrev.com/ce4a22fd3ac69b9648e14d2c91c75ed4c14e63f2
Cr-Commit-Position: refs/heads/master@{#358573}