Make |Heap::findGCInfo()| threadsafe.

Make |Heap::findGCInfo()| threadsafe.

Before this patch, |Heap::findGCInfo()|'s iteration on
AttachedThreadStateSet was not guarded. This triggered "modification
while iteration" assert when |Heap::findGCInfo()| was called from
non-safepoint.

This patch exposes |ThreadState::threadAttachMutex()| and
|Heap::findGCInfo()| will acquire lock on it when we are not in safepoint.

Unfortunately there are no trybot which has an ENABLE(GC_TRACING) flag.
NOTRY=true
TEST=content_shell --dump-render-tree http/tests/workers/terminate-during-sync-operation-websql.html wont crash.
BUG=380474
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175774

[haraken, 2014-06-05 09:11:23]

https://codereview.chromium.org/311263008/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/311263008/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1603: bool needLockForIteration =
!ThreadState::current()->isAtSafePoint();

I guess what matters is not whether we're at a safe point but whether we're in a
GC. If so, we should check ThreadState::isAnyThreadInGC() instead of
isAtSafePoint().

[kouhei (in TOK), 2014-06-05 09:26:21]

https://codereview.chromium.org/311263008/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/311263008/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1603: bool needLockForIteration =
!ThreadState::current()->isAtSafePoint();
On 2014/06/05 09:11:23, haraken wrote:
> 
> I guess what matters is not whether we're at a safe point but whether we're in
a
> GC. If so, we should check ThreadState::isAnyThreadInGC() instead of
> isAtSafePoint().

Done.

[haraken, 2014-06-05 09:31:18]

LGTM.

In my understanding, this change won't cause dead-lock, but I want to have
Denmark guys confirm it.

[Mads Ager (chromium), 2014-06-06 06:54:01]

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1611: ThreadState::threadAttachMutex().unlock();
Do we have a scoped lock object that we could use instead of having to
explicitly unlock at all returns? MutexLocker?

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:507: static Mutex& threadAttachMutex();
Instead of exposing this, can you move the code that does the iteration to
ThreadState?

Heap::findGCInfo(...)
{
  ...
  return ThreadState::findGCInfo();
}

?

[kouhei (in TOK), 2014-06-06 06:58:26]

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1611: ThreadState::threadAttachMutex().unlock();
On 2014/06/06 06:54:01, Mads Ager (chromium) wrote:
> Do we have a scoped lock object that we could use instead of having to
> explicitly unlock at all returns? MutexLocker?

I tried to use MutexLocker, but it requires lock at the c-tor, so it can't be
used for optional locks. I'll try to modify it to support use case like this.

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:507: static Mutex& threadAttachMutex();
On 2014/06/06 06:54:01, Mads Ager (chromium) wrote:
> Instead of exposing this, can you move the code that does the iteration to
> ThreadState?
> 
> Heap::findGCInfo(...)
> {
>   ...
>   return ThreadState::findGCInfo();
> }
> 
> ?
Ack.

[Mads Ager (chromium), 2014-06-06 07:03:30]

LGTM with the actual code moved to ThreadState.

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/311263008/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1611: ThreadState::threadAttachMutex().unlock();
On 2014/06/06 06:58:26, kouhei wrote:
> On 2014/06/06 06:54:01, Mads Ager (chromium) wrote:
> > Do we have a scoped lock object that we could use instead of having to
> > explicitly unlock at all returns? MutexLocker?
> 
> I tried to use MutexLocker, but it requires lock at the c-tor, so it can't be
> used for optional locks. I'll try to modify it to support use case like this.

Nah, don't bother for this change. This code is never going to be on by default
and it is correct as it is. Was just wondering if we had it already. :)

[kouhei (in TOK), 2014-06-09 05:21:50]

On 2014/06/06 07:03:30, Mads Ager (chromium) wrote:
> LGTM with the actual code moved to ThreadState.
Done.

[kouhei (in TOK), 2014-06-09 05:21:52]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-09 05:22:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/311263008/40001

[commit-bot: I haz the power, 2014-06-09 05:22:35]

Change committed as 175774