Always remove registered handlers for deleted node

Always remove registered handlers for deleted node

When a Node is about to get deleted from the Document, we should always
remove any event handlers it has registered to the EventHandlerRegistry
-- regardless of whether the Node has EventTargetData or not. This is
because native code may register nodes as having handlers without
actually inserting any handlers in in the node's event target data.

BUG=393312, 393154
TEST=WebViewTest.DeleteElementWithRegisteredHandler

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

[Rick Byers, 2014-07-16 16:42:29]

How does this change effect the behavior when a touched node is temporarily
removed from a document and re-added on touchend (as might happen in
drag-and-drop scenarios).  Does the event handler get re-registered when the
node returns to the document?  Can you please extend your test case to cover
that?

[Sami, 2014-07-16 17:00:28]

On 2014/07/16 16:42:29, Rick Byers wrote:
> How does this change effect the behavior when a touched node is temporarily
> removed from a document and re-added on touchend (as might happen in
> drag-and-drop scenarios).  Does the event handler get re-registered when the
> node returns to the document?  Can you please extend your test case to cover
> that?

This patch only changes things for nodes that are about to get deleted
(Node::willBeDeletedFromDocument is only called from destructors), and there's
no way for that same node to come back to the document.

For things like drag and drop we instead attach and detach nodes and migrate
their handlers when necessary. I think that path has good coverage in
https://codereview.chromium.org/225903009/ (once it lands again), so we probably
don't do duplicate those tests here.

Note that there is a subtle corner case which I don't have good solution for:
1. Register a node as having an event handler like we do here with
didAddEventHandler (*not* from JS).
2. Detach that node => all event handlers are unregistered.
3. Attach it again => no event handlers will get re-registered, because the
node's event target data only contains handlers from JS.

In other words, you can't migrate nodes with non-JS handlers between two
FrameHosts. The reason is that the only knowledge about those handlers was
cleared in step 2. The good news is that I haven't seen any code that does this.

It feels like the way to fix this is to get rid of the backdoor of registering
handlers directly without populating EventTargetData in the first place.

In any case, I'm hoping we can land this fix and address that problem separately
since this fixes a crash in the wild.

[Rick Byers, 2014-07-16 21:15:01]

On 2014/07/16 17:00:28, Sami wrote:
> On 2014/07/16 16:42:29, Rick Byers wrote:
> > How does this change effect the behavior when a touched node is temporarily
> > removed from a document and re-added on touchend (as might happen in
> > drag-and-drop scenarios).  Does the event handler get re-registered when the
> > node returns to the document?  Can you please extend your test case to cover
> > that?
> 
> This patch only changes things for nodes that are about to get deleted
> (Node::willBeDeletedFromDocument is only called from destructors), and there's
> no way for that same node to come back to the document.
> 
> For things like drag and drop we instead attach and detach nodes and migrate
> their handlers when necessary. I think that path has good coverage in
> https://codereview.chromium.org/225903009/ (once it lands again), so we
probably
> don't do duplicate those tests here.

Thanks for the explanation, that makes sense - sorry for the confusion.  Maybe
worth adding a comment in the test making it clear you're specifically testing
this special case? 

> Note that there is a subtle corner case which I don't have good solution for:
> 1. Register a node as having an event handler like we do here with
> didAddEventHandler (*not* from JS).
> 2. Detach that node => all event handlers are unregistered.
> 3. Attach it again => no event handlers will get re-registered, because the
> node's event target data only contains handlers from JS.
> 
> In other words, you can't migrate nodes with non-JS handlers between two
> FrameHosts. The reason is that the only knowledge about those handlers was
> cleared in step 2. The good news is that I haven't seen any code that does
this.

Yeah the 'fake' handler cases I've seen are pretty rare and special case.  One
unusual example is touch-action (nodes with CSS touch-action have implicit
handlers since they need to compute and send the touch-action on touchstart),
but since we do that as part of style application it'll happen when the node's
style is computed after reattachment and we'll be fine.

> It feels like the way to fix this is to get rid of the backdoor of registering
> handlers directly without populating EventTargetData in the first place.

Agreed.  In many cases we should probably be attaching event handler just like
JS would.

> In any case, I'm hoping we can land this fix and address that problem
separately
> since this fixes a crash in the wild.

Agreed.  LGTM.

[Sami, 2014-07-17 10:24:55]

Thanks Rick. Jochen, can I persuade you to rubberstamp the tests here?

[jochen (gone - plz use gerrit), 2014-07-17 10:48:52]

lgtm

[Sami, 2014-07-17 10:49:32]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-07-17 10:50:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/392283002/40001

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

Change committed as 178366