Last Comment Bug 388563 - setting .innerHtml doesn't fire DOMNodeInserted event
: setting .innerHtml doesn't fire DOMNodeInserted event
Status: RESOLVED FIXED
: regression, verified1.8.1.8
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 1.8 Branch
: x86 Windows XP
-- major with 3 votes (vote)
: ---
Assigned To: Vlad Sukhoy
:
: Andrew Overholt [:overholt]
Mentors:
: 390432 (view as bug list)
Depends on:
Blocks: 382754
  Show dependency treegraph
 
Reported: 2007-07-18 04:54 PDT by Aris
Modified: 2008-01-09 06:42 PST (History)
19 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple test case (434 bytes, text/html)
2007-07-18 05:02 PDT, Aris
no flags Details
testcase for DOMNodeInserted (431 bytes, text/html)
2007-07-19 00:22 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details
plain DocumentFragment testcase (564 bytes, text/html)
2007-07-19 01:29 PDT, Vlad Sukhoy
no flags Details
patch (4.02 KB, patch)
2007-07-22 22:06 PDT, Vlad Sukhoy
bzbarsky: review-
Details | Diff | Splinter Review
patch v2 (2.84 KB, patch)
2007-07-23 00:11 PDT, Vlad Sukhoy
bzbarsky: review-
Details | Diff | Splinter Review
patch v3 (2.88 KB, patch)
2007-07-23 15:44 PDT, Vlad Sukhoy
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
patch v4 (2.81 KB, patch)
2007-08-27 22:03 PDT, Vlad Sukhoy
dveditz: approval1.8.1.8+
vladimir.sukhoy: approval1.8.0.14?
Details | Diff | Splinter Review

Description User image Aris 2007-07-18 04:54:58 PDT
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 User image Aris 2007-07-18 05:02:49 PDT
Created attachment 272770 [details]
Simple test case
Comment 2 User image Aris 2007-07-18 05:06:30 PDT
This bug just enters with the 2.0.0.5 update I did this morning.
Comment 3 User image Adam Guthrie 2007-07-18 11:30:17 PDT
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 User image Aris 2007-07-19 00:01:32 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2007-07-19 00:20:18 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2007-07-19 00:22:28 PDT
Created attachment 272926 [details]
testcase for DOMNodeInserted
Comment 8 User image Vlad Sukhoy 2007-07-19 01:11:16 PDT
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 User image Vlad Sukhoy 2007-07-19 01:29:40 PDT
Created attachment 272933 [details]
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 User image Boris Zbarsky [:bz] (still a bit busy) 2007-07-21 20:30:49 PDT
Whatever code notifies here should probably also fire the mutation events...
Comment 11 User image Brendan Eich [:brendan] 2007-07-21 21:09:41 PDT
Need a patch quickly or this will miss the next train.

/be
Comment 12 User image Martijn Wargers [:mwargers] 2007-07-22 02:53:27 PDT
Or just backout the patch from bug 382754 for the moment?
Comment 13 User image Vlad Sukhoy 2007-07-22 09:06:49 PDT
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 User image Vlad Sukhoy 2007-07-22 22:06:02 PDT
Created attachment 273363 [details] [diff] [review]
patch

nsFragmentObserver::Notify method fires the mutation events now.
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2007-07-22 22:13:04 PDT
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 User image Vlad Sukhoy 2007-07-22 22:47:00 PDT
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 User image Vlad Sukhoy 2007-07-23 00:11:15 PDT
Created attachment 273370 [details] [diff] [review]
patch v2

Document observer notifications fired exactly as before, mutation event targets are collected in a list and processed then.
Comment 18 User image Boris Zbarsky [:bz] (still a bit busy) 2007-07-23 09:29:02 PDT
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 User image Vlad Sukhoy 2007-07-23 13:57:38 PDT
(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 User image Vlad Sukhoy 2007-07-23 15:44:04 PDT
Created attachment 273479 [details] [diff] [review]
patch v3

Pretty much like v2 except passes jst-review.pl.. Some answers for comment 19 questions would help.
Comment 21 User image Adam Guthrie 2007-08-02 14:31:26 PDT
*** Bug 390432 has been marked as a duplicate of this bug. ***
Comment 22 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-08-13 19:12:26 PDT
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 User image Jonathan 2007-08-17 18:59:58 PDT
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 User image Vlad Sukhoy 2007-08-17 19:05:31 PDT
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 User image clc4tts 2007-08-18 02:31:36 PDT
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 User image Scott Creabo 2007-08-26 10:23:17 PDT
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 User image Vlad Sukhoy 2007-08-26 14:13:39 PDT
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 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-08-27 15:30:12 PDT
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 User image Vlad Sukhoy 2007-08-27 22:03:51 PDT
Created attachment 278534 [details] [diff] [review]
patch v4

Null check eliminated per comment 22.
Comment 30 User image Daniel Veditz [:dveditz] 2007-08-29 14:53:49 PDT
Comment on attachment 278534 [details] [diff] [review]
patch v4

approved for 1.8.1.7, a=dveditz for release-drivers
Comment 31 User image Martijn Wargers [:mwargers] 2007-08-29 16:18:29 PDT
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 User image Vlad Sukhoy 2007-08-30 11:10:09 PDT
The patch should work for 1.8.0 I suspect.. I'll check today/tomorrow.
Comment 33 User image Vlad Sukhoy 2007-09-03 23:06:20 PDT
Comment on attachment 278534 [details] [diff] [review]
patch v4

Well, it applies on 1.8.0 and the bug is gone.
Comment 34 User image carglue a t ya hoo do tcom 2007-09-24 15:56:49 PDT
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 User image Martijn Wargers [:mwargers] 2007-09-24 16:00:47 PDT
It is already in the 1.8.1.8 tree, see comment 31. Also notice the fixed1.8.1 keyword.
Comment 36 User image carglue a t ya hoo do tcom 2007-09-24 16:13:52 PDT
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 User image Martijn Wargers [:mwargers] 2007-09-24 16:24:30 PDT
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 User image Daniel Veditz [:dveditz] 2007-09-24 16:53:17 PDT
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 User image juan becerra [:juanb] 2007-10-13 11:18:07 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2007-11-19 19:52:57 PST
Can we resolve this now?
Comment 41 User image Vlad Sukhoy 2007-11-19 19:54:48 PST
I guess so, if nobody cares about 1.8.0 enough to keep it unresolved.

Note You need to log in before you can comment on or make changes to this bug.