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.

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.

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.

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

I also get the assertion four times when I load data:text/html,<input type="file">

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

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

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

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

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

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.

Blocking per Jonas' comment above.

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

Is this really an sg:crit?

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.

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.

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.

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 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);

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.