Bug 81378 – Forbid event dispatch during teardown of a Document/DOM tree

• Status: NEW
• Component: New Bugs

[Adam Klein]

Forbid event dispatch during teardown of a Document/DOM tree

[Adam Klein]

Created attachment 132332 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 132332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132332&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests, no change in behavior.

Please explain why this change is needed then.

[Adam Klein]

Created attachment 132360 [details]
Patch with motivation

[Adam Klein]

Adding some of my favorite reviewers...

[Ojan Vafai]

Comment on attachment 132360 [details]
Patch with motivation

This looks fine to me, but should we instead put the forbid/allow directly in removeAllChildren? That documents the no event dispatch where the code that assumes it.

[Adam Klein]

(In reply to comment #5)
> (From update of attachment 132360 [details])
> This looks fine to me, but should we instead put the forbid/allow directly in removeAllChildren? That documents the no event dispatch where the code that assumes it.

Well, there's more happening in Document::removedLastRef than just the removeAllChildren...maybe it should go there instead of ~ContainerNode, though?

[Adam Klein]

Created attachment 133944 [details]
Moved from ~ContainerNode to removeAllChildren

[Ryosuke Niwa]

Comment on attachment 133944 [details]
Moved from ~ContainerNode to removeAllChildren

View in context: https://bugs.webkit.org/attachment.cgi?id=133944&action=review

> Source/WebCore/dom/ContainerNode.cpp:84
>  void ContainerNode::removeAllChildren()

Maybe we should rename this function to signify the fact it shouldn't dispatch any events?

[Adam Klein]

(In reply to comment #8)
> (From update of attachment 133944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133944&action=review
> 
> > Source/WebCore/dom/ContainerNode.cpp:84
> >  void ContainerNode::removeAllChildren()
> 
> Maybe we should rename this function to signify the fact it shouldn't dispatch any events?

Any suggestions? I chatted with Ojan about this for awhile and didn't come up with any names I liked better. tearDownSubTree?

[Adam Klein]

Created attachment 133954 [details]
Proposed renaming of removeAllChildren

[Ryosuke Niwa]

Comment on attachment 133954 [details]
Proposed renaming of removeAllChildren

View in context: https://bugs.webkit.org/attachment.cgi?id=133954&action=review

> Source/WebCore/dom/ContainerNode.cpp:87
>      removeAllChildrenInContainer<Node, ContainerNode>(this);

Should we also rename removeAllChildrenInContainer?

> Source/WebCore/dom/ContainerNode.h:67
> +    // Destroys the entire subtree without dispatching any events.

We wouldn't need this comment if we added WithoutEvents to the function name.

> Source/WebCore/dom/ContainerNode.h:68
> +    // Meant to be called only during teardown (e.g., Document::~Document) or by the parser.

This comment seems to repeat what the code (~Document and parser) says.
Also, the word "destroy" makes it pretty clear that we shouldn't be calling this function without a care.

[Early Warning System Bot]

Comment on attachment 133954 [details]
Proposed renaming of removeAllChildren

Attachment 133954 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12143253

[Early Warning System Bot]

Comment on attachment 133954 [details]
Proposed renaming of removeAllChildren

Attachment 133954 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12142251

[Eric Seidel (no email)]

Comment on attachment 133954 [details]
Proposed renaming of removeAllChildren

I think this is a very good idea.  But requires more careful thought/review than I have time for at the moment.

[Adam Klein]

Comment on attachment 133954 [details]
Proposed renaming of removeAllChildren

This is still on my radar (I've got a local git branch of it), but needs some fixing before it goes back up for review. I'll take it out of the queue for clarity's sake.