Make Event RefCountedGarbageCollected and implement trace() methods to the Event hierarcy

Make Event RefCountedGarbageCollected and implement trace() methods to the Event hierarcy

- This CL makes Event RefCountedGarbageCollected. In a next CL, I'll change this to GarbageCollectedFinalized.

- This CL just implements trace() methods and doesn't change any RefPtr<XXXEvent> to transition types. I'll do that in a next CL.

- This CL changes Persistent handles in the Event hierarchy to Members.

- The only raw pointer in the Event hierarchy is Event::m_currentTarget. This raw pointer is no problem because it's cleared after the Event is dispatched.

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

[haraken, 2014-02-26 08:05:33]

PTAL

I implemented the trace methods carefully, but I'd be happy if reviewers
double-check that I'm not missing something.

[tkent, 2014-02-26 08:14:02]

https://codereview.chromium.org/181153003/diff/1/Source/modules/device_orient...
File Source/modules/device_orientation/DeviceMotionEvent.h (right):

https://codereview.chromium.org/181153003/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceMotionEvent.h:67:
RefPtrWillBeMember<DeviceMotionData> m_deviceMotionData;
I have a concern about changes like this.

After this change, applying real Oilpan types in modules/device_orientation
would not be a simple task.
* Need to consume time to distinguish between module-related and event-related.
* Need to introduce PersistentWillBeMember transition type.

[haraken, 2014-02-26 08:18:29]

https://codereview.chromium.org/181153003/diff/1/Source/modules/device_orient...
File Source/modules/device_orientation/DeviceMotionEvent.h (right):

https://codereview.chromium.org/181153003/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceMotionEvent.h:67:
RefPtrWillBeMember<DeviceMotionData> m_deviceMotionData;
On 2014/02/26 08:14:03, tkent wrote:
> I have a concern about changes like this.
> 
> After this change, applying real Oilpan types in modules/device_orientation
> would not be a simple task.
> * Need to consume time to distinguish between module-related and
event-related.
> * Need to introduce PersistentWillBeMember transition type.

Good point. The concern is valid. However, we cannot use persistent handles in
garbage-collected objects (because they will cause leaks).

Mads: Do you have any idea to address the issue?

[haraken, 2014-02-26 08:21:09]

I'm afraid that we've already replaced a bunch of RefPtrWillBePersistent<>s with
RefPtrWillBeMember<>s, so we've already produced the issue in several places...

[Mads Ager (chromium), 2014-02-26 08:42:31]

On 2014/02/26 08:18:29, haraken wrote:
>
https://codereview.chromium.org/181153003/diff/1/Source/modules/device_orient...
> File Source/modules/device_orientation/DeviceMotionEvent.h (right):
> 
>
https://codereview.chromium.org/181153003/diff/1/Source/modules/device_orient...
> Source/modules/device_orientation/DeviceMotionEvent.h:67:
> RefPtrWillBeMember<DeviceMotionData> m_deviceMotionData;
> On 2014/02/26 08:14:03, tkent wrote:
> > I have a concern about changes like this.
> > 
> > After this change, applying real Oilpan types in modules/device_orientation
> > would not be a simple task.
> > * Need to consume time to distinguish between module-related and
> event-related.
> > * Need to introduce PersistentWillBeMember transition type.
> 
> Good point. The concern is valid. However, we cannot use persistent handles in
> garbage-collected objects (because they will cause leaks).

I don't think PersistentWillBeMember will be a problem. We can do that since it
will be a Persistent when the containing object is not in the heap and a Member
when the containing object is in the heap. 

> Mads: Do you have any idea to address the issue?

I don't think we can address it. Going forward we will have to have the
PersistentWillBeMember transition type when we start shipping some objects and
not others. We should make sure that we get compile-time errors if we are doing
it wrong. In this case, when something is no longer RefCounted we will have to
change all the RefPtrWillBeMember with PersistentWillBeMember.

[haraken, 2014-02-26 08:47:36]

> I don't think PersistentWillBeMember will be a problem. We can do that since
it
> will be a Persistent when the containing object is not in the heap and a
Member
> when the containing object is in the heap. 
> 
> > Mads: Do you have any idea to address the issue?
> 
> I don't think we can address it. Going forward we will have to have the
> PersistentWillBeMember transition type when we start shipping some objects and
> not others. We should make sure that we get compile-time errors if we are
doing
> it wrong. In this case, when something is no longer RefCounted we will have to
> change all the RefPtrWillBeMember with PersistentWillBeMember.

Agreed. PersistentWillBeMember sounds reasonable to me.

It should be possible to get compile-time errors if we misuse them.

[Mads Ager (chromium), 2014-02-26 09:03:51]

LGTM with two missing trace calls fixed.

https://codereview.chromium.org/181153003/diff/1/Source/core/events/MouseEven...
File Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/181153003/diff/1/Source/core/events/MouseEven...
Source/core/events/MouseEvent.cpp:193: void MouseEvent::trace(Visitor* visitor)
You need to trace m_clipBoard

https://codereview.chromium.org/181153003/diff/1/Source/core/speech/SpeechInp...
File Source/core/speech/SpeechInputEvent.cpp (right):

https://codereview.chromium.org/181153003/diff/1/Source/core/speech/SpeechInp...
Source/core/speech/SpeechInputEvent.cpp:74: Event::trace(visitor);
You need to trace m_result.

[tkent, 2014-02-26 09:09:27]

> > I don't think we can address it. Going forward we will have to have the
> > PersistentWillBeMember transition type when we start shipping some objects
and
> > not others. We should make sure that we get compile-time errors if we are
> doing
> > it wrong. In this case, when something is no longer RefCounted we will have
to
> > change all the RefPtrWillBeMember with PersistentWillBeMember.
> 
> Agreed. PersistentWillBeMember sounds reasonable to me.

Yeah, PersistentWillBeMember should work. I'm afraid we'll waste our time to
change types again and again.
Can we postpone this CL until we convert existing transition types in
Source/modules/ to real types?

[haraken, 2014-02-26 09:12:12]

> LGTM with two missing trace calls fixed.

Thanks for reading!

I'm thinking about how to replace RefPtr<XXXEvent> incrementally. If I add
[WillBeGarbageCollected] to Event.idl, the IDL compiler generates code that
expects PassRefPtrWillBeRawPtr for all XXXEvent pointers. This means that I need
to replace all RefPtr<XXXEvent>s in one go. (I wonder why CSSValue is not
encountering the same problem.)

One way to do the replacement incrementally would be to make
[WillBeGarbageCollected] not inherited in IDL files. We can add
[WillBeGarbageCollected] to each IDL file in the Event hierarchy, by which we
can move XXXEvents incrementally.

What do you think?

[Mads Ager (chromium), 2014-02-26 09:17:36]

On 2014/02/26 09:09:27, tkent wrote:
> > > I don't think we can address it. Going forward we will have to have the
> > > PersistentWillBeMember transition type when we start shipping some objects
> and
> > > not others. We should make sure that we get compile-time errors if we are
> > doing
> > > it wrong. In this case, when something is no longer RefCounted we will
have
> to
> > > change all the RefPtrWillBeMember with PersistentWillBeMember.
> > 
> > Agreed. PersistentWillBeMember sounds reasonable to me.
> 
> Yeah, PersistentWillBeMember should work. I'm afraid we'll waste our time to
> change types again and again.
> Can we postpone this CL until we convert existing transition types in
> Source/modules/ to real types?

We could. However, at this point new versions of clang is apparently misbehaving
and therefore it is unclear when we will be able to update clang and get rid of
the first transition types. I'm afraid we will get blocked if we are not allowed
to make these changes.

I don't think this will be much of an issue though. When you move a class to
oilpan permanently, you change all the transition types in that class to the
actual types. Then you try to compile and you will find all the
RefPtrWillBeSomethingElse occurrences because RefPtr no longer works. Then you
replace them: RefPtrWillBeRawPtr<T> -> T*, RefPtrWillBeMember<T> ->
PersistentWillBeMember<T>.

The replacing of types will be a little involved in any case. Consider:

void ABC::myMethod()
{
    ...
    RefPtrWillBeRawPtr<T> ptr;
    if (something)
        ptr = something->getPtr();
    ...
    if (ptr)
        doStuff();
}

When we replace RefPtrWillBeRawPtr<T> we need to be careful. The pointer classes
have zero initialization built in. We now need to make sure to replace it with
'T* ptr = 0;' and not just 'T* ptr'.

[Mads Ager (chromium), 2014-02-26 09:19:29]

On 2014/02/26 09:17:36, Mads Ager (chromium) wrote:
> On 2014/02/26 09:09:27, tkent wrote:
> > > > I don't think we can address it. Going forward we will have to have the
> > > > PersistentWillBeMember transition type when we start shipping some
objects
> > and
> > > > not others. We should make sure that we get compile-time errors if we
are
> > > doing
> > > > it wrong. In this case, when something is no longer RefCounted we will
> have
> > to
> > > > change all the RefPtrWillBeMember with PersistentWillBeMember.
> > > 
> > > Agreed. PersistentWillBeMember sounds reasonable to me.
> > 
> > Yeah, PersistentWillBeMember should work. I'm afraid we'll waste our time to
> > change types again and again.
> > Can we postpone this CL until we convert existing transition types in
> > Source/modules/ to real types?
> 
> We could. However, at this point new versions of clang is apparently
misbehaving
> and therefore it is unclear when we will be able to update clang and get rid
of
> the first transition types. I'm afraid we will get blocked if we are not
allowed
> to make these changes.
> 
> I don't think this will be much of an issue though. When you move a class to
> oilpan permanently, you change all the transition types in that class to the
> actual types. 

And by all the transition types I mean all the transition types for other types
that you are permanently moving the the oilpan heap.

> Then you try to compile and you will find all the
> RefPtrWillBeSomethingElse occurrences because RefPtr no longer works. Then you
> replace them: RefPtrWillBeRawPtr<T> -> T*, RefPtrWillBeMember<T> ->
> PersistentWillBeMember<T>.
> 
> The replacing of types will be a little involved in any case. Consider:
> 
> void ABC::myMethod()
> {
>     ...
>     RefPtrWillBeRawPtr<T> ptr;
>     if (something)
>         ptr = something->getPtr();
>     ...
>     if (ptr)
>         doStuff();
> }
> 
> When we replace RefPtrWillBeRawPtr<T> we need to be careful. The pointer
classes
> have zero initialization built in. We now need to make sure to replace it with
> 'T* ptr = 0;' and not just 'T* ptr'.

[haraken, 2014-02-26 09:21:53]

> Yeah, PersistentWillBeMember should work. I'm afraid we'll waste our time to
> change types again and again.
> Can we postpone this CL until we convert existing transition types in
> Source/modules/ to real types?

IMHO:

- It's never a good thing to increase transition types in the code base (as I
emailed yesterday).

- However, delaying moving core/events/ and thus avoiding using
PersistentWillBeMember won't much solve problems (although I agree that it will
decrease the amount of wasteful work to some extent). I think we'll anyway need
to introduce PersistentWillBeMember at some point. I think we can move things
forward by using PersistentWillBeMembers rather than worrying about object
dependency and avoiding using PersistentWillBeMembers.

What do you think?

[Mads Ager (chromium), 2014-02-26 09:23:31]

On 2014/02/26 09:12:12, haraken wrote:
> > LGTM with two missing trace calls fixed.
> 
> Thanks for reading!
> 
> I'm thinking about how to replace RefPtr<XXXEvent> incrementally. If I add
> [WillBeGarbageCollected] to Event.idl, the IDL compiler generates code that
> expects PassRefPtrWillBeRawPtr for all XXXEvent pointers. This means that I
need
> to replace all RefPtr<XXXEvent>s in one go. (I wonder why CSSValue is not
> encountering the same problem.)
> 
> One way to do the replacement incrementally would be to make
> [WillBeGarbageCollected] not inherited in IDL files. We can add
> [WillBeGarbageCollected] to each IDL file in the Event hierarchy, by which we
> can move XXXEvents incrementally.
> 
> What do you think?

Do you need to change all RefPtrs? I'm wondering if it would be enough to change
the create methods from:

PassRefPtr<XXXEvent> create() { adoptRef(new XXXEvent); }

to

PassRefPtrWillBeRawPtr<XXXEvent> create() {
adoptRefCountedWillBeRefCountedGarbageCollected(new XXXEvent); }

?

[haraken, 2014-02-26 10:36:03]

On 2014/02/26 09:23:31, Mads Ager (chromium) wrote:
> On 2014/02/26 09:12:12, haraken wrote:
> > > LGTM with two missing trace calls fixed.
> > 
> > Thanks for reading!
> > 
> > I'm thinking about how to replace RefPtr<XXXEvent> incrementally. If I add
> > [WillBeGarbageCollected] to Event.idl, the IDL compiler generates code that
> > expects PassRefPtrWillBeRawPtr for all XXXEvent pointers. This means that I
> need
> > to replace all RefPtr<XXXEvent>s in one go. (I wonder why CSSValue is not
> > encountering the same problem.)
> > 
> > One way to do the replacement incrementally would be to make
> > [WillBeGarbageCollected] not inherited in IDL files. We can add
> > [WillBeGarbageCollected] to each IDL file in the Event hierarchy, by which
we
> > can move XXXEvents incrementally.
> > 
> > What do you think?
> 
> Do you need to change all RefPtrs? I'm wondering if it would be enough to
change
> the create methods from:
> 
> PassRefPtr<XXXEvent> create() { adoptRef(new XXXEvent); }
> 
> to
> 
> PassRefPtrWillBeRawPtr<XXXEvent> create() {
> adoptRefCountedWillBeRefCountedGarbageCollected(new XXXEvent); }
> 
> ?

ah, probably you're right. It might be enough to change the create methods to
make [WillBeGarbageCollected] workable. Let me give it a try.

[haraken, 2014-02-26 11:16:55]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-02-26 11:17:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/181153003/20001

[commit-bot: I haz the power, 2014-02-26 12:40:34]

Change committed as 167905

[Erik Corry, 2014-02-28 14:08:51]

https://codereview.chromium.org/181153003/diff/20001/Source/core/events/Event.h
File Source/core/events/Event.h (right):

https://codereview.chromium.org/181153003/diff/20001/Source/core/events/Event...
Source/core/events/Event.h:207: RefPtr<Event> m_underlyingEvent;
I wonder why this was not a traced Member?

[haraken, 2014-02-28 14:12:40]

On 2014/02/28 14:08:51, Erik Corry wrote:
>
https://codereview.chromium.org/181153003/diff/20001/Source/core/events/Event.h
> File Source/core/events/Event.h (right):
> 
>
https://codereview.chromium.org/181153003/diff/20001/Source/core/events/Event...
> Source/core/events/Event.h:207: RefPtr<Event> m_underlyingEvent;
> I wonder why this was not a traced Member?

It's just because I wanted to do that in a next CL. There are a ton of
RefPtr<Event>s, so I need to do that incrementally.

It looks like Event changes caused several crashes. So I'll investigate them
before committing more things to Events.