Fix race at blink::GCInfoAtBase<T>::index

Fix race at blink::GCInfoAtBase<T>::index

DEFINE_STATIC_LOCAL is not thread safe and
blink::GCInfoAtBase<T>::index need to be called on multiple threads.
This causes race condition. This cl changes the struct to use a
function pointer instead of the string itself. The string gets
generated only when user asks for the class name.

BUG=519999, 490087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200451

[ssid, 2015-08-12 09:29:34]

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

[ssid, 2015-08-12 09:29:35]

Please see if this fix makes sense. I couldn't reproduce the race. Thanks

[ssid, 2015-08-12 09:43:59]

+tkonchada

[haraken, 2015-08-12 12:33:41]

https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
Source/platform/heap/Visitor.h:394: ASSERT(isMainThread());

It is not guaranteed that get() is called only by the main thread. It is called
by any GCing thread, which is not necessarily the main thread.

Can we use AtomicallyInitializedStaticReference so that non-main threads can
call get() safely?

[ssid, 2015-08-12 12:54:56]

On 2015/08/12 12:33:41, haraken wrote:
>
https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
> File Source/platform/heap/Visitor.h (right):
> 
>
https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
> Source/platform/heap/Visitor.h:394: ASSERT(isMainThread());
> 
> It is not guaranteed that get() is called only by the main thread. It is
called
> by any GCing thread, which is not necessarily the main thread.
> 
> Can we use AtomicallyInitializedStaticReference so that non-main threads can
> call get() safely?

Since this is called only in gc, is it fine if I added assert(InGC) instead of
main thread? This will still be called only in one thread if gc is happening.

[ssid, 2015-08-12 12:56:25]

On 2015/08/12 12:54:56, ssid wrote:
> On 2015/08/12 12:33:41, haraken wrote:
> >
>
https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
> > File Source/platform/heap/Visitor.h (right):
> > 
> >
>
https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
> > Source/platform/heap/Visitor.h:394: ASSERT(isMainThread());
> > 
> > It is not guaranteed that get() is called only by the main thread. It is
> called
> > by any GCing thread, which is not necessarily the main thread.
> > 
> > Can we use AtomicallyInitializedStaticReference so that non-main threads can
> > call get() safely?
> 
> Since this is called only in gc, is it fine if I added assert(InGC) instead of
> main thread? This will still be called only in one thread if gc is happening.

Basically I am trying to avoid locks for getting the names.

[haraken, 2015-08-12 13:09:57]

On 2015/08/12 12:56:25, ssid wrote:
> On 2015/08/12 12:54:56, ssid wrote:
> > On 2015/08/12 12:33:41, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
> > > File Source/platform/heap/Visitor.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1287963002/diff/40001/Source/platform/heap/Vi...
> > > Source/platform/heap/Visitor.h:394: ASSERT(isMainThread());
> > > 
> > > It is not guaranteed that get() is called only by the main thread. It is
> > called
> > > by any GCing thread, which is not necessarily the main thread.
> > > 
> > > Can we use AtomicallyInitializedStaticReference so that non-main threads
can
> > > call get() safely?
> > 
> > Since this is called only in gc, is it fine if I added assert(InGC) instead
of
> > main thread? This will still be called only in one thread if gc is
happening.
> 
> Basically I am trying to avoid locks for getting the names.

Sounds reasonable.

[ssid, 2015-08-12 17:04:36]

It was complicated change just for an assert, since I cannot get the correct
ThreadState object that calls the className() method.

Every time postGC is called the gcState is reset for the ThreadState.
ThreadState::current() would not work because ThreadState object can perform
postGC after the "GC thread"'s object does postGC.
I could not move the setting gcState to heap.cpp since I cannot reset the
gcState for different thread from the "GC thread".
So, I have no way to obtain ThreadState that is calling the get(), unless I pass
the ThreadState object to get() or move the get() method as ThreadState member.
Both these options seem dirty.

Though the get() right now is accessed in thread safe way, I am still making it
atomic since in future some other user would not know about the thread safety.
If you prefer anything other way to just do this check ,please tell me. Else,
look at the updated patch.

Thanks,
Sid.

[haraken, 2015-08-12 23:36:06]

On 2015/08/12 17:04:36, ssid wrote:
> It was complicated change just for an assert, since I cannot get the correct
> ThreadState object that calls the className() method.
> 
> Every time postGC is called the gcState is reset for the ThreadState.
> ThreadState::current() would not work because ThreadState object can perform
> postGC after the "GC thread"'s object does postGC.
> I could not move the setting gcState to heap.cpp since I cannot reset the
> gcState for different thread from the "GC thread".
> So, I have no way to obtain ThreadState that is calling the get(), unless I
pass
> the ThreadState object to get() or move the get() method as ThreadState
member.
> Both these options seem dirty.
> 
> Though the get() right now is accessed in thread safe way, I am still making
it
> atomic since in future some other user would not know about the thread safety.
> If you prefer anything other way to just do this check ,please tell me. Else,
> look at the updated patch.
> 
> Thanks,
> Sid.

I'm fine with the current CL if you add a comment about the thread safety.

LGTM.

[ssid, 2015-08-12 23:54:38]

Patchset #5 (id:80001) has been deleted

[ssid, 2015-08-13 01:03:13]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-08-13 01:03:13]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1287963002/#ps120001
(title: "Nits.")

[commit-bot: I haz the power, 2015-08-13 01:03:22]

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

[commit-bot: I haz the power, 2015-08-13 02:32:03]

Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200451