Oilpan: fix tracing of un-initialized part objects during conservative GCs.

Oilpan: fix tracing of un-initialized part objects during conservative GCs.

If a part object has a virtual trace method we need to check the vtable
before calling the trace method. This is necessary because conservative
GCs can see the part object before it has been constructed (if there
is an allocation during the construction of the containing object.)

Added simple regression test that illustrates the issue.

R=erik.corry@gmail.com, zerny@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180335

[Mads Ager (chromium), 2014-08-11 10:26:06]

+oilpan-reviews

[zerny-chromium, 2014-08-11 10:42:06]

lgtm, under the assumption that Polymorphic<T>::value => is_virtual(T::trace).

I'll make a plugin CL to verify this and a few other virtual-trace requirements
that we have discussed offline.

[Erik Corry, 2014-08-11 10:43:15]

LGTM, but please don't land before the plugin change is done.

[haraken, 2014-08-11 10:44:02]

https://codereview.chromium.org/455363002/diff/1/Source/platform/heap/Visitor.h
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/455363002/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:294: if (!vtable)

Can we merge this check with vTableInitialized() in Heap.cpp?

(I thought that the vTableInitialized() check in
HeapPage<Header>::checkAndMarkPointer was enough.)

[Mads Ager (chromium), 2014-08-11 11:00:26]

On 2014/08/11 10:43:15, Erik Corry wrote:
> LGTM, but please don't land before the plugin change is done.

Ian is going to work on those now, so hopefully we can land this soon with a
plugin change that ensures that the first pointer is  the right vtable to check.

[Mads Ager (chromium), 2014-08-11 12:17:36]

Thanks for the comments! I will not commit this until Ian has  the plugin side
change ready and we have verified our assumptions using it.

I updated the code here based on the inconsistent handling that Haraken's
comment points out. PTAL

https://codereview.chromium.org/455363002/diff/1/Source/platform/heap/Visitor.h
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/455363002/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:294: if (!vtable)
On 2014/08/11 10:44:02, haraken wrote:
> 
> Can we merge this check with vTableInitialized() in Heap.cpp?
> 
> (I thought that the vTableInitialized() check in
> HeapPage<Header>::checkAndMarkPointer was enough.)

These are two different checks:

The check that we already have is for the case where an argument to a
constructor allocates. In that case we can see a pointer on the stack that
points to a garbage collected object that should have a virtual table but it has
not yet been initialized.

The check that I'm adding here is for part objects that have virtual methods but
where the vtable is not yet initialized because the construction of the part
object hasn't started yet.

We are performing conservative scanning of the object reachable from the stack.
We shouldn't have to (when Ian has updated the plugin to make sure that the
first word is actually a vtable). I'll remove the conservative scanning on the
stack so that we just ignore the object in both cases (and maybe add asserts
that the object pointed to on the stack is completely uninitialized).

[haraken, 2014-08-11 12:37:21]

On 2014/08/11 12:17:36, Mads Ager (chromium) wrote:
> Thanks for the comments! I will not commit this until Ian has  the plugin side
> change ready and we have verified our assumptions using it.
> 
> I updated the code here based on the inconsistent handling that Haraken's
> comment points out. PTAL
> 
>
https://codereview.chromium.org/455363002/diff/1/Source/platform/heap/Visitor.h
> File Source/platform/heap/Visitor.h (right):
> 
>
https://codereview.chromium.org/455363002/diff/1/Source/platform/heap/Visitor...
> Source/platform/heap/Visitor.h:294: if (!vtable)
> On 2014/08/11 10:44:02, haraken wrote:
> > 
> > Can we merge this check with vTableInitialized() in Heap.cpp?
> > 
> > (I thought that the vTableInitialized() check in
> > HeapPage<Header>::checkAndMarkPointer was enough.)
> 
> These are two different checks:
> 
> The check that we already have is for the case where an argument to a
> constructor allocates. In that case we can see a pointer on the stack that
> points to a garbage collected object that should have a virtual table but it
has
> not yet been initialized.
> 
> The check that I'm adding here is for part objects that have virtual methods
but
> where the vtable is not yet initialized because the construction of the part
> object hasn't started yet.
> 
> We are performing conservative scanning of the object reachable from the
stack.
> We shouldn't have to (when Ian has updated the plugin to make sure that the
> first word is actually a vtable). I'll remove the conservative scanning on the
> stack so that we just ignore the object in both cases (and maybe add asserts
> that the object pointed to on the stack is completely uninitialized).

Thanks for the clarification, makes sense. LGTM.

[Mads Ager (chromium), 2014-08-15 06:16:14]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-08-15 06:16:34]

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

[commit-bot: I haz the power, 2014-08-15 07:29:05]

Committed patchset #3 (40001) as 180335