429175 - Mutating DOM with aNotify=true while unsafe to run script

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

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

Filing this as a tracker as the fixes will be small so we can probably fix most of them with a patch or two.

We have several callsites that mutate non-native-anonymous DOM with aNotify=true while it's unsafe to execute script.

Currently there is code in trunk that will refuse to fire the mutation event, and assert if someone was listening for it. However if no-one listens we don't assert, but that doesn't mean we could potentially be masking problems once someone, webpage or chrome, do listen.


While loading chrome we sync-load XBL files. Parsing these XBL files notify when inserting the docelement into the document.


nsProgressMeterFrame::AttributeChanged calls SetAttr. The offending lines are:
    barChild->GetContent()->SetAttr(kNameSpaceID_None, nsGkAtoms::flex, leftFlex, PR_TRUE);
    remainderContent->SetAttr(kNameSpaceID_None, nsGkAtoms::flex, rightFlex, PR_TRUE);


nsCSSFrameConstructor::CreateGeneratedFrameFor calls SetText. The problem here is that it does so before calling SetNativeAnonymous(). I don't actually even see why it sets aNotify to true. The offending line is:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1472&root=/cvsroot#2197


Initing plantext editors fires like mad. The problem seems to be that we're mutating (specifically setting attributes) on nodes with aNotify=true before inserting the nodes into the native-anon tree.

Comment 1

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

Attachment: Patch to fix

Not sure if we want to take this for 1.9 or not. It doesn't seem to fix any serious issues, possibly other than the progressmeter one.

However I do seem to be able to run the browser without asserting about this stuff at all with this which is very comforting.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The CSSFrameConstructor thing used to use the DOM API long ago.  When it was switched to SetText, the notification was preserved.  There's no reason for it to notify.

Comment 3

[Jesse Ruderman]

Attachment: "Want to fire" assertions on startup

I get this assertion twice on startup.  Stacks attached.

###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file /Users/jruderman/trunk/mozilla/content/base/src/nsContentUtils.cpp, line 3201

Comment 4

[Jesse Ruderman]

I also get the assertion four times when I load

data:text/html,<input type="file">

Comment 5

[Brandon Sterne (:bsterne)]

Jesse, what sg: rating do you think this one deserves?

Comment 6

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

let's do crit? for now, none of the places I found were problems iirc, but there probably are some

Comment 7

[Boris Zbarsky [:bzbarsky]]

Is this still relevant?  In particular the CSS Frame constructor part?  Jonas?

Comment 8

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

Looks like the nsCSSFrameCtor parts are out-of-date. But I still think we should land the rest of this patch. Will try to merge and land soon

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jonas, are you going to be able to work on this soon?

Comment 10

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

Ugh, had completely forgotten about this. I'm unlikely to have time to work on this very soon. If anyone wants to pick it up definitely go for it.

Otherwise I'll get to this before 1.9.3 release for sure. I think it should be marked as a blocker.

Comment 11

[Johnny Stenback (:jst)]

Blocking per Jonas' comment above.

Comment 12

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

ok, should pick this one up soon. ETA end of next week.

Comment 13

[Damon Sicore (:damons)]

Is this really an sg:crit?

Comment 14

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

IIRC it's more likely than not that it is. And it definitely prevents us from adding assertions that we can use to find/prevent other sg:crit bugs.

Comment 15

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

Attachment: Updated patch

Updated patch. This lets us start up the browser without asserting, I've pushed a version with printf to tryserver that should detect if any of our tests trigger the condition.

Comment 16

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

The attached patch appears to mostly work. There are a couple of cases related to cloning attribute nodes (!!) that still trigger the assertion. I believe those callsites are in fact exploitable so sg:crit is appropriate here.

Comment 17

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

Attachment: Patch to fix

This fixes the bad callsites I can find.

The last holdout was the element-cloning code. When cloning elements we also clone any attribute nodes attached to the element. This so that we trigger any UserDataHandlers attach to such attribute nodes and let them know that the node they are attached to is getting cloned. In the process of doing that we call SetAttributeNode on the newly created element, which notifies and trigger the assertion.

However, it's extremely unlikely that anyone cares about this. UserData is rarely used on the web. As is attribute nodes. And so having user data on an attribute node is extremely unlikely that anyone is depending on.

The alternative way of fixing this would be to add a bunch of code to nsDOMAttributeMap to allow adding attribute nodes to an element without notifying.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 441685 [details] [diff] [review]
Patch to fix

>+    // Really, we should just remove support for attribute nodes.

No need for this comment.

>-  if (elem) {
>-    // aNode's attributes.
>-    const nsDOMAttributeMap *map = elem->GetAttributeMap();
>-    if (map) {
>-      // If we're cloning we need to insert the cloned attribute nodes into the
>-      // cloned element. We assume that the clone of an nsGenericElement is also
>-      // an nsGenericElement.

You should at least comment here that we're not following the spec. This should probably be documented somewhere.

>diff --git a/editor/libeditor/base/nsEditor.h b/editor/libeditor/base/nsEditor.h

> #define kMOZEditorBogusNodeAttr NS_LITERAL_STRING("_moz_editor_bogus_node")
>+#define kMOZEditorBogusNodeAttrAtom nsEditProperty::mozEditorBogusNode

Make IsMozEditorBogusNode use GetAttr and remove kMOZEditorBogusNodeAttr.

>diff --git a/layout/forms/nsTextControlFrame.cpp b/layout/forms/nsTextControlFrame.cpp

>     if (value.IsEmpty()) {
>-      ShowPlaceholder();
>+      SetPlaceholderClass(PR_TRUE, aNotify);
>     } else {
>-      HidePlaceholder();
>+      SetPlaceholderClass(PR_FALSE, aNotify);
>     }

    SetPlaceholderClass(value.IsEmpty(), aNotify);

Comment 19

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

Checked in.

http://hg.mozilla.org/mozilla-central/rev/02ca6f9215bc

I think we should backport removing support for the UserDataHandler on attribute nodes as that was the only place I found that actually seemed exploitable.