Oilpan:Allow custom handling of classes that contain weak pointers that are embedded in collections.

Allow custom handling of classes that contain weak pointers that are embedded in collections.

This is currently tested with HeapHashSet. Extending it to other hash
collections is postponed to a later change. To have your type removed from
collections when weak pointers in it no longer point to live objects you need
to give it two methods and a special hash trait. See WeakHandlingHashTraits in
Heap.h for details and PairWithWeakHandling in HeapTest.cpp for an example.

R=ager@chromium.org, haraken@chromium.org, wibling@chromium.org, zerny@chromium.org
BUG=

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

[Erik Corry, 2014-06-05 05:36:51]

PTAL

[haraken, 2014-06-06 08:03:02]

I'm not confident about reviewing this CL, given the complex templates. I'd be
happy if template experts could take a close look :)

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

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1992: // weak elements.

Is there any collection where we want to allow to have WeakMembers that are not
automatically removed? (We don't allow it in vectors, as you mentioned.) If
there is no such collection, can we add ASSERT_NOT_REACHED()?

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:2181: // non-live pointers, otherwise we would have
dangerous dangling pointers.

This comment is a bit misleading. "otherwise we would have dangerous dangling
pointers" can read like "we would have dangerous dangling pointers if
shouldRemoveFromCollection returns false".

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

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:240: void trace(Member<T>& t)

Just help me understand: Why do we have both const version and non-const version
for trace(T*) and trace(Member<T>&), whereas we have only const version for
trace(WeakMember<T>&), trace(T&) etc?

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:265: // the same constness on their argument to
allow the type to decide.

I agree that we need a const version of trace(const WeakMember<T>&) but won't we
need a non-const version as well? We have both const version and non-const
version for trace(T*) and trace(Member<T>&).

[Erik Corry, 2014-06-06 08:47:36]

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

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1992: // weak elements.
On 2014/06/06 08:03:01, haraken wrote:
> 
> Is there any collection where we want to allow to have WeakMembers that are
not
> automatically removed? (We don't allow it in vectors, as you mentioned.) If
> there is no such collection, can we add ASSERT_NOT_REACHED()?

HeapListHashSet does not support weakness, so I added an assert there.

But this is not just about collections of WeakMembers, it is also about
collections of composite types that contain weak members. In such collections,
if the composite type in the collection does not use WeakHandlingHashTraits or
an equivalent then the WeakMembers inside the composite type will just be zeroed
independent of the collection it is in. That doesn't seem unreasonable, so I
don't want to forbid it.

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:2181: // non-live pointers, otherwise we would have
dangerous dangling pointers.
On 2014/06/06 08:03:01, haraken wrote:
> 
> This comment is a bit misleading. "otherwise we would have dangerous dangling
> pointers" can read like "we would have dangerous dangling pointers if
> shouldRemoveFromCollection returns false".

It is true that if we _incorrectly_ return false from this routine then we will
have dangerous dangling pointers.  I will try to clarify the comment.

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

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:240: void trace(Member<T>& t)
On 2014/06/06 08:03:02, haraken wrote:
> 
> Just help me understand: Why do we have both const version and non-const
version
> for trace(T*) and trace(Member<T>&), whereas we have only const version for
> trace(WeakMember<T>&), trace(T&) etc?

We have to have a const and non-const version for T* because if we only have the
non-const version it doesn't work when the pointer is const at the call site,
and if we only have the const version then the compiler will pick the T& version
rather than the const T* version when given a non-const T*.

For Member<T>& we don't need the non-const version, so I deleted it.

https://codereview.chromium.org/319593004/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:265: // the same constness on their argument to
allow the type to decide.
On 2014/06/06 08:03:02, haraken wrote:
> 
> I agree that we need a const version of trace(const WeakMember<T>&) but won't
we
> need a non-const version as well?

If you have only const methods to choose between then the best match for a
non-const WeakMember<T>& is to convert it to a const WeakMember<T>& and call
that.  We have to be sure that there is not a non-const T& method, because that
would be a better match for WeakMember<T>& than the method with const
WeakMember<T>& signature.

[Mads Ager (chromium), 2014-06-06 09:59:17]

LGTM

There will be some plugin cleanup to do after this change.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:1046: if (strongify ==
WebCore::WeakPointersActStrong)
In the heap tests code, it looks like you added a method in the visitor for
this. Can you do visitor->traceInCollection(weakMember, strongify) here or does
that jump through hoops that you don't want here?

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:2153: struct CollectionBackingTraceTrait<false,
true, WeakPointersActStrong, T, Traits> {
I'm a little lost in boolean parameters. Maybe we can introduce some enums for
these? ;-) The false here seems to mean that we don't normally need tracing for
this backing (because this is a backing of weak members) but we do need tracing
in this case because WeakPointersActStrong.

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

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:47: class IntWrapper;
Can we just move IntWrapper to right after ThreadMarker? IntWrapper should be
very simple and not really rely on anything else and we use it in most of the
tests so it makes sense to have it at the top.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:82: struct PairWithWeakHandling : public
StrongWeakPair {
This is clever since it shuts up the gc plugin. ;-)

I think there is some work to do here to make this work properly with the gc
plugin if you do this manually instead:

class PairWithWeakHandling {
    ALLOW_ONLY_INLINE_ALLOCATION();
public:
    ...
private:
    Member<IntWrapper> m_first;
    WeakMember<IntWrapper> m_second;
};

In this case the gc plugin needs to treat the
traceInCollection/shouldRemoveFromCollection pair as a trace method. That is, it
needs to require either a trace method or both of traceInCollection and
shouldRemoveFromCollection. traceInCollection should have the normal trace
requirements that all members and weak members are mentioned. That seems like a
fairly simple change to the plugin. To make it safe, the plugin would have to
also check that if you use traceInCollection and not trace you need to
specialize the trait. Not sure how easy that will be to check. Maybe it should
just require that you always have a trace method and then if you have these
methods it should check that they do the right thing as well. That is something
to work out after this change though.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4088: // No looking at the set and set2 now,
they are busted by our lying about the heap pointers on the stack!
How about scoping them so we can't touch them?

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4089: if (set3->size()) {
Since we ignore all heap pointers on the stack this should be guaranteed to
clear out the element, right? So this should be EXPECT_EQ(0, set3->size())?

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4094: set3->clear();
And then this is not needed.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4159: if (map3->size()) {
Same here. This should be reduced to the EXPECT_EQ in the else branch?

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4236: if (map3->size()) {
This one too.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:372: void trace(WeakPtr<T>&)
Where do you need this? This doesn't really look like something we want.

[Erik Corry, 2014-06-06 12:37:21]

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:1046: if (strongify ==
WebCore::WeakPointersActStrong)
On 2014/06/06 09:59:16, Mads Ager (chromium) wrote:
> In the heap tests code, it looks like you added a method in the visitor for
> this. Can you do visitor->traceInCollection(weakMember, strongify) here or
does
> that jump through hoops that you don't want here?

That would go into a loop, since the visitor method you mention ultimately calls
this.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:2153: struct CollectionBackingTraceTrait<false,
true, WeakPointersActStrong, T, Traits> {
On 2014/06/06 09:59:16, Mads Ager (chromium) wrote:
> I'm a little lost in boolean parameters. Maybe we can introduce some enums for
> these? ;-) The false here seems to mean that we don't normally need tracing
for
> this backing (because this is a backing of weak members) but we do need
tracing
> in this case because WeakPointersActStrong.

The true means that this is a specialization for types that have special weak
handling when used in sets and maps, and we are passing that to the
TraceInCollectionTrait below.  I changed it to an enum.

[Erik Corry, 2014-06-10 11:21:19]

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

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:47: class IntWrapper;
On 2014/06/06 09:59:17, Mads Ager (chromium) wrote:
> Can we just move IntWrapper to right after ThreadMarker? IntWrapper should be
> very simple and not really rely on anything else and we use it in most of the
> tests so it makes sense to have it at the top.

Done.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:82: struct PairWithWeakHandling : public
StrongWeakPair {
On 2014/06/06 09:59:17, Mads Ager (chromium) wrote:
> This is clever since it shuts up the gc plugin. ;-)
> 
> I think there is some work to do here to make this work properly with the gc
> plugin if you do this manually instead:
> 
> class PairWithWeakHandling {
>     ALLOW_ONLY_INLINE_ALLOCATION();
> public:
>     ...
> private:
>     Member<IntWrapper> m_first;
>     WeakMember<IntWrapper> m_second;
> };
> 
> In this case the gc plugin needs to treat the
> traceInCollection/shouldRemoveFromCollection pair as a trace method. That is,
it
> needs to require either a trace method or both of traceInCollection and
> shouldRemoveFromCollection. traceInCollection should have the normal trace
> requirements that all members and weak members are mentioned. That seems like
a
> fairly simple change to the plugin. To make it safe, the plugin would have to
> also check that if you use traceInCollection and not trace you need to
> specialize the trait. Not sure how easy that will be to check. Maybe it should
> just require that you always have a trace method and then if you have these
> methods it should check that they do the right thing as well. That is
something
> to work out after this change though.

OK

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4088: // No looking at the set and set2 now,
they are busted by our lying about the heap pointers on the stack!
On 2014/06/06 09:59:17, Mads Ager (chromium) wrote:
> How about scoping them so we can't touch them?

Done.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4089: if (set3->size()) {
On 2014/06/06 09:59:17, Mads Ager (chromium) wrote:
> Since we ignore all heap pointers on the stack this should be guaranteed to
> clear out the element, right? So this should be EXPECT_EQ(0, set3->size())?

Done.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4094: set3->clear();
On 2014/06/06 09:59:17, Mads Ager (chromium) wrote:
> And then this is not needed.

Done.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4159: if (map3->size()) {
On 2014/06/06 09:59:16, Mads Ager (chromium) wrote:
> Same here. This should be reduced to the EXPECT_EQ in the else branch?

Done.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:4236: if (map3->size()) {
On 2014/06/06 09:59:17, Mads Ager (chromium) wrote:
> This one too.

Done.

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/319593004/diff/60001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:372: void trace(WeakPtr<T>&)
On 2014/06/06 09:59:17, Mads Ager (chromium) wrote:
> Where do you need this? This doesn't really look like something we want.

Removed.

[Erik Corry, 2014-06-10 11:21:30]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-06-10 11:22:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erik.corry@gmail.com/319593004/120001

[Erik Corry, 2014-06-10 12:54:44]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-06-10 12:56:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erik.corry@gmail.com/319593004/140001

[commit-bot: I haz the power, 2014-06-10 14:27:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11432)

[commit-bot: I haz the power, 2014-06-10 15:57:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 15:57:52]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11442)

[Erik Corry, 2014-06-12 06:03:07]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-06-12 06:04:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erik.corry@gmail.com/319593004/160001

[commit-bot: I haz the power, 2014-06-12 07:17:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11688)

[commit-bot: I haz the power, 2014-06-12 08:26:10]

Change committed as 176009