Fixed memory issues with EventHandler::TouchInfo.

Fixed memory issues with EventHandler::TouchInfo.

Switched the type of TouchInfo.touchTarget to a ref-counted type, and made the TouchInfo vector oilpan-compliant.

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

[mustaq, 2015-06-22 16:38:12]

mustaq@chromium.org changed reviewers:
+ rbyers@chromium.org

[mustaq, 2015-06-22 16:38:12]

ptal, verified that the "minimized testcase" mentioned in the bug works after
the fix.

[Rick Byers, 2015-06-22 17:15:48]

This looks right for ref counting, but will probably still be broken in oilpan
builds.  Since there's nothing in the caller keeping the individual EventTargets
alive, I think we need TouchInfo to contain a Member<EventTarget> which gets
Trace'd.  See
https://www.chromium.org/blink/blink-gc#TOC-Collections-and-ALLOW_ONLY_INLINE....
 Looks like there's a quick fix with Peristent<T> but that's not recommended.

/cc oilpan-reviews to help with specific guidance here.

[mustaq, 2015-06-22 20:52:44]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-22 20:53:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198193005/20001

[commit-bot: I haz the power, 2015-06-22 22:48:06]

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

[commit-bot: I haz the power, 2015-06-22 22:48:07]

Dry run: Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60164)

[haraken, 2015-06-22 23:40:34]

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

[haraken, 2015-06-22 23:40:35]

Sorry that Oilpan is causing trouble...

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:321: using TouchInfo = struct {

class TouchInfo {
  ALLOW_ONLY_INLINE_ALLOCATION();
public:

  DEFINE_INLINE_TRACE()
  {
    visitor->trace(touchTarget);
    visitor->trace(targetFrame);
  }

  ...;
  RefPtrWillBeMember<EventTarget> touchTarget;
  RawPtrWillBeMember<LocalFrame> targetFrame;  // BTW, is it OK to use a raw
pointer to LocalFrame?
};

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:331: void
dispatchPointerEventsForTouchEvent(const PlatformTouchEvent&,
Vector<TouchInfo>&);

You need to use WillBeHeapVector<TouchInfo>.

[haraken, 2015-06-22 23:41:20]

Also make sure that the CL compiles with 'enable_oilpan=1 blink_gc_plugin=1'. It
will detect most oilpan-related errors at compile time for you.

[mustaq, 2015-06-23 15:25:51]

ptal.

Also tested the oilpan build, seems fine now but had to make TouchInfo public.
See patch#4.

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:321: using TouchInfo = struct {
On 2015/06/22 23:40:35, haraken wrote:
> 
> class TouchInfo {
>   ALLOW_ONLY_INLINE_ALLOCATION();
> public:
> 
>   DEFINE_INLINE_TRACE()
>   {
>     visitor->trace(touchTarget);
>     visitor->trace(targetFrame);
>   }
> 
>   ...;
>   RefPtrWillBeMember<EventTarget> touchTarget;
>   RawPtrWillBeMember<LocalFrame> targetFrame;  // BTW, is it OK to use a raw
> pointer to LocalFrame?
> };

Done, but kept the pointer to LocalFrame as is because Document::frame() returns
a raw pointer.

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:331: void
dispatchPointerEventsForTouchEvent(const PlatformTouchEvent&,
Vector<TouchInfo>&);
On 2015/06/22 23:40:35, haraken wrote:
> 
> You need to use WillBeHeapVector<TouchInfo>.

Done.

[sof, 2015-06-23 15:57:35]

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:321: using TouchInfo = struct {
On 2015/06/23 15:25:51, mustaq wrote:
> On 2015/06/22 23:40:35, haraken wrote:
> > 
> > class TouchInfo {
> >   ALLOW_ONLY_INLINE_ALLOCATION();
> > public:
> > 
> >   DEFINE_INLINE_TRACE()
> >   {
> >     visitor->trace(touchTarget);
> >     visitor->trace(targetFrame);
> >   }
> > 
> >   ...;
> >   RefPtrWillBeMember<EventTarget> touchTarget;
> >   RawPtrWillBeMember<LocalFrame> targetFrame;  // BTW, is it OK to use a raw
> > pointer to LocalFrame?
> > };
> 
> Done, but kept the pointer to LocalFrame as is because Document::frame()
returns
> a raw pointer.

It ought to work fine as suggested; could you explain what issue you ran into?

[mustaq, 2015-06-23 16:36:27]

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:321: using TouchInfo = struct {
On 2015/06/23 15:57:34, sof wrote:
> On 2015/06/23 15:25:51, mustaq wrote:
> > On 2015/06/22 23:40:35, haraken wrote:
> > > 
> > > class TouchInfo {
> > >   ALLOW_ONLY_INLINE_ALLOCATION();
> > > public:
> > > 
> > >   DEFINE_INLINE_TRACE()
> > >   {
> > >     visitor->trace(touchTarget);
> > >     visitor->trace(targetFrame);
> > >   }
> > > 
> > >   ...;
> > >   RefPtrWillBeMember<EventTarget> touchTarget;
> > >   RawPtrWillBeMember<LocalFrame> targetFrame;  // BTW, is it OK to use a
raw
> > > pointer to LocalFrame?
> > > };
> > 
> > Done, but kept the pointer to LocalFrame as is because Document::frame()
> returns
> > a raw pointer.
> 
> It ought to work fine as suggested; could you explain what issue you ran into?

I kept the LocalFrame* as is because from this piece of code can't control the
life of the object accessible only through a raw pointer. Not an issue really,
just thought it doesn't make sense to use a ref-counted type here.


In case my previous comments were confusing: the oilpan compile issue I
mentioned is not related to this. It is related only to the use of
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS.

[sof, 2015-06-23 18:52:01]

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

[sof, 2015-06-23 18:52:02]

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:321: using TouchInfo = struct {
On 2015/06/23 16:36:27, mustaq wrote:
> On 2015/06/23 15:57:34, sof wrote:
> > On 2015/06/23 15:25:51, mustaq wrote:
> > > On 2015/06/22 23:40:35, haraken wrote:
> > > > 
> > > > class TouchInfo {
> > > >   ALLOW_ONLY_INLINE_ALLOCATION();
> > > > public:
> > > > 
> > > >   DEFINE_INLINE_TRACE()
> > > >   {
> > > >     visitor->trace(touchTarget);
> > > >     visitor->trace(targetFrame);
> > > >   }
> > > > 
> > > >   ...;
> > > >   RefPtrWillBeMember<EventTarget> touchTarget;
> > > >   RawPtrWillBeMember<LocalFrame> targetFrame;  // BTW, is it OK to use a
> raw
> > > > pointer to LocalFrame?
> > > > };
> > > 
> > > Done, but kept the pointer to LocalFrame as is because Document::frame()
> > returns
> > > a raw pointer.
> > 
> > It ought to work fine as suggested; could you explain what issue you ran
into?
> 
> I kept the LocalFrame* as is because from this piece of code can't control the
> life of the object accessible only through a raw pointer. Not an issue really,
> just thought it doesn't make sense to use a ref-counted type here.
> 

Yes, it probably isn't worth adding a RefPtr<> here now -- the above suggested
using RawPtrWillBeMember<LocalFrame>.

> 
> In case my previous comments were confusing: the oilpan compile issue I
> mentioned is not related to this. It is related only to the use of
> WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS.

The trait declaration is correct, how did you work out how to address it? (Just
interested to know if we can improve error reporting for the times you need a
trait decl like this.)

[mustaq, 2015-06-24 14:40:25]

Patchset #5 (id:80001) has been deleted

[mustaq, 2015-06-24 14:40:32]

Patchset #4 (id:60001) has been deleted

[mustaq, 2015-06-24 14:47:52]

ptal, addressed all comments.

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.h:321: using TouchInfo = struct {
On 2015/06/23 18:52:02, sof wrote:
> On 2015/06/23 16:36:27, mustaq wrote:
> > On 2015/06/23 15:57:34, sof wrote:
> > > On 2015/06/23 15:25:51, mustaq wrote:
> > > > On 2015/06/22 23:40:35, haraken wrote:
> > > > > 
> > > > > class TouchInfo {
> > > > >   ALLOW_ONLY_INLINE_ALLOCATION();
> > > > > public:
> > > > > 
> > > > >   DEFINE_INLINE_TRACE()
> > > > >   {
> > > > >     visitor->trace(touchTarget);
> > > > >     visitor->trace(targetFrame);
> > > > >   }
> > > > > 
> > > > >   ...;
> > > > >   RefPtrWillBeMember<EventTarget> touchTarget;
> > > > >   RawPtrWillBeMember<LocalFrame> targetFrame;  // BTW, is it OK to use
a
> > raw
> > > > > pointer to LocalFrame?
> > > > > };
> > > > 
> > > > Done, but kept the pointer to LocalFrame as is because Document::frame()
> > > returns
> > > > a raw pointer.
> > > 
> > > It ought to work fine as suggested; could you explain what issue you ran
> into?
> > 
> > I kept the LocalFrame* as is because from this piece of code can't control
the
> > life of the object accessible only through a raw pointer. Not an issue
really,
> > just thought it doesn't make sense to use a ref-counted type here.
> > 
> 
> Yes, it probably isn't worth adding a RefPtr<> here now -- the above suggested
> using RawPtrWillBeMember<LocalFrame>.
> 

Okay, switched away from raw ptr for LocalFrame.

> > 
> > In case my previous comments were confusing: the oilpan compile issue I
> > mentioned is not related to this. It is related only to the use of
> > WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS.
> 
> The trait declaration is correct, how did you work out how to address it?
(Just
> interested to know if we can improve error reporting for the times you need a
> trait decl like this.)
> 

The compile-time error itself wasn't very useful, it was saying something around
memset() I think. I skimmed through the following page for a clue, and
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS seemed relevant. I think adding the error
message in this page would be helpful.

https://www.chromium.org/blink/blink-gc#TOC-Collections-and-ALLOW_ONLY_INLINE...

[sof, 2015-06-25 05:15:43]

lgtm.

thanks for doing the Oilpan iterations, appreciated.

[sof, 2015-06-25 09:05:04]

On 2015/06/24 14:47:52, mustaq wrote:
> ptal, addressed all comments.
> 
>
https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
> File Source/core/input/EventHandler.h (right):
> 
>
https://codereview.chromium.org/1198193005/diff/20001/Source/core/input/Event...
> Source/core/input/EventHandler.h:321: using TouchInfo = struct {
> On 2015/06/23 18:52:02, sof wrote:
> > On 2015/06/23 16:36:27, mustaq wrote:
> > > On 2015/06/23 15:57:34, sof wrote:
> > > > On 2015/06/23 15:25:51, mustaq wrote:
> > > > > On 2015/06/22 23:40:35, haraken wrote:
> > > > > > 
> > > > > > class TouchInfo {
> > > > > >   ALLOW_ONLY_INLINE_ALLOCATION();
> > > > > > public:
> > > > > > 
> > > > > >   DEFINE_INLINE_TRACE()
> > > > > >   {
> > > > > >     visitor->trace(touchTarget);
> > > > > >     visitor->trace(targetFrame);
> > > > > >   }
> > > > > > 
> > > > > >   ...;
> > > > > >   RefPtrWillBeMember<EventTarget> touchTarget;
> > > > > >   RawPtrWillBeMember<LocalFrame> targetFrame;  // BTW, is it OK to
use
> a
> > > raw
> > > > > > pointer to LocalFrame?
> > > > > > };
> > > > > 
> > > > > Done, but kept the pointer to LocalFrame as is because
Document::frame()
> > > > returns
> > > > > a raw pointer.
> > > > 
> > > > It ought to work fine as suggested; could you explain what issue you ran
> > into?
> > > 
> > > I kept the LocalFrame* as is because from this piece of code can't control
> the
> > > life of the object accessible only through a raw pointer. Not an issue
> really,
> > > just thought it doesn't make sense to use a ref-counted type here.
> > > 
> > 
> > Yes, it probably isn't worth adding a RefPtr<> here now -- the above
suggested
> > using RawPtrWillBeMember<LocalFrame>.
> > 
> 
> Okay, switched away from raw ptr for LocalFrame.
> 
> > > 
> > > In case my previous comments were confusing: the oilpan compile issue I
> > > mentioned is not related to this. It is related only to the use of
> > > WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS.
> > 
> > The trait declaration is correct, how did you work out how to address it?
> (Just
> > interested to know if we can improve error reporting for the times you need
a
> > trait decl like this.)
> > 
> 
> The compile-time error itself wasn't very useful, it was saying something
around
> memset() I think. I skimmed through the following page for a clue, and
> WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS seemed relevant. I think adding the error
> message in this page would be helpful.
> 
>
https://www.chromium.org/blink/blink-gc#TOC-Collections-and-ALLOW_ONLY_INLINE...

yes, thanks for the suggestion. Added a mention of the error message.

[haraken, 2015-06-25 09:15:42]

LGTM

[mustaq, 2015-06-25 13:35:08]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2015-06-25 13:35:33]

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

[commit-bot: I haz the power, 2015-06-25 14:55:13]

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