Last Comment Bug 382754 - Unsafe DOM mutation events fired in presentation code on 1.8 and 1.8.0
: Unsafe DOM mutation events fired in presentation code on 1.8 and 1.8.0
[sg:critical?] keep private until 355...
: fixed1.8.0.13, fixed1.8.1.5
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Vlad Sukhoy
Depends on: 382681 388563
Blocks: 355548 378963
  Show dependency treegraph
Reported: 2007-05-31 20:46 PDT by Vlad Sukhoy
Modified: 2012-12-03 22:06 PST (History)
16 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

No mutation if aNotify is false. (17.13 KB, patch)
2007-06-01 16:53 PDT, Vlad Sukhoy
jonas: review-
Details | Diff | Splinter Review
No mutation if aNotify is false rev2. -w (13.61 KB, patch)
2007-06-11 20:13 PDT, Vlad Sukhoy
jonas: review+
jonas: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review
No mutation if aNotify is false rev2. (16.67 KB, patch)
2007-06-11 20:55 PDT, Vlad Sukhoy
no flags Details | Diff | Splinter Review

Description Vlad Sukhoy 2007-05-31 20:46:27 PDT
Content/Layout on 1.8 and 1.8.0 fire DOM mutation events on any call to tree manipulation methods, that is nsGenericElement::SetAttr, nsGenericElement::UnsetAttr, nsGenericElement::AppendChildTo, nsGenericElement::RemoveChildAt even when aNotify is false.

When these methods are used from within frame code in many instances the JavaScript mutation events are triggered while presentation tree is in an inconsistent state, it is unlikely that the frame code developers were aware of such complications at the time of writing, therefore each call site of the above within layout is a potential security risk. In fact, bug 382568, bug 382681, bug 382700 were all found after a surface screening of some of call sites of the above within frame code.

See the linked bugs for some specific examples of the issue.

Also see the discussion in bug 355548.
Comment 1 Jonas Sicking (:sicking) PTO Until July 5th 2007-05-31 22:55:11 PDT
In bug 90983 I made us not send these events when aNotify is false. I don't see a reason we couldn't do this on branch as well.
Comment 2 Vlad Sukhoy 2007-05-31 22:58:17 PDT
bz has some reasons though, see bug 355548 comment 41 .
Comment 3 Boris Zbarsky [:bz] 2007-05-31 23:33:52 PDT
If we don't break any of the branch code and any of the various Mozilla-based apps and binary extensions we promised compat with on branch, go for it.  Someone needs to do that testing, though.
Comment 4 Olli Pettay [:smaug] 2007-06-01 01:00:20 PDT
Not all SetAttr calls in layout are unsafe. Worst are those which 
happen during layout, but most others, if handled carefully unfortunately this isn't always the case), are ok.
If the DOM mutation happens in native anonymous, we could stop
propagating those mutation events to non-anon-content (like we do
in trunk).
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2007-06-01 01:57:53 PDT
If we go down that route then for native-anonymous content we could simply avoid firing the event at all. I.e. we wouldn't fire if aNotify is false and it's a native-anonymous node.
Comment 6 Vlad Sukhoy 2007-06-01 16:53:53 PDT
Created attachment 266968 [details] [diff] [review]
No mutation if aNotify is false.

Added revised aNotify patch. The issue with native-anonymous content is indeed separate, testcase in bug 382568 still crashes (perhaps, another metabug for that?).
Comment 7 Vlad Sukhoy 2007-06-01 23:24:38 PDT
...bugzilla newbie... sorry (
Comment 8 Vlad Sukhoy 2007-06-11 00:08:00 PDT
@Boris/Jonas - I'm wondering if you guys could take a glance at that patch for aNotify i.e. attachment 266968 [details] [diff] [review].. The issue is still around and then there is native-anonymous stuff.. Firefox 2 will be around for a while and it is vulnerable to this kind of thing. If the patch is ok then someone is needed to test all sorts of stuff that could be broken due to it, if the patch is not ok then I'll write another..
Comment 9 Boris Zbarsky [:bz] 2007-06-11 00:10:19 PDT
I'm probably not going to be able to look at this for at least several weeks, unfortunately.  :(
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2007-06-11 17:43:40 PDT
Comment on attachment 266968 [details] [diff] [review]
No mutation if aNotify is false.

>Index: base/src/nsGenericElement.cpp
>@@ -2827,41 +2827,40 @@ doInsertChildAt(nsIContent* aKid, PRUint
>   // The kid may have removed its parent from the document, so recheck that
>   // that's still in the document before proceeding.  Also, the kid may have
>   // just removed itself, in which case we don't really want to fire
>   // ContentAppended or a mutation event.
>   // XXXbz What if the kid just moved us in the document?  Scripts suck.  We
>   // really need to stop running them while we're in the middle of modifying
>   // the DOM....
>-  if (aDocument && aKid->GetCurrentDoc() == aDocument &&
>+  // XXX bug 382754: not fire mutation events if aNotify is false.
>+  if (aNotify && aDocument && aKid->GetCurrentDoc() == aDocument &&
>       (!aParent || aKid->GetParent() == aParent)) {
>-    if (aNotify) {
>+    if (aParent) {
>       // Note that we always want to call ContentInserted when things ar
>       // as kids to documents
>-      if (aParent && isAppend) {
>+      if (isAppend) {
>         aDocument->ContentAppended(aParent, aIndex);
>       } else {
>         aDocument->ContentInserted(aParent, aKid, aIndex);
>       }

This looks wrong, you'll only call ContentInserted if aParent is non-null, that wasn't the case in the old code.

Also, please attach a patch diffed with -w to ease review of the patch.
Comment 11 Vlad Sukhoy 2007-06-11 18:06:05 PDT
Comment 12 Vlad Sukhoy 2007-06-11 20:13:45 PDT
Created attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

Ok, here we go.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2007-06-11 20:43:33 PDT
Sorry, I wasn't clear enough, when attaching a -w patch, always attach a version without it so that the reviewer can check that whitespace is done correctly.
Comment 14 Vlad Sukhoy 2007-06-11 20:47:53 PDT
Ahm, ok. I'm also editing so that other people wont repeat my stupid mistakes..
Comment 15 Vlad Sukhoy 2007-06-11 20:55:31 PDT
Created attachment 268048 [details] [diff] [review]
No mutation if aNotify is false rev2.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2007-06-12 15:02:52 PDT
Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

Looks good.

For future reference though, for a branch patch like this it might be worth creating the simplest possible patch, even though that doesn't necessarily produce the neatest code. I would probably simply have added |aNotify| arguments before every |HasMutationListeners| call rather than to try to consolidate if-statements and the like.

r/sr=me with the patch as it is though.
Comment 17 Vlad Sukhoy 2007-06-12 15:25:28 PDT
Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

Now, per comment 3 this should be tested with at least most common plugin/websites... bug 355548 comment 50 implies that there is a prior experience of doing this kind of thing, well, it seems that whatever was done can be repeated.
Comment 18 Vlad Sukhoy 2007-06-19 12:21:43 PDT
I don't have access to bug 378963. Is it related to the issue with unsafe DOM events?
Comment 19 Jonas Sicking (:sicking) PTO Until July 5th 2007-06-19 13:14:02 PDT
yes, it's basically a dup of this one
Comment 20 Daniel Veditz [:dveditz] 2007-06-22 10:55:20 PDT
Does this patch *fix* the "depends on" bugs (355548, 382568, 382681, 382700)? if so it would be better marked as "blocking" those. If not, in what way does this patch depend on those currently-unfixed bugs?
Comment 21 Vlad Sukhoy 2007-06-22 11:31:46 PDT
For bug 355548 it does indeed fix the testcase on the bug, i.e. attachment 266332 [details] no longer crashes branches. I suspect there is still some way to crash MathML frame style reresolution code there and also on trunk, in particular - see bug 378146 for an example of debug-only crash on trunk, but this patch does fix the mutation event vector there.
For others, disabling native-anonymous content as it is done in attachment 267939 [details] [diff] [review] should do.

Note that there are more vulnerable call-sites with aNotify=false in MathML code, see e.g. bug 355548 comment 40. This patch fixes those as well. Also there suspicious call-sites with aNotify=false within XUL.
Comment 22 Vlad Sukhoy 2007-06-22 11:54:29 PDT
Adjusted the blocking chain to reflect the current idea of what fixes what here.
Comment 23 Daniel Veditz [:dveditz] 2007-06-25 10:46:26 PDT
Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

approved for, a=dveditz for release-drivers
Comment 24 Damon Sicore (:damons) 2007-07-06 16:57:17 PDT
Who's landing this?
Comment 25 Vlad Sukhoy 2007-07-06 17:27:54 PDT
I mailed Jonas on July 1st about committing the patch..
Comment 26 Jonas Sicking (:sicking) PTO Until July 5th 2007-07-06 17:37:15 PDT
The best place to find someone to check it in is to go on #developers.
Comment 27 Daniel Veditz [:dveditz] 2007-07-06 18:19:58 PDT
Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

approved for, a=dveditz
Comment 28 Vlad Sukhoy 2007-07-06 18:31:36 PDT
Checked in on branches.

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