Oilpan: Move the EventListener hierarchy to Oilpan's heap

Oilpan: Move the EventListener hierarchy to Oilpan's heap

Not yet ready for review.

BUG=

Committed: https://crrev.com/98155149a92c3a691674e2a6281dd0e7e8aff6fe
git-svn-id: svn://svn.chromium.org/blink/trunk@199521 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[haraken, 2015-07-16 11:41:36]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, yutak@chromium.org

[haraken, 2015-07-16 11:43:25]

This CL is not yet ready for review, but it seems that I'm hitting a bug of the
GC plugin.

yutak@: Would you take a look at the issue?

https://codereview.chromium.org/1238083002/diff/1/Source/modules/filesystem/I...
File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right):

https://codereview.chromium.org/1238083002/diff/1/Source/modules/filesystem/I...
Source/modules/filesystem/InspectorFileSystemAgent.cpp:102:
BaseCallback::trace(visitor);

I'm hitting the following error. It seems that the GC plugin fails at
interpreting "BaseCallback".

../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
error: [blink-gc] Base class 'ErrorCallback' of derived class
'CallbackDispatcher<blink::ErrorCallback, blink::(anonymous
namespace)::FileSystemRootRequest, blink::FileError *>' requires tracing.
    DEFINE_INLINE_VIRTUAL_TRACE()
    ^
../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note: expanded
from macro 'DEFINE_INLINE_VIRTUAL_TRACE'
#define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
                                      ^
../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note: expanded
from macro 'DEFINE_INLINE_TRACE_IMPL'
    inline void traceImpl(VisitorDispatcher visitor)
    ^
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
error: [blink-gc] Base class 'EntryCallback' of derived class
'CallbackDispatcher<blink::EntryCallback, blink::(anonymous
namespace)::FileSystemRootRequest, blink::Entry *>' requires tracing.
../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note: expanded
from macro 'DEFINE_INLINE_VIRTUAL_TRACE' 
#define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
                                      ^
../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note: expanded
from macro 'DEFINE_INLINE_TRACE_IMPL'
    inline void traceImpl(VisitorDispatcher visitor)
    ^
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
error: [blink-gc] Base class 'ErrorCallback' of derived class
'CallbackDispatcher<blink::ErrorCallback, blink::(anonymous
namespace)::DirectoryContentRequest, blink::FileError *>' requires tracing.
../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note: expanded
from macro 'DEFINE_INLINE_VIRTUAL_TRACE' 
#define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
                                      ^
../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note: expanded
from macro 'DEFINE_INLINE_TRACE_IMPL'
    inline void traceImpl(VisitorDispatcher visitor)
    ^

[haraken, 2015-07-17 02:44:29]

https://codereview.chromium.org/1238083002/diff/20001/Source/modules/filesyst...
File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right):

https://codereview.chromium.org/1238083002/diff/20001/Source/modules/filesyst...
Source/modules/filesystem/InspectorFileSystemAgent.cpp:100: GC_PLUGIN_IGNORE("")

Hmm, this doesn't work either. I don't know how we can suppress the GC plugin
error.

[Yuta Kitamura, 2015-07-17 04:23:24]

https://codereview.chromium.org/1238083002/diff/1/Source/modules/filesystem/I...
File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right):

https://codereview.chromium.org/1238083002/diff/1/Source/modules/filesystem/I...
Source/modules/filesystem/InspectorFileSystemAgent.cpp:102:
BaseCallback::trace(visitor);
On 2015/07/16 11:43:25, haraken wrote:
> 
> I'm hitting the following error. It seems that the GC plugin fails at
> interpreting "BaseCallback".
> 
>
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
> error: [blink-gc] Base class 'ErrorCallback' of derived class
> 'CallbackDispatcher<blink::ErrorCallback, blink::(anonymous
> namespace)::FileSystemRootRequest, blink::FileError *>' requires tracing.
>     DEFINE_INLINE_VIRTUAL_TRACE()
>     ^
> ../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note: expanded
> from macro 'DEFINE_INLINE_VIRTUAL_TRACE'
> #define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
>                                       ^
> ../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note: expanded
> from macro 'DEFINE_INLINE_TRACE_IMPL'
>     inline void traceImpl(VisitorDispatcher visitor)
>     ^
>
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
> error: [blink-gc] Base class 'EntryCallback' of derived class
> 'CallbackDispatcher<blink::EntryCallback, blink::(anonymous
> namespace)::FileSystemRootRequest, blink::Entry *>' requires tracing.
> ../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note: expanded
> from macro 'DEFINE_INLINE_VIRTUAL_TRACE' 
> #define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
>                                       ^
> ../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note: expanded
> from macro 'DEFINE_INLINE_TRACE_IMPL'
>     inline void traceImpl(VisitorDispatcher visitor)
>     ^
>
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
> error: [blink-gc] Base class 'ErrorCallback' of derived class
> 'CallbackDispatcher<blink::ErrorCallback, blink::(anonymous
> namespace)::DirectoryContentRequest, blink::FileError *>' requires tracing.
> ../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note: expanded
> from macro 'DEFINE_INLINE_VIRTUAL_TRACE' 
> #define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
>                                       ^
> ../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note: expanded
> from macro 'DEFINE_INLINE_TRACE_IMPL'
>     inline void traceImpl(VisitorDispatcher visitor)
>     ^

Hmm, getting this correct can be pretty hard since BaseCallback
is a template-dependent type.

[haraken, 2015-07-17 04:24:29]

On 2015/07/17 04:23:24, Yuta Kitamura wrote:
>
https://codereview.chromium.org/1238083002/diff/1/Source/modules/filesystem/I...
> File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right):
> 
>
https://codereview.chromium.org/1238083002/diff/1/Source/modules/filesystem/I...
> Source/modules/filesystem/InspectorFileSystemAgent.cpp:102:
> BaseCallback::trace(visitor);
> On 2015/07/16 11:43:25, haraken wrote:
> > 
> > I'm hitting the following error. It seems that the GC plugin fails at
> > interpreting "BaseCallback".
> > 
> >
>
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
> > error: [blink-gc] Base class 'ErrorCallback' of derived class
> > 'CallbackDispatcher<blink::ErrorCallback, blink::(anonymous
> > namespace)::FileSystemRootRequest, blink::FileError *>' requires tracing.
> >     DEFINE_INLINE_VIRTUAL_TRACE()
> >     ^
> > ../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note:
expanded
> > from macro 'DEFINE_INLINE_VIRTUAL_TRACE'
> > #define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
> >                                       ^
> > ../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note:
expanded
> > from macro 'DEFINE_INLINE_TRACE_IMPL'
> >     inline void traceImpl(VisitorDispatcher visitor)
> >     ^
> >
>
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
> > error: [blink-gc] Base class 'EntryCallback' of derived class
> > 'CallbackDispatcher<blink::EntryCallback, blink::(anonymous
> > namespace)::FileSystemRootRequest, blink::Entry *>' requires tracing.
> > ../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note:
expanded
> > from macro 'DEFINE_INLINE_VIRTUAL_TRACE' 
> > #define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
> >                                       ^
> > ../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note:
expanded
> > from macro 'DEFINE_INLINE_TRACE_IMPL'
> >     inline void traceImpl(VisitorDispatcher visitor)
> >     ^
> >
>
../../third_party/WebKit/Source/modules/filesystem/InspectorFileSystemAgent.cpp:99:5:
> > error: [blink-gc] Base class 'ErrorCallback' of derived class
> > 'CallbackDispatcher<blink::ErrorCallback, blink::(anonymous
> > namespace)::DirectoryContentRequest, blink::FileError *>' requires tracing.
> > ../../third_party/WebKit/Source/platform/heap/Visitor.h:142:39: note:
expanded
> > from macro 'DEFINE_INLINE_VIRTUAL_TRACE' 
> > #define DEFINE_INLINE_VIRTUAL_TRACE() DEFINE_INLINE_TRACE_IMPL(virtual)
> >                                       ^
> > ../../third_party/WebKit/Source/platform/heap/Visitor.h:113:5: note:
expanded
> > from macro 'DEFINE_INLINE_TRACE_IMPL'
> >     inline void traceImpl(VisitorDispatcher visitor)
> >     ^
> 
> Hmm, getting this correct can be pretty hard since BaseCallback
> is a template-dependent type.

Is there any way to suppress the error at least?

[Yuta Kitamura, 2015-07-17 05:54:36]

On 2015/07/17 04:24:29, haraken wrote:
> Is there any way to suppress the error at least?

Don't know, but it may be worth trying to put GC_PLUGIN_IGNORE to
the class.

[haraken, 2015-07-17 06:22:21]

On 2015/07/17 05:54:36, Yuta Kitamura wrote:
> On 2015/07/17 04:24:29, haraken wrote:
> > Is there any way to suppress the error at least?
> 
> Don't know, but it may be worth trying to put GC_PLUGIN_IGNORE to
> the class.

Thanks, it works :)

[haraken, 2015-07-17 07:49:36]

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

[haraken, 2015-07-17 07:49:38]

kinuko-san, I have one question.

https://codereview.chromium.org/1238083002/diff/20001/Source/modules/filesyst...
File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right):

https://codereview.chromium.org/1238083002/diff/20001/Source/modules/filesyst...
Source/modules/filesystem/InspectorFileSystemAgent.cpp:212:
reportResult(FileError::ABORT_ERR);

kinuko@: Currently we're reporting FileError::ABORT_ERR every time the
DirectoryContentRequest object gets destructed (even if the request has
completed successfully). Is this an expected behavior?

If we need to keep reporting FileError::ABORT_ERR in DirectoryContentRequest's
destructor, we can do that in the oilpan world as well. But if we can remove it,
that is easier.

[haraken, 2015-07-21 00:11:48]

On 2015/07/17 07:49:38, haraken wrote:
> kinuko-san, I have one question.
> 
>
https://codereview.chromium.org/1238083002/diff/20001/Source/modules/filesyst...
> File Source/modules/filesystem/InspectorFileSystemAgent.cpp (right):
> 
>
https://codereview.chromium.org/1238083002/diff/20001/Source/modules/filesyst...
> Source/modules/filesystem/InspectorFileSystemAgent.cpp:212:
> reportResult(FileError::ABORT_ERR);
> 
> kinuko@: Currently we're reporting FileError::ABORT_ERR every time the
> DirectoryContentRequest object gets destructed (even if the request has
> completed successfully). Is this an expected behavior?
> 
> If we need to keep reporting FileError::ABORT_ERR in DirectoryContentRequest's
> destructor, we can do that in the oilpan world as well. But if we can remove
it,
> that is easier.

kinuk-san: friendly ping?

[haraken, 2015-07-22 00:09:45]

kinuko-san: ping?

[kinuko, 2015-07-22 01:15:00]

kinuko@chromium.org changed reviewers:
+ tzik@chromium.org

[kinuko, 2015-07-22 01:48:08]

On 2015/07/22 00:09:45, haraken wrote:
> kinuko-san: ping?

Wow sorry somehow this kept slipping from my priority box. From a quick
inspection, and given it's inspector only code, I think we can remove the dtor
code. (cc-ing tzik in case I'm missing sth obvious)

[sof, 2015-07-22 15:25:08]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-07-22 15:25:09]

lgtm

https://codereview.chromium.org/1238083002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/1238083002/diff/20001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:98:
RefPtrWillBeRawPtr<{{argument.idl_type}}> {{argument.name}}
Wouldn't it be preferable to nullptr initialize this?

And doesn't it show up in the bindings tests so that you need to sync expected
results?

[haraken, 2015-07-23 00:53:02]

Thanks for review!

>
https://codereview.chromium.org/1238083002/diff/20001/Source/bindings/templat...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/1238083002/diff/20001/Source/bindings/templat...
> Source/bindings/templates/methods.cpp:98:
> RefPtrWillBeRawPtr<{{argument.idl_type}}> {{argument.name}}
> Wouldn't it be preferable to nullptr initialize this?
> 
> And doesn't it show up in the bindings tests so that you need to sync expected
> results?

It seems that we don't have a binding test for this part.

[haraken, 2015-07-23 11:05:51]

https://codereview.chromium.org/1238083002/diff/60001/Source/core/events/Even...
File Source/core/events/EventListenerMap.h (right):

https://codereview.chromium.org/1238083002/diff/60001/Source/core/events/Even...
Source/core/events/EventListenerMap.h:70: Vector<std::pair<AtomicString,
OwnPtr<EventListenerVector>>, 2> m_entries;

The GC plugin should have detected this bug... This must be
OwnPtrWillBeMember<EventListenerVector>.

[haraken, 2015-07-23 11:26:19]

https://codereview.chromium.org/1238083002/diff/60001/Source/core/events/Even...
File Source/core/events/EventListenerMap.h (right):

https://codereview.chromium.org/1238083002/diff/60001/Source/core/events/Even...
Source/core/events/EventListenerMap.h:70: Vector<std::pair<AtomicString,
OwnPtr<EventListenerVector>>, 2> m_entries;
On 2015/07/23 11:05:51, haraken wrote:
> 
> The GC plugin should have detected this bug... This must be
> OwnPtrWillBeMember<EventListenerVector>.

Filed a bug: https://code.google.com/p/chromium/issues/detail?id=513202

[haraken, 2015-07-23 11:28:00]

Sigbjorn: I made a non-trivial change to EventTarget.h, EventListenerMap.h and
Node.cpp. Would you take another look?

[sof, 2015-07-23 11:54:16]

still lgtm

https://codereview.chromium.org/1238083002/diff/100001/Source/core/events/Eve...
File Source/core/events/EventListenerMap.h (right):

https://codereview.chromium.org/1238083002/diff/100001/Source/core/events/Eve...
Source/core/events/EventListenerMap.h:92: EventListenerMap* m_map;
Add a comment documenting that this raw (part object) pointer is safe?

Worth considering adding the incoming EventTarget as a Member?

[sof, 2015-07-23 12:11:45]

https://codereview.chromium.org/1238083002/diff/100001/Source/core/events/Eve...
File Source/core/events/EventListenerMap.h (right):

https://codereview.chromium.org/1238083002/diff/100001/Source/core/events/Eve...
Source/core/events/EventListenerMap.h:92: EventListenerMap* m_map;
On 2015/07/23 11:54:16, sof wrote:
> Add a comment documenting that this raw (part object) pointer is safe?
> 
> Worth considering adding the incoming EventTarget as a Member?

Hmm, this is being overly fussy wrt Member<> :) If it is stack allocated,
there's little reason to suspect that the incoming EventTarget isn't on the
stack already.

[haraken, 2015-07-23 15:10:27]

On 2015/07/23 12:11:45, sof wrote:
>
https://codereview.chromium.org/1238083002/diff/100001/Source/core/events/Eve...
> File Source/core/events/EventListenerMap.h (right):
> 
>
https://codereview.chromium.org/1238083002/diff/100001/Source/core/events/Eve...
> Source/core/events/EventListenerMap.h:92: EventListenerMap* m_map;
> On 2015/07/23 11:54:16, sof wrote:
> > Add a comment documenting that this raw (part object) pointer is safe?
> > 
> > Worth considering adding the incoming EventTarget as a Member?
> 
> Hmm, this is being overly fussy wrt Member<> :) If it is stack allocated,
> there's little reason to suspect that the incoming EventTarget isn't on the
> stack already.

Added a comment. I think it's a nice idea to use Member<EventTarget>.

It looks like the oilpan bots are not happy. Will take a look.

[haraken, 2015-07-27 15:07:29]

https://codereview.chromium.org/1238083002/diff/120001/Source/core/events/Eve...
File Source/core/events/EventTarget.cpp (left):

https://codereview.chromium.org/1238083002/diff/120001/Source/core/events/Eve...
Source/core/events/EventTarget.cpp:371:
AtomicallyInitializedStaticReference(EventListenerVector, emptyVector, new
EventListenerVector);

It took me hours to notice this bug... The created EventListenerVector gets
collected because it is not stored in a persistent handle.

The GC plugin should detect this kind of programming error.

[haraken, 2015-07-27 17:21:27]

On 2015/07/27 15:07:29, haraken wrote:
>
https://codereview.chromium.org/1238083002/diff/120001/Source/core/events/Eve...
> File Source/core/events/EventTarget.cpp (left):
> 
>
https://codereview.chromium.org/1238083002/diff/120001/Source/core/events/Eve...
> Source/core/events/EventTarget.cpp:371:
> AtomicallyInitializedStaticReference(EventListenerVector, emptyVector, new
> EventListenerVector);
> 
> It took me hours to notice this bug... The created EventListenerVector gets
> collected because it is not stored in a persistent handle.
> 
> The GC plugin should detect this kind of programming error.

The bot looks happy; let me land this.

[haraken, 2015-07-27 17:21:34]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-07-27 17:21:34]

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

[commit-bot: I haz the power, 2015-07-27 17:21:50]

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

[commit-bot: I haz the power, 2015-07-27 18:29:56]

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

[sof, 2015-07-30 07:35:58]

Meta remark: a non-trivial CL with a fairly big footprint. Should we try to
associate those with filed bugs for better tracking? Allows for easier grouping
of commits if followups are needed, if nothing else.

[haraken, 2015-07-30 07:57:30]

On 2015/07/30 07:35:58, sof wrote:
> Meta remark: a non-trivial CL with a fairly big footprint. Should we try to
> associate those with filed bugs for better tracking? Allows for easier
grouping
> of commits if followups are needed, if nothing else.

Agreed. At the very least, I shouldn't have committed this one with the
not-yet-ready-for-review description...

[haraken, 2015-07-30 07:58:13]

On 2015/07/30 07:57:30, haraken wrote:
> On 2015/07/30 07:35:58, sof wrote:
> > Meta remark: a non-trivial CL with a fairly big footprint. Should we try to
> > associate those with filed bugs for better tracking? Allows for easier
> grouping
> > of commits if followups are needed, if nothing else.
> 
> Agreed. At the very least, I shouldn't have committed this one with the
> not-yet-ready-for-review description...

It would be unsafe to revert this kind of a big change, so let me keep it. I'll
update the CL description here.

[commit-bot: I haz the power, 2015-09-23 11:48:58]

Patchset 7 (id:??) landed as
https://crrev.com/98155149a92c3a691674e2a6281dd0e7e8aff6fe