Reset all EventHandler data members in EventHandler::clear()

Reset all EventHandler data members in EventHandler::clear()

All members are reset with the exception of m_frame which is the pointer to
the owner frame. This ensures any residual state is cleared.

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

[majidvp, 2014-10-14 13:26:13]

majidvp@chromium.org changed reviewers:
+ rbyers@chromium.org, zeeshanq@chromium.org

[majidvp, 2014-10-14 13:26:13]

Please review.

[Rick Byers, 2014-10-14 13:45:00]

Duplicating the initialization logic in both the ctor and clear is rather
brittle.  Perhaps we can just re-initialize the member in LocalFrame instead of
having a clear method at all?

If that proves difficult/awkward for some reason then I'm OK with this change
(it's certainly an improvement).  I just really hate the types of bugs that
result from mismatches here (eg. test B fails if run after test A, but not when
run in isolation).

Thanks!
   Rick

[Zeeshan Qureshi, 2014-10-14 15:26:53]

nit: Even though there aren't line length restrictions in blink, it's preferred
to have commit messages wrapped to 80 characters.

[majidvp, 2014-11-04 15:46:35]

On 2014/10/14 13:45:00, Rick Byers (Slow until 11-6) wrote:
> Duplicating the initialization logic in both the ctor and clear is rather
> brittle.  Perhaps we can just re-initialize the member in LocalFrame instead
of
> having a clear method at all?
> 
> If that proves difficult/awkward for some reason then I'm OK with this change
> (it's certainly an improvement).  I just really hate the types of bugs that
> result from mismatches here (eg. test B fails if run after test A, but not
when
> run in isolation).
> 
> Thanks!
>    Rick

Re-initializing the EventHandler member instead of using a clear method is a
good idea but it seems to be trickier than expected. The problem is that in
some situations it is possible for EventHandler to trigger a clear on itself
in the middle of handling an event. In such cases, re-initializing the event
handler result in its premature destruction.

A concrete example of this behavior is captured n this test case:
https://chromium.googlesource.com/chromium/blink/+/m
aster/LayoutTests/fast/events/touch/gesture/gesture-tap-frame-removed.html

The program removes an iframe as a result of the mousedown event which has
been dispatched as part of handling the tap gesture. The iframe removal causes
the event handler to be cleared but it is expected for the tap gesture handler
(|EventHandler::handleGestureTap|) to be able to complete even after event
handler is cleared.

If this is still desired the only solution I can think of is to protect a
reference to self anytime that an event handler can result in a clear (e.g.,
any handler that dispatches another event). I find this to be an awkward
solution. Thoughts?

Majid

[Zeeshan Qureshi, 2014-11-06 21:14:09]

> The program removes an iframe as a result of the mousedown event which has
> been dispatched as part of handling the tap gesture. The iframe removal causes
> the event handler to be cleared but it is expected for the tap gesture handler
> (|EventHandler::handleGestureTap|) to be able to complete even after event
> handler is cleared.
> 
> If this is still desired the only solution I can think of is to protect a
> reference to self anytime that an event handler can result in a clear (e.g.,
> any handler that dispatches another event). I find this to be an awkward
> solution. Thoughts?
> 
> Majid

I think making clear() work properly is a good tradeoff, we can choose to
overhaul
event handler later on if need be.

LGTM

[Rick Byers, 2014-11-07 01:41:39]

On 2014/11/04 15:46:35, majidvp wrote:
> On 2014/10/14 13:45:00, Rick Byers (Slow until 11-6) wrote:
> > Duplicating the initialization logic in both the ctor and clear is rather
> > brittle.  Perhaps we can just re-initialize the member in LocalFrame instead
> of
> > having a clear method at all?
> > 
> > If that proves difficult/awkward for some reason then I'm OK with this
change
> > (it's certainly an improvement).  I just really hate the types of bugs that
> > result from mismatches here (eg. test B fails if run after test A, but not
> when
> > run in isolation).
> > 
> > Thanks!
> >    Rick
> 
> Re-initializing the EventHandler member instead of using a clear method is a
> good idea but it seems to be trickier than expected. The problem is that in
> some situations it is possible for EventHandler to trigger a clear on itself
> in the middle of handling an event. In such cases, re-initializing the event
> handler result in its premature destruction.
> 
> A concrete example of this behavior is captured n this test case:
> https://chromium.googlesource.com/chromium/blink/+/m
> aster/LayoutTests/fast/events/touch/gesture/gesture-tap-frame-removed.html
> 
> The program removes an iframe as a result of the mousedown event which has
> been dispatched as part of handling the tap gesture. The iframe removal causes
> the event handler to be cleared but it is expected for the tap gesture handler
> (|EventHandler::handleGestureTap|) to be able to complete even after event
> handler is cleared.
> 
> If this is still desired the only solution I can think of is to protect a
> reference to self anytime that an event handler can result in a clear (e.g.,
> any handler that dispatches another event). I find this to be an awkward
> solution. Thoughts?

Ah, thanks - that makes sense.  Sorry that didn't occur to me (I believe I wrote
the test you refer to because I introduced a bug where that sort of thing would
crash <grin>).

Ok, I'm fine with PS#2 then, but maybe we should at least add a comment
somewhere (eg. in the field section of the header file) to warn people to update
clear() when adding new fields?

[majidvp, 2014-11-10 15:30:20]

Patchset #3 (id:100001) has been deleted

[majidvp, 2014-11-10 16:06:12]

On 2014/11/07 01:41:39, Rick Byers wrote:
> On 2014/11/04 15:46:35, majidvp wrote:
> > On 2014/10/14 13:45:00, Rick Byers (Slow until 11-6) wrote:
> > > Duplicating the initialization logic in both the ctor and clear is rather
> > > brittle.  Perhaps we can just re-initialize the member in LocalFrame
instead
> > of
> > > having a clear method at all?
> > > 
> > > If that proves difficult/awkward for some reason then I'm OK with this
> change
> > > (it's certainly an improvement).  I just really hate the types of bugs
that
> > > result from mismatches here (eg. test B fails if run after test A, but not
> > when
> > > run in isolation).
> > > 
> > > Thanks!
> > >    Rick
> > 
> > Re-initializing the EventHandler member instead of using a clear method is a
> > good idea but it seems to be trickier than expected. The problem is that in
> > some situations it is possible for EventHandler to trigger a clear on itself
> > in the middle of handling an event. In such cases, re-initializing the event
> > handler result in its premature destruction.
> > 
> > A concrete example of this behavior is captured n this test case:
> > https://chromium.googlesource.com/chromium/blink/+/m
> > aster/LayoutTests/fast/events/touch/gesture/gesture-tap-frame-removed.html
> > 
> > The program removes an iframe as a result of the mousedown event which has
> > been dispatched as part of handling the tap gesture. The iframe removal
causes
> > the event handler to be cleared but it is expected for the tap gesture
handler
> > (|EventHandler::handleGestureTap|) to be able to complete even after event
> > handler is cleared.
> > 
> > If this is still desired the only solution I can think of is to protect a
> > reference to self anytime that an event handler can result in a clear (e.g.,
> > any handler that dispatches another event). I find this to be an awkward
> > solution. Thoughts?
> 
> Ah, thanks - that makes sense.  Sorry that didn't occur to me (I believe I
wrote
> the test you refer to because I introduced a bug where that sort of thing
would
> crash <grin>).
> 
> Ok, I'm fine with PS#2 then, but maybe we should at least add a comment
> somewhere (eg. in the field section of the header file) to warn people to
update
> clear() when adding new fields?

Added a comment noting that new fields must be cleared.

[Rick Byers, 2014-11-10 18:04:08]

LGTM, thanks!

FWIW (In case we haven't discussed testing yet), I don't think it's worth trying
to reverse engineer test cases for this.  They'd be different per field,
probably subtle and hard to derive.  But most importantly, the point of this CL
isn't to fix bugs - it's to reduce the chance of future bugs.  So I see it like
a refactoring change, not a bug fix.

[Zeeshan Qureshi, 2014-11-10 18:06:47]

The CQ bit was checked by zeeshanq@chromium.org

[commit-bot: I haz the power, 2014-11-10 18:07:11]

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

[commit-bot: I haz the power, 2014-11-10 21:07:03]

Committed patchset #3 (id:120001) as 185062