699321 - Consider dispatching NodeWillBeDestroyed or equivalent notification when unlinking an element

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

In particular, unlinking removes all the kids without notifying, so observers that depend on state being consistent until NodeWillBeDestroyed can get all confused., unless I'm missing something.  See bug 620122 comment 23.

_Am_ I missing something?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

NodeWillBeDestroyed does get fired for all the nodes in nsNodeUtils::LastRelease().
It can be called when CC unroots objects, or it can happen during unlink
or outside CC - whenever refcount drops to 0.
It would be wrong to fire it in other cases.

Unlinking does fire ParentChainChanged (via unbind).

So, I'm not sure what kind of notification your thinking about.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> NodeWillBeDestroyed does get fired for all the nodes in nsNodeUtils::LastRelease().

Yes, that's too late.

> Unlinking does fire ParentChainChanged (via unbind).

That's not good enough.  It doesn't fire ContentRemoved on the child nodes, in particular.

If a nodelist Y has node X as a root and X gets unlinked before Y, then it's possible for X to drop its child nodes without notifying Y (no ContentRemoved), then those child nodes to ask Y to walk the DOM while being unbound.  At this point Y expects that those child nodes are still descendants of X and will crash if they're not.

Normally when Y gets NodeWillBeDestroyed it will stop trying to walk the DOM.  But in this case that notification just comes too late.

What Y _really_ needs in this case is either for unlink on X to fire ContentRemoved or for some other notification that X is starting unlink and that Y should therefore stop assuming that DOM state is consistent.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So unlinking should notify ContentRemoved. That sounds the right thing to do, IMO.
(I hope that doesn't get too slow. I'm already worried about nsIMutationObserver notification performance)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> So unlinking should notify ContentRemoved.

I'm really worried by the performance aspects there...

On the other hand, I guess depending on the order the unlink happens in for nodes in the subtree being unlinked, things could end up disconnected before the nodelist's root is unlinked.  So we probably do need to just notify ContentRemoved.  :(  Any ways you see around that?

Comment 5

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.