Forbid event dispatch during teardown of a Document/DOM tree
Created attachment 132332 [details] Patch
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.
Created attachment 132360 [details] Patch with motivation
Adding some of my favorite reviewers...
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.
(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?
Created attachment 133944 [details] Moved from ~ContainerNode to removeAllChildren
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?
(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?
Created attachment 133954 [details] Proposed renaming of removeAllChildren
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.
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
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
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.
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.