423355 - "ASSERTION: Unexpected child of document element containing block" involving removing root, mutation listener, and fresh script node

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: Unexpected child of document element containing block: 'mDocElementContainingBlock->GetFirstChild(nsnull) == nsnull', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8962

###!!! ASSERTION: already have a child frame: 'mFrames.IsEmpty()', file /Users/jruderman/trunk/mozilla/layout/generic/nsHTMLFrame.cpp, line 263

Closing or reloading the testcase triggers:

###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 673

The testcase causes a bunch of stuff to leak, including an nsGenericElement.

I don't know whether this is a security hole, but I'm filing as security-sensitive because the last assertion scares me.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is really bad.  We're running script (the contents of that <script> element) during frame construction.  The stack to that, starting with the AppendChild() call on the script, is:

#9  0xb529203a in nsJSContext::EvaluateString (this=0x88ccc18, aScript=@0xbfffbb14, 
    aScopeObject=0xb1ea8840, aPrincipal=0x88e2238, 
    aURL=0x8911618 "file:///home/bzbarsky/test.html", aLineNo=0, aVersion=0, 
    aRetValue=0x0, aIsUndefined=0xbfffba10)
    at ../../../../mozilla/dom/src/base/nsJSEnvironment.cpp:1519
#10 0xb507cea3 in nsScriptLoader::EvaluateScript (this=0x88e21e0, aRequest=0x88f9028, 
    aScript=@0xbfffbb14) at ../../../../mozilla/content/base/src/nsScriptLoader.cpp:620
#11 0xb507c948 in nsScriptLoader::ProcessRequest (this=0x88e21e0, aRequest=0x88f9028)
    at ../../../../mozilla/content/base/src/nsScriptLoader.cpp:538
#12 0xb507d196 in nsScriptLoader::ProcessPendingRequests (this=0x88e21e0)
    at ../../../../mozilla/content/base/src/nsScriptLoader.cpp:671
#13 0xb507f64c in nsRunnableMethod<nsScriptLoader>::Run (this=0x88ee090)
    at ../../../dist/include/xpcom/nsThreadUtils.h:261
#14 0xb4fea7b6 in nsContentUtils::RemoveScriptBlocker ()
    at ../../../../mozilla/content/base/src/nsContentUtils.cpp:4026
#15 0xb5027cd6 in mozAutoDocUpdateContentUnnest::mozAutoDocUpdateContentUnnest ()
    at ../../../dist/include/xpcom/nsAutoPtr.h:1063
#16 0xb501feed in nsDocument::MutationEventDispatched (this=0x8907a40, aTarget=0x88f9108)
    at ../../../../mozilla/content/base/src/nsDocument.cpp:5833
#17 0xb503a58c in mozAutoSubtreeModified::UpdateTarget (this=0xbfffbdd4, 
    aSubtreeOwner=0x0, aTarget=0x0)
    at ../../../../dist/include/content/nsIDocument.h:1129
#18 0xb503a3ae in ~mozAutoSubtreeModified (this=0xbfffbdd4)
    at ../../../dist/include/content/nsIDocument.h:1123
#19 0xb5045210 in nsGenericElement::doInsertChildAt (aKid=0x89053c8, aIndex=0, 
    aNotify=1, aParent=0x88f9108, aDocument=0x8907a40, aChildArray=@0x88f9120)
    at ../../../../mozilla/content/base/src/nsGenericElement.cpp:2765
#20 0xb5044b79 in nsGenericElement::InsertChildAt (this=0x88f9108, aKid=0x89053c8, 
    aIndex=0, aNotify=1)
    at ../../../../mozilla/content/base/src/nsGenericElement.cpp:2677
#21 0xb50478c3 in nsGenericElement::doReplaceOrInsertBefore (aReplace=0, 
    aNewChild=0x89053e4, aRefChild=0x0, aParent=0x88f9108, aDocument=0x8907a40, 
    aReturn=0xbfffc134) at ../../../../mozilla/content/base/src/nsGenericElement.cpp:3402
#22 0xb5045c15 in nsGenericElement::InsertBefore (this=0x88f9108, aNewChild=0x89053e4, 
    aRefChild=0x0, aReturn=0xbfffc134)
    at ../../../../mozilla/content/base/src/nsGenericElement.cpp:2983
#23 0xb5103743 in nsHTMLDivElement::InsertBefore (this=0x88f9108, newChild=0x89053e4, 
    refChild=0x0, _retval=0xbfffc134)
    at ../../../../../mozilla/content/html/content/src/nsHTMLDivElement.cpp:56
#24 0xb53759d1 in InsertElementTxn::DoTransaction (this=0x8905430)
    at ../../../../mozilla/editor/libeditor/base/InsertElementTxn.cpp:111
#25 0xb5350739 in nsEditor::DoTransaction (this=0x88f91d8, aTxn=0x8905430)
    at ../../../../mozilla/editor/libeditor/base/nsEditor.cpp:687
#26 0xb53526fe in nsEditor::InsertNode (this=0x88f91d8, aNode=0x89053e4, 
    aParent=0x88f9124, aPosition=0)
    at ../../../../mozilla/editor/libeditor/base/nsEditor.cpp:1393
#27 0xb534475e in nsTextEditRules::CreateBogusNodeIfNeeded (this=0x8905360, 
    aSelection=0x88f93a0)
    at ../../../../mozilla/editor/libeditor/text/nsTextEditRules.cpp:1272
#28 0xb534031e in nsTextEditRules::Init (this=0x8905360, aEditor=0x88f91d8, aFlags=515)
    at ../../../../mozilla/editor/libeditor/text/nsTextEditRules.cpp:141
#29 0xb5337bac in nsPlaintextEditor::InitRules (this=0x88f91d8)
    at ../../../../mozilla/editor/libeditor/text/nsPlaintextEditor.cpp:344
#30 0xb5336b6c in nsPlaintextEditor::EndEditorInit (this=0x88f91d8)
    at ../../../../mozilla/editor/libeditor/text/nsPlaintextEditor.cpp:180
#31 0xb533fb1e in ~nsAutoEditInitRulesTrigger (this=0xbfffc564)
    at ../../../../mozilla/editor/libeditor/text/nsTextEditUtils.cpp:134
#32 0xb5336a0f in nsPlaintextEditor::Init (this=0x88f91d8, aDoc=0x8907adc, 
    aPresShell=0x88be2e8, aRoot=0x88f9108, aSelCon=0x89052e0, aFlags=515)
    at ../../../../mozilla/editor/libeditor/text/nsPlaintextEditor.cpp:145
#33 0xb4e8e894 in nsTextControlFrame::CreateFrameFor (this=0x88f2cc8, aContent=0x88f9108)
    at ../../../mozilla/layout/forms/nsTextControlFrame.cpp:1463
#34 0xb4d2b914 in nsCSSFrameConstructor::CreateAnonymousFrames (this=0x88be670, 
    aState=@0xbfffcd64, aParent=0x884d0f8, aDocument=0x8907a40, aParentFrame=0x88f2cc8, 
    aAppendToExisting=0, aChildItems=@0xbfffc8fc)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:5769
#35 0xb4d2b686 in nsCSSFrameConstructor::CreateAnonymousFrames (this=0x88be670, 
    aTag=0x8096f20, aState=@0xbfffcd64, aParent=0x884d0f8, aNewFrame=0x88f2cc8, 
    aAppendToExisting=0, aChildItems=@0xbfffc8fc, aIsRoot=0)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:5710
#36 0xb4d2b53d in nsCSSFrameConstructor::ConstructHTMLFrame (this=0x88be670, 
    aState=@0xbfffcd64, aContent=0x884d0f8, aParentFrame=0x88f1ec4, aTag=0x8096f20, 
    aNameSpaceID=0, aStyleContext=0x88f1d6c, aFrameItems=@0xbfffcbfc, aHasPseudoParent=0)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:5650
#37 0xb4d2fdf2 in nsCSSFrameConstructor::ConstructFrameInternal (this=0x88be670, 
    aState=@0xbfffcd64, aContent=0x884d0f8, aParentFrame=0x88f1ec4, aTag=0x8096f20, 
    aNameSpaceID=0, aStyleContext=0x88f1d6c, aFrameItems=@0xbfffcbfc, aXBLBaseTag=0)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:7720
#38 0xb4d2f8a0 in nsCSSFrameConstructor::ConstructFrame (this=0x88be670, 
    aState=@0xbfffcd64, aContent=0x884d0f8, aParentFrame=0x88f1ec4, 
    aFrameItems=@0xbfffcbfc)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:7592
#39 0xb4d382db in nsCSSFrameConstructor::ProcessChildren (this=0x88be670, 
    aState=@0xbfffcd64, aContent=0x89084c0, aFrame=0x88f1ec4, 
    aCanHaveGeneratedContent=1, aFrameItems=@0xbfffcbfc, aParentIsBlock=1)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:11425
#40 0xb4d288ec in nsCSSFrameConstructor::ConstructDocElementFrame (this=0x88be670, 
    aState=@0xbfffcd64, aDocElement=0x89084c0, aParentFrame=0x8904640, 
    aNewFrame=0xbfffce48) at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:4463
#41 0xb4d33281 in nsCSSFrameConstructor::ContentInserted (this=0x88be670, 
    aContainer=0x0, aChild=0x89084c0, aIndexInContainer=1, aFrameState=0x0)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:8950
#42 0xb4d9ee5b in PresShell::ContentInserted (this=0x88be2e8, aDocument=0x8907a40, 
    aContainer=0x0, aChild=0x89084c0, aIndexInContainer=1)
    at ../../../mozilla/layout/base/nsPresShell.cpp:4754
#43 0xb505e3c1 in nsNodeUtils::ContentInserted (aContainer=0x8907a40, aChild=0x89084c0, 
    aIndexInContainer=1) at ../../../../mozilla/content/base/src/nsNodeUtils.cpp:142
#44 0xb504514a in nsGenericElement::doInsertChildAt (aKid=0x89084c0, aIndex=1, 
    aNotify=1, aParent=0x0, aDocument=0x8907a40, aChildArray=@0x8907b4c)
    at ../../../../mozilla/content/base/src/nsGenericElement.cpp:2754
#45 0xb5013d19 in nsDocument::InsertChildAt (this=0x8907a40, aKid=0x89084c0, aIndex=1, 
    aNotify=1) at ../../../../mozilla/content/base/src/nsDocument.cpp:2273
#46 0xb50478c3 in nsGenericElement::doReplaceOrInsertBefore (aReplace=0, 
    aNewChild=0x89084dc, aRefChild=0x0, aParent=0x0, aDocument=0x8907a40, 
    aReturn=0xbfffd410) at ../../../../mozilla/content/base/src/nsGenericElement.cpp:3402
#47 0xb501a4d1 in nsDocument::InsertBefore (this=0x8907a40, aNewChild=0x89084dc, 
    aRefChild=0x0, aReturn=0xbfffd410)
    at ../../../../mozilla/content/base/src/nsDocument.cpp:4097
#48 0xb501a550 in nsDocument::AppendChild (this=0x8907a40, aNewChild=0x89084dc, 
    aReturn=0xbfffd410) at ../../../../mozilla/content/base/src/nsDocument.cpp:4118
#49 0xb51828f9 in nsHTMLDocument::AppendChild (this=0x8907a40, newChild=0x89084dc, 
    _retval=0xbfffd410)
    at ../../../../../mozilla/content/html/document/src/nsHTMLDocument.h:149

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the reason the script runs is because of the mozAutoDocUpdateContentUnnest thing from bug 401155 (which I don't recall from the patches that I reviewed).  But even if we took that out, the mutation listener still runs!  It runs with a target == currentTarget == originalTarget, but the point is that it's running at all.  Which seems pretty bad.

And yes, I know, the real issue here is the editor suck.  We have bugs on that.  They're old...

Perhaps we should run the editor init when we unblock script?  And perhaps we should add a script blocker in the presshell when we call into frame construction, instead of just changing the update nest count?  But if there were still a script blocker around, the mutation event would still fire, right?  It might just assert in the process...

Comment 3

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

Yeah, the mozAutoDocUpdateContentUnnest was something i added last minute to unbreak mochitests.

So is this a regression from bug 401155? Or is this an issue that existed before just in a different form?

We should definitely add scripblockers around sections that mustn't run scripts, but for now I don't want to actually block events, but rather just assert.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so I was wrong.  The mutation listener runs, but _not_ for the mutation event fired during frame construction.  We're safe on that one, since we don't propagate those out of native anonymous content.  So yes, this is a regression from bug 401155.  Now we're running page script before dispatching this mutation event.  Perhaps the real thing to change here is the mutation-event-listener detection code: it really shouldn't walk out of the native anonymous subtree, right?

_And_ we should add script blockers in presshell, probably.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Somewhat simpler testcase that crashes with deleted object access due to this bug

Comment 6

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

Ah, so there are possibly three issues here:

1. The mozAutoDocUpdateContentUnnest unblock the scriptblockers from both
   mutations.
2. The PresShell should be adding a scriptblocker while executing this code.
3. Editor shouldn't be inited until it's safe to run script.

Fixing any one of these three should fix the bug, but ideally all three should get fixed.

Does that sound right?

I'm not sure how to do 1. And it does scare me a bit that we're firing mutation events even though they will get blocked higher up. Maybe we could fix 1 by not firing mutation events ever for native-anonymous content?

Comment 7

[Boris Zbarsky [:bzbarsky]]

If that doesn't break the various editor anon content stuff, or file inputs, etc, etc, then we could do that...

I'd say we should fix #2 for sure.  #3 is not 1.9-type work.

Comment 8

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

None of the editor-internal code should be (and i'm pretty sure isn't) listening to mutation events anyway, so it shouldn't be affected.

Comment 9

[Mike Schroepfer]

+ing given comment 1.  Sicking please adjust priority or assignment as you see fit.

Comment 10

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

Attachment: disable mutation events in native anon content

This fixes the crash, but IMO fix#2 is the right thing to do.

Comment 11

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

I think we should do both. Firing events when it's not safe to run script is scary, even if the events should only fire on native-anonymous content. We have had (do have?) cases where content can reach native-anon content so in general we try to prevent it, but also try to not rely on it.

Comment 12

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

Attachment: Patch to fix (-w)

So the problem was that we ended up nesting two mozAutoDocUpdateContentUnnest. We had 3 scriptblockers in place, two from BeginUpdate(CONTENT) and one from nsPresShell. The first mozAutoDocUpdateContentUnnest just unblocked two blockers and we correctly didn't run the pending script (but we did fire the mutation event). However then we created another mozAutoDocUpdateContentUnnest which unblocked twice again, and so we reached 0 blockers and ran the script.

This patch fixes this so that the inner mozAutoDocUpdateContentUnnest won't do any additional unblocking. It also makes us not fire mutation events if there are blockers still pending, as is the case here.

I also made it only unblock UPDATE_CONTENT_MODEL calls, rather than unblock all BeginUpdates and assert that they were all UPDATE_CONTENT_MODEL. I think there might be cases where we do a UPDATE_CONTENT_STATE, get into frame construction, and then get into an mozAutoDocUpdateContentUnnest possibly.

I do still think that we should take smaugs patch here. Even though in this case my patch stops the event from firing too. But I really don't see why we should ever fire mutation events for native-anon content. It feels like asking for trouble given the mess that mutation events are.

Comment 13

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

Attachment: Patch to fix

With whitespace changes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 313235 [details] [diff] [review]
Patch to fix (-w)

>Index: content/base/public/nsIDocument.h
>+  virtual PRUint32 GetUnnestableUpdateLevel()
>+  virtual void SetUnnestableUpdateLevel(PRUint32 aLevel)

Either these methods or the member should be documented.

For that matter, should the setter be protected, with the update-unnesting class a friend class or something?  That would make it harder to screw up mUnnestableUpdateLevel...

As far as that goes, mUnnestableUpdateLevel sounds like it's something that can't be nested...  Maybe instead of "unnest" we could use "pop"?  So mPoppableUpdateLevel and maybe s/mozAutoDocUpdateContentUnnest/mozAutDocUpdatePopper/?  Not sure what the naming should be like, and I can live with it being a separate bug.

>Index: content/base/src/nsDocument.cpp
>@@ -5866,21 +5855,23 @@ nsDocument::MutationEventDispatched(nsIN

Do we really want to clear mSubtreeModifiedTargets and then not fire the event on any of them?  I guess we really aren't expecting to hit this code, right?  It might be a good idea to assert that IsSafeToRunScript() so we can catch cases when we do hit it...

>Index: content/base/src/nsGenericElement.cpp
>+#include "nsStackWalk.h"

This doesn't seem relevant to this patch.

>   ~mozAutoDocUpdateContentUnnest()
>+    mDocument->SetUnnestableUpdateLevel(mNestingLevel);

Isn't it possible for mDocument to be null here?  The constructor certainly expects that as a possibility....

>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
>+        mObservedDocument->RemoveObserver(this);

Looks reasonable to me, but enn should check it out.

Waiting for the answers to some of the questions above before marking reviews.

Oh, and I absolutely agree that we should take smaug's patch too.

Comment 15

[Neil Deakin]

> >Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
> >+        mObservedDocument->RemoveObserver(this);
> 
> Looks reasonable to me, but enn should check it out.

It doesn't, and the patch that added that condition block doesn't either. Removing observers should only occur if aIsFinal is true, as Uninit is called with aIsFinal=PR_FALSE when rebuilding the template and we don't want the observer disappearing then. aIsFinal=PR_TRUE only if the template is actually going away.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It doesn't, and the patch that added that condition block doesn't either.

Sure.  It just makes sense to RemoveObserver before nulling out mObservedDocument, which is all I could really check there...

I do think the code as written right now will crash if the template builder dies and then the document tries to notify it, right?  This should be easily testable.  The aFinal thing sounds like it should be too.

Do we want a separate bug on this chunk of code?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Actually, the code would have crashed before the change from bug 417840 as well, no?  I wonder whether we just manage to never destroy template builders without destroying the document too...

Comment 18

[Neil Deakin]

My comment was more about making sure that there was a check for aIsFinal == PR_TRUE around the observer removing code.

Comment 19

[Neil Deakin]

XULDocument holds a (cycle collected) reference to any builders used in the document. A builder is removed from the document's list and RemoveObserver is called when the ContentRemoved notification happens.

Comment 20

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

Doh, sorry, the template builder change belongs in bug 408268, so ignore it here please.

Comment 21

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

(In reply to comment #14)
> (From update of attachment 313235 [details] [diff] [review])
> >Index: content/base/public/nsIDocument.h
> >+  virtual PRUint32 GetUnnestableUpdateLevel()
> >+  virtual void SetUnnestableUpdateLevel(PRUint32 aLevel)
> 
> Either these methods or the member should be documented.
> 
> For that matter, should the setter be protected, with the update-unnesting
> class a friend class or something?  That would make it harder to screw up
> mUnnestableUpdateLevel...

Done

> As far as that goes, mUnnestableUpdateLevel sounds like it's something that
> can't be nested...  Maybe instead of "unnest" we could use "pop"?  So
> mPoppableUpdateLevel and maybe
> s/mozAutoDocUpdateContentUnnest/mozAutDocUpdatePopper/?  Not sure what the
> naming should be like, and I can live with it being a separate bug.

I used "Removeable" rather than "Poppable".

> >Index: content/base/src/nsDocument.cpp
> >@@ -5866,21 +5855,23 @@ nsDocument::MutationEventDispatched(nsIN
> 
> Do we really want to clear mSubtreeModifiedTargets and then not fire the event
> on any of them?  I guess we really aren't expecting to hit this code, right? 
> It might be a good idea to assert that IsSafeToRunScript() so we can catch
> cases when we do hit it...

Without smaugs patch in this does happen, but once it has landed I think we should be fine. Eventually I'll want to get to a point where we make nsEventDispatcher::Dispatch assert and early-return if !IsSafeToRunScript()

> >   ~mozAutoDocUpdateContentUnnest()
> >+    mDocument->SetUnnestableUpdateLevel(mNestingLevel);
> 
> Isn't it possible for mDocument to be null here?  The constructor certainly
> expects that as a possibility....

Doh! Yes, it happens.

Comment 22

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

Attachment: Patch v2 -w

Comment 23

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

Comment on attachment 310628 [details] [diff] [review]
disable mutation events in native anon content

Actually, for performance I think you should check this after the window->HasMutationListeners check.

Most of the time IsInNativeAnonymousSubtree won't be true, but !window->HasMutationListeners will be, so we should do that test first.

Comment 24

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

Attachment: Patvh v2

Same as above but with whitespace changes.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 313533 [details] [diff] [review]
Patch v2 -w

>Index: content/base/public/nsIDocument.h
>+  friend class mozAutoDocUpdateRemover;

This needs to also be forward-declared at the top of the file.

>Index: content/base/src/nsGenericElement.cpp
>@@ -2751,25 +2751,29 @@ nsGenericElement::doInsertChildAt(nsICon
>+      if (nsContentUtils::IsSafeToRunScript()) {
...
>+        if (nsContentUtils::IsSafeToRunScript()) {

I don't think you want that second check.

With those fixed, looks good.

Comment 26

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

(In reply to comment #23)
> (From update of attachment 310628 [details] [diff] [review])
> Actually, for performance I think you should check this after the
> window->HasMutationListeners check.
The idea to put the |if| there was to prevent DOMSubtreeModified to be fired
when the target is in native anonymous. So |if| before doc->MayDispatchMutationEvent(aTargetForSubtreeModified)
Adding some more |if|s and checking window->HasMutationsListeners(NS_EVENT_BITS_MUTATION_SUBTREEMODIFIED)
could speed up this, though making the code uglier.

Comment 27

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

Won't that get caught when we call nsContentUtils::HasMutationsListeners(NS_EVENT_BITS_MUTATION_SUBTREEMODIFIED) before firing that event?

Comment 28

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

Attachment: don't dispatch mutation events for native anon content

I think this should do it.

Comment 29

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

Comment on attachment 313584 [details] [diff] [review]
don't dispatch mutation events for native anon content

Ah, excellent!

Comment 30

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

Attachment: Patch that'll land

Fixes last review comments

Comment 31

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

Attachment: Patch that'll land -w

Comment 32

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

Landed mine and smaugs patch, so marking this one fixed.

Thanks for the reviews and patches!

Comment 33

[:Gavin Sharp [email: [e-mail]]]

I backed these patches out at sicking's request. They caused some unit test failures in toolkit/content/tests/widgets/test_scale.xul:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1207354260.1207356060.14518.gz&fulltext=1

Comment 34

[Boris Zbarsky [:bzbarsky]]

We should be checking in tests here.  Certainly for smaug's patch....

Comment 35

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

I'll write some tests for mutation events.
(I have tested that the patch doesn't regress those 'mutation event propagates from native-anon of form elements')

Did anyone test whether it was Jonas' or my patch which caused the unit test failures?

Comment 36

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

Applying only my patch doesn't cause those mochitest failures here.
Now testing with attachment 313731 [details] [diff] [review] ...

Comment 37

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

It is the attachment 313731 [details] [diff] [review] which causes the failures.
(With or without the don't-dispatch-mutation-events-in-native-anon patch)

Comment 38

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

I made a mochitest for mutation events and noticed that if .textContent or
.innerHTML is used with native anonymous content, a DOMSubtreeModified is 
dispatched. Will upload a new patch with the testcase. Change to the
patch is small.

Comment 39

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

Attachment: don't dispatch mutation events for native anon content v2, + test

The only change in code (comparing to the previous patch) is that there isn't an 
assertion in nsDocument::MutationEventDispatched, but it just prevents 
dispatching DOMSubtreeModified events when target is in native anonymous content.
The reason for that change is that because of the batching the possible target 
for DOMSubtreeModified may not be in document when added to 
mSubtreeModifiedTargets.

Comment 40

[Jesse Ruderman]

Attachment: testcase 2

With the patch, this testcase triggers:

###!!! ASSERTION: Mutation event dispatched in native anonymous content!?!: 'aVisitor.mEvent->eventStructType != NS_MUTATION_EVENT || aVisitor.mDOMEvent', file /Users/jruderman/trunk/mozilla/content/base/src/nsGenericElement.cpp, line 2315

Comment 41

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

Attachment: fix IsInNativeAnonymousSubtree()

Jesse, thanks for testing!!!
The testcase revealed a bug in IsInNativeAnonymousSubtree() - that method
doesn't work properly after the element is unbound.
So lazy testing doesn't work here. Have to set a flag when binding elements to
tree.

The change to previous patch is the NODE_IS_IN_ANONYMOUS_SUBTREE flag, which
is set in BindToTree and tested in nsIContent::IsInNativeAnonymousSubtree().

Comment 42

[Boris Zbarsky [:bzbarsky]]

Uh... once an element has been unbound, it should be gone, no?  That is, the only things holding refs to the native anonymous content should be frames, which we should be killing on unbind.

Or are we unbinding during event dispatch, with the event system holding a ref?

Comment 43

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

(In reply to comment #42)
> Uh... once an element has been unbound, it should be gone, no?  That is, the
> only things holding refs to the native anonymous content should be frames,
> which we should be killing on unbind.
Actually editor uses native anonymous content too.

> Or are we unbinding during event dispatch, with the event system holding a ref?
Right :(

Comment 44

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

Comment on attachment 314568 [details] [diff] [review]
fix IsInNativeAnonymousSubtree()

>? content/base/test/test_bug
>? content/events/test/test_bug328885.html
>? content/html/content/src/nsGenericHTMLElement.cpp__
>Index: content/base/public/nsINode.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsINode.h,v
>retrieving revision 3.76
>diff -u -8 -p -r3.76 nsINode.h
>--- content/base/public/nsINode.h	20 Feb 2008 07:40:04 -0000	3.76
>+++ content/base/public/nsINode.h	9 Apr 2008 11:21:16 -0000
>@@ -71,62 +71,64 @@ enum {
>   NODE_HAS_LISTENERMANAGER =     0x00000002U,
> 
>   // Whether this node has had any properties set on it
>   NODE_HAS_PROPERTIES =          0x00000004U,
> 
>   // Whether this node is anonymous
>   // NOTE: Should only be used on nsIContent nodes
>   NODE_IS_ANONYMOUS =            0x00000008U,
>+  
>+  NODE_IS_IN_ANONYMOUS_SUBTREE = 0x00000010U,
> 
>   // Whether this node may have a frame
>   // NOTE: Should only be used on nsIContent nodes
>-  NODE_MAY_HAVE_FRAME =          0x00000010U,
>+  NODE_MAY_HAVE_FRAME =          0x00000020U,
> 
>   // Forces the XBL code to treat this node as if it were
>   // in the document and therefore should get bindings attached.
>-  NODE_FORCE_XBL_BINDINGS =      0x00000020U,
>+  NODE_FORCE_XBL_BINDINGS =      0x00000040U,
> 
>   // Whether a binding manager may have a pointer to this
>-  NODE_MAY_BE_IN_BINDING_MNGR =  0x00000040U,
>+  NODE_MAY_BE_IN_BINDING_MNGR =  0x00000080U,
> 
>-  NODE_IS_EDITABLE =             0x00000080U,
>+  NODE_IS_EDITABLE =             0x00000100U,
> 
>   // Optimizations to quickly check whether element may have ID, class or style
>   // attributes. Not all element implementations may use these!
>-  NODE_MAY_HAVE_ID =             0x00000100U,
>-  NODE_MAY_HAVE_CLASS =          0x00000200U,
>-  NODE_MAY_HAVE_STYLE =          0x00000400U,
>+  NODE_MAY_HAVE_ID =             0x00000200U,
>+  NODE_MAY_HAVE_CLASS =          0x00000400U,
>+  NODE_MAY_HAVE_STYLE =          0x00000800U,
> 
>-  NODE_IS_INSERTION_PARENT =     0x00000800U,
>+  NODE_IS_INSERTION_PARENT =     0x00001000U,
> 
>   // Node has an :empty or :-moz-only-whitespace selector
>-  NODE_HAS_EMPTY_SELECTOR =      0x00001000U,
>+  NODE_HAS_EMPTY_SELECTOR =      0x00002000U,
> 
>   // A child of the node has a selector such that any insertion,
>   // removal, or appending of children requires restyling the parent.
>-  NODE_HAS_SLOW_SELECTOR =       0x00002000U,
>+  NODE_HAS_SLOW_SELECTOR =       0x00004000U,
> 
>   // A child of the node has a :first-child, :-moz-first-node,
>   // :only-child, :last-child or :-moz-last-node selector.
>-  NODE_HAS_EDGE_CHILD_SELECTOR = 0x00004000U,
>+  NODE_HAS_EDGE_CHILD_SELECTOR = 0x00008000U,
> 
>   // A child of the node has a selector such that any insertion or
>   // removal of children requires restyling the parent (but append is
>   // OK).
>   NODE_HAS_SLOW_SELECTOR_NOAPPEND
>-                               = 0x00008000U,
>+                               = 0x00010000U,
> 
>   NODE_ALL_SELECTOR_FLAGS =      NODE_HAS_EMPTY_SELECTOR |
>                                  NODE_HAS_SLOW_SELECTOR |
>                                  NODE_HAS_EDGE_CHILD_SELECTOR |
>                                  NODE_HAS_SLOW_SELECTOR_NOAPPEND,
> 
>   // Four bits for the script-type ID
>-  NODE_SCRIPT_TYPE_OFFSET =               16,
>+  NODE_SCRIPT_TYPE_OFFSET =               17,
> 
>   NODE_SCRIPT_TYPE_SIZE =                  4,
> 
>   // Remaining bits are node type specific.
>   NODE_TYPE_SPECIFIC_BITS_OFFSET =
>     NODE_SCRIPT_TYPE_OFFSET + NODE_SCRIPT_TYPE_SIZE
> };
> 
>Index: content/base/src/nsContentUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v
>retrieving revision 1.300
>diff -u -8 -p -r1.300 nsContentUtils.cpp
>--- content/base/src/nsContentUtils.cpp	9 Apr 2008 03:20:39 -0000	1.300
>+++ content/base/src/nsContentUtils.cpp	9 Apr 2008 11:21:17 -0000
>@@ -3186,25 +3186,32 @@ nsContentUtils::HasMutationListeners(nsI
>                                      PRUint32 aType,
>                                      nsINode* aTargetForSubtreeModified)
> {
>   nsIDocument* doc = aNode->GetOwnerDoc();
>   if (!doc) {
>     return PR_FALSE;
>   }
> 
>-  doc->MayDispatchMutationEvent(aTargetForSubtreeModified);
>-
>   // global object will be null for documents that don't have windows.
>   nsCOMPtr<nsPIDOMWindow> window;
>   window = do_QueryInterface(doc->GetScriptGlobalObject());
>+  // This relies on nsEventListenerManager::AddEventListener, which sets
>+  // all mutation bits when there is a listener for DOMSubtreeModified event.
>   if (window && !window->HasMutationListeners(aType)) {
>     return PR_FALSE;
>   }
> 
>+  if (aNode->IsNodeOfType(nsINode::eCONTENT) &&
>+      static_cast<nsIContent*>(aNode)->IsInNativeAnonymousSubtree()) {
>+    return PR_FALSE;
>+  }
>+
>+  doc->MayDispatchMutationEvent(aTargetForSubtreeModified);
>+
>   // If we have a window, we can check it for mutation listeners now.
>   nsCOMPtr<nsPIDOMEventTarget> piTarget(do_QueryInterface(window));
>   if (piTarget) {
>     nsCOMPtr<nsIEventListenerManager> manager;
>     piTarget->GetListenerManager(PR_FALSE, getter_AddRefs(manager));
>     if (manager) {
>       PRBool hasListeners = PR_FALSE;
>       manager->HasMutationListeners(&hasListeners);
>Index: content/base/src/nsDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsDocument.cpp,v
>retrieving revision 3.815
>diff -u -8 -p -r3.815 nsDocument.cpp
>--- content/base/src/nsDocument.cpp	9 Apr 2008 03:20:39 -0000	3.815
>+++ content/base/src/nsDocument.cpp	9 Apr 2008 11:21:18 -0000
>@@ -5833,19 +5833,16 @@ nsDocument::MutationEventDispatched(nsIN
>       return;
>     }
> 
>     nsCOMArray<nsINode> realTargets;
>     for (PRInt32 i = 0; i < count; ++i) {
>       nsINode* possibleTarget = mSubtreeModifiedTargets[i];
>       nsCOMPtr<nsIContent> content = do_QueryInterface(possibleTarget);
>       if (content && content->IsInNativeAnonymousSubtree()) {
>-        if (realTargets.IndexOf(possibleTarget) == -1) {
>-          realTargets.AppendObject(possibleTarget);
>-        }
>         continue;
>       }
> 
>       nsINode* commonAncestor = nsnull;
>       PRInt32 realTargetCount = realTargets.Count();
>       for (PRInt32 j = 0; j < realTargetCount; ++j) {
>         commonAncestor =
>           nsContentUtils::GetCommonAncestor(possibleTarget, realTargets[j]);
>Index: content/base/src/nsGenericDOMDataNode.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericDOMDataNode.cpp,v
>retrieving revision 3.256
>diff -u -8 -p -r3.256 nsGenericDOMDataNode.cpp
>--- content/base/src/nsGenericDOMDataNode.cpp	9 Apr 2008 03:20:39 -0000	3.256
>+++ content/base/src/nsGenericDOMDataNode.cpp	9 Apr 2008 11:21:19 -0000
>@@ -600,16 +600,20 @@ nsGenericDOMDataNode::BindToTree(nsIDocu
>   }
> 
>   // First set the binding parent
>   if (aBindingParent) {
>     nsDataSlots *slots = GetDataSlots();
>     NS_ENSURE_TRUE(slots, NS_ERROR_OUT_OF_MEMORY);
> 
>     slots->mBindingParent = aBindingParent; // Weak, so no addref happens.
>+    if (IsNativeAnonymous() ||
>+        aBindingParent->IsInNativeAnonymousSubtree()) {
>+      SetFlags(NODE_IS_IN_ANONYMOUS_SUBTREE);
>+    }

What if the parent is native-anon? Do you need to add
 || (aParent && aParent->IsInNativeAnonymousSubtree())

Also, could you assert if the flag is already set and we're inserting into a place which isn't native-anon. I.e. if we ever end up in a state where a native-anon is inserted into a "normal" subtree.

Same thing in nsGenericElement.


> nsIContent::IsInNativeAnonymousSubtree() const
> {
>+  if (HasFlag(NODE_IS_IN_ANONYMOUS_SUBTREE)) {
>+    return PR_TRUE;
>+  }
>+#ifdef DEBUG
>   nsIContent* content = GetBindingParent();
>   while (content) {
>     if (content->IsNativeAnonymous()) {
>-      return PR_TRUE;
>+      NS_ERROR("Element not marked to be in native anonymous subtree!");
>+      break;
>     }
>     content = content->GetBindingParent();
>   }
>+#endif
>   return PR_FALSE;
> }

Could you inline this? The non-debug code should simply be:
return HasFlag(NODE_IS_IN_ANONYMOUS_SUBTREE);

>@@ -2060,18 +2066,23 @@ nsGenericElement::BindToTree(nsIDocument
>     if (aBindingParent) {
>       nsDOMSlots *slots = GetDOMSlots();
> 
>       if (!slots) {
>         return NS_ERROR_OUT_OF_MEMORY;
>       }
> 
>       slots->mBindingParent = aBindingParent; // Weak, so no addref happens.
>+      
>     }
>   }

No need for this empty line.

r/sr=me with that fixed.

Thanks for the tests!

Comment 45

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

Attachment: Patch v3 -w

Comment 46

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

Comment on attachment 314751 [details] [diff] [review]
Patch v3 -w

So this changes two things since last patch. First of all it moves the nsSliderFrame scrollbar->SetAttr call out to when it's safe to run scripts, which means that the mutation event will fire appropriately.

Second, it makes mozAutoDocUpdate add a scriptblocker even when we weren't passed a document (or when aNotify is false). This isn't strictly needed by this patch, but we'll want to do it sooner or later anyway, and this patch does it right so that mutation events aren't prevented in out-of-doc subtrees.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Jonas, could you maybe post an interdiff against the last patch I reviewed?  Bugzilla's diff is failing to deal, as usual.

It might be a good idea to get roc to look at the slider thing.

Do we need to worry about the fact that mutation events won't fire for nodes inheriting attributes using xbl:inherits (because that does a SetAttr under AttributeChanged)?

Comment 48

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

They should still fire since the only blockers are ones added through BeginUpdate(UPDATE_CONTENT_MODEL), right?

Comment 49

[Boris Zbarsky [:bzbarsky]]

Oh, I guess the issue with the frame is that the frame construction blockers are in place there?  OK.  That makes sense.

I'd still love an interdiff; it would make reviewing much simpler.

Comment 50

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

Exactly. Note that this part hasn't changed since last patch. The only thing that's changed is that I moved off one callsite to a nsIRunnable.

Still working on getting interdiff installed :( Though I'm not sure that it'll turn up that useful. The names of all the functions have changed so I suspect most of the patch is going to show up in the change set.

Comment 51

[Boris Zbarsky [:bzbarsky]]

An interdiff against the "patch that will land" (which had the renamings already, right?) would be just fine by me.

Comment 52

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

No it didn't, it only fixed the things you had commented on.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment 46 didn't sound like there was that much changed.  :(

If you think I just need to review from scratch, I can do that, I guess.  I won't be able to get to that until Tuesday, though.

Comment 54

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

There really isn't that much that has changed functionality wise, but the names of most things have changed since it's now nsContentUtils that keep track of the "removable count" rather than the document (since a document isn't always available).

I guess I can revert that part of the patch... I don't actually *know* of any cases where inserting stuff into orphaned trees require blockers.

Comment 55

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

> What if the parent is native-anon? Do you need to add
>  || (aParent && aParent->IsInNativeAnonymousSubtree())
If parent is native anon, then aBindingParent points to parent (because
parent's binding parent is it itself).

> Also, could you assert if the flag is already set and we're inserting into a
> place which isn't native-anon. I.e. if we ever end up in a state where a
> native-anon is inserted into a "normal" subtree.
Sure.

> 
> Could you inline this? The non-debug code should simply be:
> return HasFlag(NODE_IS_IN_ANONYMOUS_SUBTREE);
ok.

Comment 56

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

Attachment: inline IsInNativeAnonymousSubtree()

Comment 57

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

Attachment: this might even compile without DEBUG

Comment 58

[Mike Beltzner [:beltzner, not reading bugmail]]

Which patch are drivers meant to consider? The one with reviews or the one without? The comments aren't making it clear to us ...

Comment 59

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

The one which has approval1.9?, and the one which still needs review.

Comment 60

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

Attachment: Patch v3.5 -w

Ok, this is the latest and greatest. This is the same as the previous patch, but I also moved out the SetAttr calls from nsSplitterFrame.cpp into a runnable in order to avoid turning off the mutation events it should cause.

Interdiff coming up

Comment 61

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

Attachment: Patch v3.5

Same as above but with whitespace changes

Comment 62

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

Attachment: Interdiff to "Patch that'll land"

So here are the changes since the last reviewed patch:

Instead of letting the documents keep track of which blockers are removable and which aren't, let nsContentUtils do that. This allows us to add blockers in mozAutoDocUpdate even when we don't have a document.

Move SetAttr calls in nsSplitterFrame.cpp and nsSliderFrame.cpp to a runnable so that the SetAttr call correctly results in mutation events firing.

Since mozAutoDocUpdate now calls nsContentUtils directly I moved it out to a separate file to avoid include hell. Unfortunately that meant that a bunch of places had to #include this new file. I've removed these #includes from this interdiff to make it easier to read.

Comment 63

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

An alternative fix would be to just land the "Patch that'll land" patch + the
changes to move the SetAttr calls to where they are safe to make. That should
also pass all mochitests.

However it would mean that we wouldn't have any blockers on the stack when
making UnbindFromTree/BindToTree calls into/outof orphaned trees.

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 315004 [details] [diff] [review]
Patch v3.5

Looks good, except I'd prefer explicit if/then instead of ?: when the point is to actually do something, not to compute a value.

I still think roc should look at the XUL changes...

Comment 65

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314840 [details] [diff] [review]
this might even compile without DEBUG

Jonas, you're owner of this bug. Can you please nominate for approval the actual patches that are needed to fix this bug, once they've been reviewed? I continue to see "approve this one ... no wait, I came up with a better idea, approve this one!" which doesn't lend a lot of confidence.

Comment 66

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

Comment on attachment 315004 [details] [diff] [review]
Patch v3.5

I assume those scriptrunner things would run before the next reflow? If so, this is should be fine.

Comment 67

[Boris Zbarsky [:bzbarsky]]

I don't think anything guarantees that.  In fact, that would only be the case if the next reflow hasn't had its event posted yet and there is no reflow flush before the attribute setting events fire...

Comment 68

[Boris Zbarsky [:bzbarsky]]

roc, see comment 67.

Comment 69

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

Hmm, I thought that nsContentUtils::RemoveScriptBlocker is responsible for running those callbacks, and surely all script blockers are removed before we return to the event loop. Surely? Or is the problem that someone might flush reflows while there are script blockers on the stack?

Comment 70

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

That aside, the nsSplitterFrame changes look safe in any case, although it would make more sense for the splitter to just have an internal collapsed state that it can set and pick up during layout, avoiding the need for it to set its own "collapsed" attribute. File a bug for that?

The nsSliderFrame change seems like it could let the curpos be temporarily greater than maxpos (or less than minpos). That seems bad, but the best idea I have there is to stop clamping the curpos content attribute and just clamp it internally whenever we use it --- and that doesn't seem like a great idea at this point.

I have to say, this patch looks a bit scary and if we're just doing it to fix fuzz tests, I'd be inclined to not take it at this point.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Oh, those are added as script runners.  Then yeah, they should be run when the script blocker goes away.  That should be before reflow, since we don't reflow while script blockers are live.

Comment 72

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

(In reply to comment #69)
> Hmm, I thought that nsContentUtils::RemoveScriptBlocker is responsible for
> running those callbacks, and surely all script blockers are removed before we
> return to the event loop. Surely?

Yes, the idea is that all of this is true. I.e. RemoveScriptBlocker 

> Or is the problem that someone might flush
> reflows while there are script blockers on the stack?

I assume this could in theory happen. Though only if a single javascript call can result in two reflows happening. All script blockers should get removed by the time we get back to content javascript.

Comment 73

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

Comment on attachment 314840 [details] [diff] [review]
this might even compile without DEBUG

Beltzner: There are two patches developed in this bug, we're basically adding belts and suspenders here.

This patch removes all traces of mutation events when we modify anonymous content. Mutation events fire at scary times, especially when it comes to native-anon content since we modify that during reflow. So getting rid of them should remove a potential attack vector. The risk is relatively low since there aren't currently any listeners for these events.

Comment 74

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

Comment on attachment 315004 [details] [diff] [review]
Patch v3.5

This is the second patch that needs to land.

It fixes some very bad broken-ness in new security architecture that landed in bug 401155. It would be really good to get this architecture working so that we can use it in future security fixes on the branch.

The biggest risk here is that we in some edge cases won't fire mutation events when we should. However mutation events are very rarely used on the web, and so it's unlikely that any such edge cases would affect anyone.

Comment 75

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

(In reply to comment #70)
> That aside, the nsSplitterFrame changes look safe in any case, although it
> would make more sense for the splitter to just have an internal collapsed state
> that it can set and pick up during layout, avoiding the need for it to set its
> own "collapsed" attribute. File a bug for that?

Filed bug 428480.

> The nsSliderFrame change seems like it could let the curpos be temporarily
> greater than maxpos (or less than minpos). That seems bad, but the best idea I
> have there is to stop clamping the curpos content attribute and just clamp it
> internally whenever we use it --- and that doesn't seem like a great idea at
> this point.

So we should never be in this state by the time we get back to script. The idea of the AddScriptRunner API is that it guarantees that the runner executes before we return to javascript. Not sure if that makes you feel better about it?

> I have to say, this patch looks a bit scary and if we're just doing it to fix
> fuzz tests, I'd be inclined to not take it at this point.

Refactoring how the removable blockers work we definitely need to do. As things stand now we can easily end up with negative number of script blockers which I'm sure could cause all sorts of bad things to happen. This will happen if you simply listen to the right two mutation events.

Blocking mutation events while we have script blockers isn't known to be needed. Though likely any time that that happens it would be possible for a mutation-handler to cause exploitable crashes. 

Comment 76

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314840 [details] [diff] [review]
this might even compile without DEBUG

a=beltzner, thanks for the clarification

Comment 77

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

Checked in!

Comment 78

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

Checked in! Smaugs patch (attachment 314840 [details] [diff] [review]) landed friday 4/11, mine (attachment 315004 [details] [diff] [review]) landed a few mins ago.

Comment 79

[Carsten Book [:Tomcat]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre and the testcases - no crash/assertion

--> Verified fixed