388563 - setting .innerHtml doesn't fire DOMNodeInserted event

Status: RESOLVED FIXED

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

Description

[u283834]

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

With the update to Firefox 2.0.0.5, 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.

Comment 1

[u283834]

Attachment: Simple test case

Comment 2

[u283834]

This bug just enters with the 2.0.0.5 update I did this morning.

Comment 3

[Adam Guthrie]

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 2.0.0.1. This works fine for on trunk, though.

Comment 4

[u283834]

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.

Comment 5

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

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 2.0.0.5 in this case.
Looks like a regression :(

Comment 6

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

Attachment: testcase for DOMNodeInserted

Comment 7

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

Works in 20070706, not in 20070707.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH+&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-06&maxdate=2007-07-07&cvsroot=%2Fcvsroot
So I'd say bug 382754.

Comment 8

[Vlad Sukhoy]

Just by looking at it...
Most likely the subtle logic somewhere around http://mxr.mozilla.org/mozilla1.8/source/content/base/src/nsGenericElement.cpp#3450 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?

Comment 9

[Vlad Sukhoy]

Attachment: plain DocumentFragment testcase

The same, but for plain DocumentFragment. Works in 2.0.0.4, does not work in 2.0.0.5 for me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Whatever code notifies here should probably also fire the mutation events...

Comment 11

[Brendan Eich [:brendan]]

Need a patch quickly or this will miss the next train.

/be

Comment 12

[Martijn Wargers (dead)]

Or just backout the patch from bug 382754 for the moment?

Comment 13

[Vlad Sukhoy]

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?

Comment 14

[Vlad Sukhoy]

Attachment: patch

nsFragmentObserver::Notify method fires the mutation events now.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273363 [details] [diff] [review]
patch

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.

Comment 16

[Vlad Sukhoy]

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.

Comment 17

[Vlad Sukhoy]

Attachment: patch v2

Document observer notifications fired exactly as before, mutation event targets are collected in a list and processed then.

Comment 18

[Boris Zbarsky [: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.

Comment 19

[Vlad Sukhoy]

(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:

http://mxr.mozilla.org/mozilla1.8/source/content/base/src/nsGenericElement.cpp#2852

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?

Comment 20

[Vlad Sukhoy]

Attachment: patch v3

Pretty much like v2 except passes jst-review.pl.. Some answers for comment 19 questions would help.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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) {
       kidsToNotify.AppendObject(mParent->GetChildAt(i));
     }

Comment 23

[Jonathan]

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.

Comment 24

[Vlad Sukhoy]

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..

Comment 25

[clc4tts]

This bug breaks support for tagged WAI-ARIA live regions as well as untagged live regions using heuristics (such as finance.yahoo.com) in Fire Vox on any page that adds content using innerHTML.

Comment 26

[Scott Creabo]

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 2.0.0.7 update, is Jonas' requested change from comment #22 the only thing preventing approval for a 1.8.1.7 checkin?

Comment 27

[Vlad Sukhoy]

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..

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 29

[Vlad Sukhoy]

Attachment: patch v4

Null check eliminated per comment 22.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 278534 [details] [diff] [review]
patch v4

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 31

[Martijn Wargers (dead)]

Checking in nsGenericElement.cpp;
/cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v  <--  nsGenericElement.
cpp
new revision: 3.395.2.32; previous revision: 3.395.2.31
done

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?

Comment 32

[Vlad Sukhoy]

The patch should work for 1.8.0 I suspect.. I'll check today/tomorrow.

Comment 33

[Vlad Sukhoy]

Comment on attachment 278534 [details] [diff] [review]
patch v4

Well, it applies on 1.8.0 and the bug is gone.

Comment 34

[carglue a t ya hoo do tcom]

Any chance the fix will make it into 1.8.1.8? (i.e., Firefox 2.0.0.8)
I see its currently marked as blocking 1.8.1.8, but it was also previously marked as blocking 1.8.1.7 and had been listed as being fixed in 1.8.1.7, so I was surprised to see it didn't make this last FF 2.0.0.7 cut.  Just curious because I've got people down-rev'd to 2.0.0.4 until this regression is fixed.

Comment 35

[Martijn Wargers (dead)]

It is already in the 1.8.1.8 tree, see comment 31. Also notice the fixed1.8.1 keyword.

Comment 36

[carglue a t ya hoo do tcom]

I guess the 1.8.1 branch checkin missed the 1.8.1.7 branch cut even though it was approved for a 1.8.1.7 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?

Comment 37

[Martijn Wargers (dead)]

1.8.1.7 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. 

Comment 38

[Daniel Veditz [:dveditz]]

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

Comment 39

[juan becerra [:juanb]]

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:1.8.1.8) Gecko/2007100816 Firefox/2.0.0.8

Comment 40

[Boris Zbarsky [:bzbarsky]]

Can we resolve this now?

Comment 41

[Vlad Sukhoy]

I guess so, if nobody cares about 1.8.0 enough to keep it unresolved.