Oilpan: Support nested pre-finalizers

Oilpan: Support nested pre-finalizers

The current problematic constraint of the pre-finalizer is that it is not nestable.
This CL supports nested pre-finalizers like this:

class A : public GarbageCollected<A> {
  USING_PRE_FINALIZER(A, dispose);
  A() { ThreadState::current()->registerPreFinalizer(this); }
  void dispose() { }
};

class B : public GarbageCollectedMixin {
  USING_PRE_FINALIZER(B, dispose);
  B() { ThreadState::current()->registerPreFinalizer(this); }
  void dispose() { }
};

class C : public A, public B {
  USING_PRE_FINALIZER(C, dispose);
  C() { ThreadState::current()->registerPreFinalizer(this); }
  void dispose() { }
};

BUG=420515
TEST=HeapTest.NestedPrefinalizers

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

[haraken, 2015-06-24 07:56:44]

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

[haraken, 2015-06-24 07:56:50]

PTAL

[haraken, 2015-06-24 07:59:27]

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

[haraken, 2015-06-24 07:59:28]

+yutak about plugin issues.

https://codereview.chromium.org/1200333006/diff/1/Source/platform/heap/HeapTe...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1200333006/diff/1/Source/platform/heap/HeapTe...
Source/platform/heap/HeapTest.cpp:1195: class ObservableWithPreFinalizer :
public GarbageCollectedFinalized<ObservableWithPreFinalizer> {

This was a bug -- ObservableWithPreFinalizer should be
GarbageCollected"Finalized". I'm wondering why the GC plugin cannot detect the
error.

https://codereview.chromium.org/1200333006/diff/1/Source/platform/heap/HeapTe...
Source/platform/heap/HeapTest.cpp:1200: DEFINE_INLINE_VIRTUAL_TRACE() { }

Not related to this CL, it took me a couple of hours to notice that this VIRTUAL
is missing. It would be great if the GC plugin could detect the error.

[Raymond Toy, 2015-06-24 11:00:47]

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

[Raymond Toy, 2015-06-24 11:00:47]

lgtm

[sof, 2015-06-24 11:21:57]

https://codereview.chromium.org/1200333006/diff/1/Source/platform/heap/HeapTe...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1200333006/diff/1/Source/platform/heap/HeapTe...
Source/platform/heap/HeapTest.cpp:1200: DEFINE_INLINE_VIRTUAL_TRACE() { }
On 2015/06/24 07:59:27, haraken wrote:
> 
> Not related to this CL, it took me a couple of hours to notice that this
VIRTUAL
> is missing. It would be great if the GC plugin could detect the error.

Please notice that the GC plugin doesn't run inside of platform/heap/ :-)

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/He...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/He...
Source/platform/heap/HeapTest.cpp:3704:
EXPECT_TRUE(ObservableWithPreFinalizer::s_disposeWasCalled);
Could you add (or adapt) a test that checks expected ordering?

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1314: if (!(callbackVector->at(0))(object))
I wonder if it would be tidier to just check if 'object' is unmarked here?

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1317: for (size_t i = 1; i <
callbackVector->size(); i++) {
Don't you want to do this the other way around? the vector will have the
ordering as given by the constructors (base classes first). I think you want to
run the prefinalizers in the other direction.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:100: //     USING_PRE_FINALIZER(Foo);
Sync? The macro (still) takes two arguments.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:566: if (sweepForbidden())
Is this an assertable condition?

[haraken, 2015-06-24 12:54:01]

Thanks for review!

https://codereview.chromium.org/1200333006/diff/1/Source/platform/heap/HeapTe...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1200333006/diff/1/Source/platform/heap/HeapTe...
Source/platform/heap/HeapTest.cpp:1200: DEFINE_INLINE_VIRTUAL_TRACE() { }
On 2015/06/24 11:21:56, sof wrote:
> On 2015/06/24 07:59:27, haraken wrote:
> > 
> > Not related to this CL, it took me a couple of hours to notice that this
> VIRTUAL
> > is missing. It would be great if the GC plugin could detect the error.
> 
> Please notice that the GC plugin doesn't run inside of platform/heap/ :-)

Ah, makes sense; thanks!

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/He...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/He...
Source/platform/heap/HeapTest.cpp:3704:
EXPECT_TRUE(ObservableWithPreFinalizer::s_disposeWasCalled);
On 2015/06/24 11:21:56, sof wrote:
> Could you add (or adapt) a test that checks expected ordering?

Good idea. Done.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1314: if (!(callbackVector->at(0))(object))
On 2015/06/24 11:21:56, sof wrote:
> I wonder if it would be tidier to just check if 'object' is unmarked here?

Unfortunately we can't do this because isHeapObjectIsAlive requires T*.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1317: for (size_t i = 1; i <
callbackVector->size(); i++) {
On 2015/06/24 11:21:56, sof wrote:
> Don't you want to do this the other way around? the vector will have the
> ordering as given by the constructors (base classes first). I think you want
to
> run the prefinalizers in the other direction.

Nice catch! Done.

However, we cannot guarantee the order between Mixin::dispose and
SubClass::dispose, because |this| pointer is different between them and thus
they are registered in a different slot of the hash map of m_preFinalizers.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:100: //     USING_PRE_FINALIZER(Foo);
On 2015/06/24 11:21:56, sof wrote:
> Sync? The macro (still) takes two arguments.

Done.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:100: //     USING_PRE_FINALIZER(Foo);
On 2015/06/24 11:21:56, sof wrote:
> Sync? The macro (still) takes two arguments.

Done.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:566: if (sweepForbidden())
On 2015/06/24 11:21:56, sof wrote:
> Is this an assertable condition?

Done. Previously IDBSomething was unregistering pre-finalziers during
pre-finalizers or destructors. This check was to ignore those unregistrations.
Now that we don't have the use case, I just changed it to ASSERT.

[sof, 2015-06-24 13:40:32]

lgtm.

re: mixins - we'll just have to find out if this is a real problem.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1314: if (!(callbackVector->at(0))(object))
On 2015/06/24 12:54:01, haraken wrote:
> On 2015/06/24 11:21:56, sof wrote:
> > I wonder if it would be tidier to just check if 'object' is unmarked here?
> 
> Unfortunately we can't do this because isHeapObjectIsAlive requires T*.

..and we have to support mixin prefinalizers.

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1317: for (size_t i = 1; i <
callbackVector->size(); i++) {
On 2015/06/24 12:54:01, haraken wrote:
> On 2015/06/24 11:21:56, sof wrote:
> > Don't you want to do this the other way around? the vector will have the
> > ordering as given by the constructors (base classes first). I think you want
> to
> > run the prefinalizers in the other direction.
> 
> Nice catch! Done.
> 
> However, we cannot guarantee the order between Mixin::dispose and
> SubClass::dispose, because |this| pointer is different between them and thus
> they are registered in a different slot of the hash map of m_preFinalizers.
> 

It would have been preferable to avoid that non-determinism -- the worry is that
a mixin instance would somehow touch fields of the mixin when prefinalizing. And
find them disposed of..or not, potentially.

I suppose it wouldn't work out to do a 'this' pointer adjustment to its leftmost
base when registering the prefinalizer either, as the vptr hasn't been
initialized to the mixin instance's vtbl yet (i.e., perform a similar trick to
isHeapObjectAlive().)

[haraken, 2015-06-24 13:47:50]

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1317: for (size_t i = 1; i <
callbackVector->size(); i++) {
On 2015/06/24 13:40:31, sof wrote:
> On 2015/06/24 12:54:01, haraken wrote:
> > On 2015/06/24 11:21:56, sof wrote:
> > > Don't you want to do this the other way around? the vector will have the
> > > ordering as given by the constructors (base classes first). I think you
want
> > to
> > > run the prefinalizers in the other direction.
> > 
> > Nice catch! Done.
> > 
> > However, we cannot guarantee the order between Mixin::dispose and
> > SubClass::dispose, because |this| pointer is different between them and thus
> > they are registered in a different slot of the hash map of m_preFinalizers.
> > 
> 
> It would have been preferable to avoid that non-determinism -- the worry is
that
> a mixin instance would somehow touch fields of the mixin when prefinalizing.
And
> find them disposed of..or not, potentially.
> 
> I suppose it wouldn't work out to do a 'this' pointer adjustment to its
leftmost
> base when registering the prefinalizer either, as the vptr hasn't been
> initialized to the mixin instance's vtbl yet (i.e., perform a similar trick to
> isHeapObjectAlive().)

Maybe can we use a LinkedListHashSet instead of a Vector?

[haraken, 2015-06-24 15:15:10]

On 2015/06/24 13:47:50, haraken wrote:
>
https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1200333006/diff/60001/Source/platform/heap/Th...
> Source/platform/heap/ThreadState.cpp:1317: for (size_t i = 1; i <
> callbackVector->size(); i++) {
> On 2015/06/24 13:40:31, sof wrote:
> > On 2015/06/24 12:54:01, haraken wrote:
> > > On 2015/06/24 11:21:56, sof wrote:
> > > > Don't you want to do this the other way around? the vector will have the
> > > > ordering as given by the constructors (base classes first). I think you
> want
> > > to
> > > > run the prefinalizers in the other direction.
> > > 
> > > Nice catch! Done.
> > > 
> > > However, we cannot guarantee the order between Mixin::dispose and
> > > SubClass::dispose, because |this| pointer is different between them and
thus
> > > they are registered in a different slot of the hash map of
m_preFinalizers.
> > > 
> > 
> > It would have been preferable to avoid that non-determinism -- the worry is
> that
> > a mixin instance would somehow touch fields of the mixin when prefinalizing.
> And
> > find them disposed of..or not, potentially.
> > 
> > I suppose it wouldn't work out to do a 'this' pointer adjustment to its
> leftmost
> > base when registering the prefinalizer either, as the vptr hasn't been
> > initialized to the mixin instance's vtbl yet (i.e., perform a similar trick
to
> > isHeapObjectAlive().)
> 
> Maybe can we use a LinkedListHashSet instead of a Vector?

Sorry I was confused.

If we don't need to worry about the performance of unregisterPreFinalizer
(Currently there is no user of unregisterPreFinalizer), we can just use a link
list.

[haraken, 2015-06-26 04:02:37]

Now it looks safe to land this.

[haraken, 2015-06-26 04:02:41]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-06-26 04:02:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org,
sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1200333006/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-06-26 04:02:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1200333006/100001

[commit-bot: I haz the power, 2015-06-26 04:06:23]

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