Comment 36 for bug 256624

Revision history for this message
In , Mozilla+ben (mozilla+ben) wrote :

(In reply to comment #33)
> Hmm. What happens if we have inserted multiple kids at mInsertionPoint? Or
> are we guaranteed to have notified on all but one of them?

Bad times. I suspect the author of the content sink intended to make such a guarantee, but it's implicit at best. Code like the following appears in multiple places:

    if (parent.numFlushed < childCount) {
        notify(parent, child, ...);
        parent.numFlushed = childCount;
    }

This assumes childCount - parent.numFlushed <= 1, since the notification only flushes one child. This assumption probably does warrant investigation, even if it's not directly responsible for the current bug.

> Also, when would the child on the stack not be the one at the insertion point?

As far as I can tell, this would never happen if HTMLContentSink::EndContext called FlushTags, but it doesn't, so it's possible for nodes to get inserted in a new context but notified only after the context is closed. Such nodes won't be on the stack in the old context, but their parents might be.

> Here's what I worry about. The following sequence of events:
>
> 1) Append two kids. Notify on them.
> 2) Insert a kid at position 1 (between the two). Don't notify.
> 3) Append another kid
>
> Can this happen? Or are we guaranteed to notify on the inserted kids if
> mInsertionPoint becomes -1 again?

I share this concern. There may be an argument that two children are never inserted/appended into the same parent without a notification, but we are definitely hosed if that happens, since we don't keep track of enough information to recover. If we want to enforce this invariant explicitly, we have to do it at insertion-time (when we're about to insert/append a child, make sure the parent is fully flushed).