Closed Bug 388563 Opened 17 years ago Closed 17 years ago

setting .innerHtml doesn't fire DOMNodeInserted event


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

1.8 Branch
Windows XP
Not set





(Reporter: u283834, Assigned: vladimir.sukhoy)



(Keywords: regression, verified1.8.1.8)


(5 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv: Gecko/20070713 Firefox/
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv: Gecko/20070713 Firefox/

With the update to Firefox, the DOMNodeInserted event (or the DOMSubtreeModified event) doesn't seem to be called when the .innerHtml of an element has changed.

Reproducible: Always

Steps to Reproduce:
1. Attach the DOMNodeInserted or DOMSubtreeModified event to an element.
2. Change the .innerHTML of the element (.innerHTML += "<span />").
Actual Results:  
The element changes, the handler attached is not reached.
Attached file Simple test case
This bug just enters with the update I did this morning.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071605 Minefield/3.0a7pre

Which version did you update from? I see this bug in every dot release of 2.0.0.x going back to This works fine for on trunk, though.
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → 1.8 Branch
Sorry, your comment only holds for the DOMSubtreeModified event (for which I uploaded the test case), which doesn't work either. The problem I had was with the DOMNodeInserted event.
DOMSubtreeModified is not supported in 1.8.x (Firefox 2.x).
DOMSubtreeModified and DOMNodeInserted work fine in 1.9.
DOMNodeInserted doesn't seem to work in in this case.
Looks like a regression :(
Ever confirmed: true
Just by looking at it...
Most likely the subtle logic somewhere around did not expect that mutation events are no longer fired for aNotify == false.

How about if inserting document fragment directly?.. Does that fire DOMNodeInserted still?
The same, but for plain DocumentFragment. Works in, does not work in for me
Flags: blocking1.8.1.6?
Whatever code notifies here should probably also fire the mutation events...
Need a patch quickly or this will miss the next train.

Or just backout the patch from bug 382754 for the moment?
Bug 382754 one fixes exploitable call-sites in MathML (and there are other call-sites which are "suspicious" e.g. in XUL, see bug 382754 discussion).. Maybe we can just make this code fire appropriate mutation events as per comment 10?
Attached patch patch (obsolete) — Splinter Review
nsFragmentObserver::Notify method fires the mutation events now.
Attachment #273363 - Flags: review?(bzbarsky)
Comment on attachment 273363 [details] [diff] [review]

Firing mutation events interleaved with the mutation notifications is a good way to crash when those mutation events change the DOM around.

This code should fire all the document observer notifications (exactly as they were fired before), then iterate the kids and fire mutation events as needed.  Note that as you iterate and fire events the child list can change, so you need to be pretty careful about how you do it.
Attachment #273363 - Flags: review?(bzbarsky) → review-
Ok.., is storing the children we need in a list an option? That is not done in the current 1.8 code, from what I understand. If the child array changes as we traverse it, there is a check for element at position i being null, but no measures against the whole child list being changed are taken. I am not sure about document observer notifications, but mutation event handlers are definitely able to transform the list.
Attached patch patch v2 (obsolete) — Splinter Review
Document observer notifications fired exactly as before, mutation event targets are collected in a list and processed then.
Attachment #273363 - Attachment is obsolete: true
Attachment #273370 - Flags: review?(bzbarsky)
Comment on attachment 273370 [details] [diff] [review]
patch v2

I think the NODEINSERTED check should be on the kid, not the parent.  See what nsGenericElement::doInsertChildAt does....

In any case, sicking is probably the right reviewer for this, not me.
Attachment #273370 - Flags: review?(bzbarsky) → review-
(In reply to comment #18)
> (From update of attachment 273370 [details] [diff] [review])
> I think the NODEINSERTED check should be on the kid, not the parent.  See what
> nsGenericElement::doInsertChildAt does....

I'm not quite sure, but here is what doInsertChildAt does on 1.8 (its a static, not of nsGenericElement) does:

And that's what I did at Notify(). Is it right to copy trunk's behavior in this case at Notify()? If it is, maybe we should do that at 1.8's doInsertChildAt too? If it is not, then is there any other reason the patch is not ok?
Attached patch patch v3Splinter Review
Pretty much like v2 except passes Some answers for comment 19 questions would help.
Attachment #273370 - Attachment is obsolete: true
Attachment #273479 - Flags: review?(jonas)
Comment on attachment 273479 [details] [diff] [review]
patch v3

>+        if (nsGenericElement::HasMutationListeners(mParent, eventBits)) {
>+          // Walk over the child nodes and collect those we will fire
>+          // notifications for
>+          nsCOMArray<nsIContent> kidsToNotify;
>+          for (i = notifySlot; i < boundCount; ++i) {
>+            child = mParent->GetChildAt(i);
>+            if (child) {
>+              kidsToNotify.AppendObject(child);
>+            }
>+          }

No need to null-check |child|. Just do

     for (i = notifySlot; i < boundCount; ++i) {
Attachment #273479 - Flags: superreview+
Attachment #273479 - Flags: review?(jonas)
Attachment #273479 - Flags: review+
Attachment #273479 - Flags: approval1.8.1.7?
Will this be patched for the next release?  I have an app that partially breaks because of this.  I use DOMNodeInserted to trigger updates to related page elements.
Up to drivers, but this is just a fix for regression of security patch, so I don't see a reason why this shouldn't go in..
This bug breaks support for tagged WAI-ARIA live regions as well as untagged live regions using heuristics (such as in Fire Vox on any page that adds content using innerHTML.
This major reqression also breaks extensions including GreaseMonkey, DejaClick and others that need to detect when page content has changed.  With time ticking for the FF update, is Jonas' requested change from comment #22 the only thing preventing approval for a checkin?
It has "r/sr+" on it which means it is ok as far as I understand it. If that change cannot be done at checkin, I will update the patch..
Btw, it never hurts to attach an updated patch when requesting approval (my bad in this case since I was the one that actually did the request). Especially since I'm guessing you're going to have someone else check this in.
Attached patch patch v4Splinter Review
Null check eliminated per comment 22.
Flags: blocking1.8.1.7? → blocking1.8.1.7+
Comment on attachment 278534 [details] [diff] [review]
patch v4

approved for, a=dveditz for release-drivers
Attachment #278534 - Flags: approval1.8.1.7+
Attachment #273479 - Flags: approval1.8.1.7?
Keywords: checkin-needed
Checking in nsGenericElement.cpp;
/cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v  <--  nsGenericElement.
new revision: 3.395.2.32; previous revision: 3.395.2.31

Checked in on the 1.8.1 branch.

I'm a bit confused. The patch from bug 382754 was also checked in on the 1.8.0.x branch, so should this patch not be checked in the 1.8.0.x branch, too?
Assignee: nobody → vladimir.sukhoy
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
The patch should work for 1.8.0 I suspect.. I'll check today/tomorrow.
Comment on attachment 278534 [details] [diff] [review]
patch v4

Well, it applies on 1.8.0 and the bug is gone.
Attachment #278534 - Flags: approval1.8.0.14?
Any chance the fix will make it into (i.e., Firefox
I see its currently marked as blocking, but it was also previously marked as blocking and had been listed as being fixed in, so I was surprised to see it didn't make this last FF cut.  Just curious because I've got people down-rev'd to until this regression is fixed.
Flags: blocking1.8.1.8+
It is already in the tree, see comment 31. Also notice the fixed1.8.1 keyword.
I guess the 1.8.1 branch checkin missed the branch cut even though it was approved for a checkin (see comment #30 and #31). Anyway, Thanks.  Also, it appears I somehow caused the blocking1.8.1.8+ flag to be dropped in my prior post.  Could someone please fix the oops for me? was a fire drill release to fix the quicktime vulnerability, so all other branch fixes got bumped up a version number.
I can't fix the blocking1.8.1.8+ flag, but I don't think it's worth the effort anymore, since the patch already went in there. 
What was there to "fix" about the blocking1.8.1.8+ flag?

carglue: please don't muck with the blocking flags if you're not a release driver, other than to nominate bugs as appropriate
Flags: blocking1.8.1.8+
Verified using testcases in comment #6 and comment #9. Observed only "test1" alert in 2005 and 2007, but I saw "test1" and "test" alerts plus node insertion in 2008rc2:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2007100816 Firefox/
Can we resolve this now?
I guess so, if nobody cares about 1.8.0 enough to keep it unresolved.
Closed: 17 years ago
Resolution: --- → FIXED
Flags: wanted1.8.0.x?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.