In http://trac.webkit.org/changeset/108152, I fixed a bug whose root cause was that Node::inDocument() did not match the true in-document state. That is, Node::inDocument was true when a Node was not part of the Document tree. After fixing that bug, I tried adding an assertion to inDocument() to ensure it would always return the "correct" value. But it turns out that many callers of inDocument() actually depend on this out-of-sync behavior: those that are called from overrides of Node::removedFromDocument. Essentially what these callers are attempting to determine is, "was this node in the document". What I propose is that we should make the InDocumentFlag state of a node match its true state as closely as possible; ideally, clearing the flag as soon as its parent is set to 0. In order to support the "was this node in the document" use case, we would then need some other mechanism. What this should be is less obvious to me. One idea would be for such callers to override Node::willRemove, and to fix willRemove to happen just before the node is actually removed from the tree (i.e., not calling it too early). This would work well for one of the main offenders I ran into, radio buttons. See HTMLInputElement::removedFromDocument(). A more modest but more invasive approach would be to force any overrides of removedFromDocument to pass along with "we are being removed from the document" state to all methods it calls, rather than letting them use the inDocument flag as a proxy. Or we could add a IsBeingRemovedFromDocument flag. I'm interested in the insights of those with more history on the InDocumentFlag, the removedFromDocument & insertedIntoDocument methods, and the willRemove method.
Another complication I wanted to mention: during destruction, the flag also tends to be wrong. It would be nice to fix this as well, though for now, my assert just ignores the bad state if m_deletionHasBegun is true.
We need to make all node tree manipulation go through only a few functions, and make sure those are audited and do the right thing. With so much node tree code throughout WebCore, I'm sure lots of palces get this sort of thing interestingly wrong. :)
(In reply to comment #1) > Another complication I wanted to mention: during destruction, the flag also tends to be wrong. It would be nice to fix this as well, though for now, my assert just ignores the bad state if m_deletionHasBegun is true. It'll be nice to explicitly define where inDocument flag might be wrong, and force clients not to call inDocument() in such cases (maybe some sort of assertion ?)
(In reply to comment #3) > (In reply to comment #1) > > Another complication I wanted to mention: during destruction, the flag also tends to be wrong. It would be nice to fix this as well, though for now, my assert just ignores the bad state if m_deletionHasBegun is true. > > It'll be nice to explicitly define where inDocument flag might be wrong, and force clients not to call inDocument() in such cases (maybe some sort of assertion ?) Alternatively, we could try to make ContainerNodeAlgorithms.h actually set the flag appropriately. so it's never wrong.
(In reply to comment #4) > Alternatively, we could try to make ContainerNodeAlgorithms.h actually set the flag appropriately. so it's never wrong. Well, we can't really make them "never wrong" while updating pointers but updating the flag on time is definitely better than any other solutions.