612447 - reproducible crash [@ nsEditor::DeleteNode(nsIDOMNode*) ] when using jsbin.com edit feature

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Asa Dotzler [:asa]]

I can consistently crash Firefox's latest trunk nightly on win7 by visiting http://jsbin.com/odoho3 and clicking the "edit with js bin" link in the top right corner of the window. 

Or, just click here http://jsbin.com/odoho3/edit

here's a crash report http://crash-stats.mozilla.com/report/index/bp-22051991-0c88-4f8d-aa1d-775812101115

Comment 1

[Asa Dotzler [:asa]]

actually, it's not enough to click the link directly. You must first visit http://jsbin.com/odoho3 and then click the link.

Comment 2

[Asa Dotzler [:asa]]

http://crash-stats.mozilla.com/report/index/bp-8d5be0c6-7b2b-4443-89c9-dab982101115 is the same crash, I think, with a better report.  So, -> editor?

nsEditor::DeleteNode

Comment 3

[Asa Dotzler [:asa]]

ehsan, is this in anything you're working on?

Comment 4

[Asa Dotzler [:asa]]

comment #1 was wrong. I can reproduce simply by clicking this link http://jsbin.com/odoho3/edit

Comment 5

[Marcia Knous [:marcia]]

Occurs using Mac as well, just by clicking the link in Comment 4.

Comment 7

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

Regression from bug 611182?

Comment 8

[Asa Dotzler [:asa]]

why is this marked security-confidential? not contesting, just interested.

Comment 9

[Scoobidiver (away)]

From the dupe, here is the regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=674f2ed15cea&tochange=edf41ff32f08

Comment 10

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

e091e8999c1d doesn't crash, 98b0ae888195 does.

The first one is right before https://bugzilla.mozilla.org/show_bug.cgi?id=611182#c29

Comment 11

[(no longer active)]

So, this doesn't happen with the fix to bug 612565 on jsbin.com, but the issue is very serious, and it could potentially happen in a lot of other cases.

What's happening is that we call nsIEditor::DeleteNode, which indirectly calls nsHTMLCSSUtils::GetComputedProperty, which flushes.  When the flush is finished and we call nsContentUtils::RemoveScriptBlocker, nsFrameLoader::Show gets called, which calls SetDesignMode("off") and SetDesignMode("on") to restore the editor on the frame.  This causes the original editor attached to the document to be destroyed with a stack like this:

#0  0x00000001143fed82 in nsPlaintextEditor::~nsPlaintextEditor (this=0x127d88500) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/text/nsPlaintextEditor.cpp:110
#1  0x000000011476a011 in nsHTMLEditor::~nsHTMLEditor (this=0x127d88500) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:228
#2  0x0000000114418bb0 in nsEditor::Release (this=0x127d88500) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/base/nsEditor.cpp:215
#3  0x0000000114767d99 in nsHTMLEditor::Release (this=0x127d88500) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:238
#4  0x0000000113b72578 in nsCOMPtr<nsIEditor>::~nsCOMPtr (this=0x7fff5fbfa1c0) at nsCOMPtr.h:533
#5  0x0000000113b7b77f in nsEditingSession::TearDownEditorOnWindow (this=0x127d881f0, aWindow=0x127e0f0e0) at /Users/ehsanakhgari/moz/mozilla-central/editor/composer/src/nsEditingSession.cpp:623
#6  0x0000000114189a6d in nsHTMLDocument::TurnEditingOff (this=0x12993a800) at /Users/ehsanakhgari/moz/mozilla-central/content/html/document/src/nsHTMLDocument.cpp:3197
#7  0x0000000114193d23 in nsHTMLDocument::EditingStateChanged (this=0x12993a800) at /Users/ehsanakhgari/moz/mozilla-central/content/html/document/src/nsHTMLDocument.cpp:3244
#8  0x0000000114195110 in nsHTMLDocument::SetDesignMode (this=0x12993a800, aDesignMode=@0x7fff5fbfa6b0) at /Users/ehsanakhgari/moz/mozilla-central/content/html/document/src/nsHTMLDocument.cpp:3439
#9  0x0000000113fa83cd in nsFrameLoader::Show (this=0x127e0e790, marginWidth=-1, marginHeight=-1, scrollbarPrefX=1, scrollbarPrefY=1, frame=0x101638ca8) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsFrameLoader.cpp:689
#10 0x0000000113d8af6a in nsSubDocumentFrame::ShowViewer (this=0x101638ca8) at /Users/ehsanakhgari/moz/mozilla-central/layout/generic/nsSubDocumentFrame.cpp:235
#11 0x0000000113d8dbb7 in AsyncFrameInit::Run (this=0x11d87be50) at /Users/ehsanakhgari/moz/mozilla-central/layout/generic/nsSubDocumentFrame.cpp:152
#12 0x0000000113f34154 in nsContentUtils::RemoveScriptBlocker () at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsContentUtils.cpp:4758
#13 0x0000000113c1e981 in nsAutoScriptBlocker::~nsAutoScriptBlocker (this=0x7fff5fbfa8ff) at nsContentUtils.h:1892
#14 0x0000000113c9ca5a in PresShell::FlushPendingNotifications (this=0x127c77350, aType=Flush_Layout) at /Users/ehsanakhgari/moz/mozilla-central/layout/base/nsPresShell.cpp:4851
#15 0x0000000113f6bb86 in nsDocument::FlushPendingNotifications (this=0x10142b400, aType=Flush_Layout) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsDocument.cpp:6437
#16 0x0000000113f6bb41 in nsDocument::FlushPendingNotifications (this=0x12993a800, aType=Flush_Style) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsDocument.cpp:6432
#17 0x0000000113e1808e in nsComputedDOMStyle::GetPropertyCSSValue (this=0x127e574a0, aPropertyName=@0x7fff5fbfac20, aReturn=0x7fff5fbfab00) at /Users/ehsanakhgari/moz/mozilla-central/layout/style/nsComputedDOMStyle.cpp:486
#18 0x0000000113e17a7f in nsComputedDOMStyle::GetPropertyValue (this=0x127e574a0, aPropertyName=@0x7fff5fbfac20, aReturn=@0x7fff5fbfab80) at /Users/ehsanakhgari/moz/mozilla-central/layout/style/nsComputedDOMStyle.cpp:310
#19 0x0000000114748060 in nsHTMLCSSUtils::GetCSSInlinePropertyBase (this=0x127eb5b20, aNode=0x127dc1198, aProperty=0x101843d70, aValue=@0x7fff5fbfadf0, aViewCSS=0x127e0f188, aStyleType=2 '\002') at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/nsHTMLCSSUtils.cpp:571
#20 0x00000001147494b6 in nsHTMLCSSUtils::GetComputedProperty (this=0x127eb5b20, aNode=0x127dc1198, aProperty=0x101843d70, aValue=@0x7fff5fbfadf0) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/nsHTMLCSSUtils.cpp:545
#21 0x0000000114755cdc in nsHTMLEditor::FindUserSelectAllNode (this=0x127d88500, aNode=0x127dc1198) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:3918
#22 0x0000000114755f69 in nsHTMLEditor::DeleteNode (this=0x127d88500, aNode=0x127dc1198) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:3801
#23 0x0000000114777b17 in nsHTMLEditRules::DocumentModifiedWorker (this=0x129f07400) at /Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/nsHTMLEditRules.cpp:9181
#24 0x00000001147af65d in nsRunnableMethodImpl<void (nsHTMLEditRules::*)(), true>::Run (this=0x12ea311c0) at nsThreadUtils.h:347
#25 0x0000000113f34154 in nsContentUtils::RemoveScriptBlocker () at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsContentUtils.cpp:4758
#26 0x0000000113fc7e74 in nsContentUtils::RemoveRemovableScriptBlocker () at nsContentUtils.h:1483
#27 0x0000000113f89759 in nsDocument::EndUpdate (this=0x12993a800, aUpdateType=1) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsDocument.cpp:3968
#28 0x0000000114196d7f in nsHTMLDocument::EndUpdate (this=0x12993a800, aUpdateType=1) at /Users/ehsanakhgari/moz/mozilla-central/content/html/document/src/nsHTMLDocument.cpp:2961
#29 0x0000000113e2f9fe in mozAutoDocConditionalContentUpdateBatch::~mozAutoDocConditionalContentUpdateBatch (this=0x7fff5fbfb1a0) at mozAutoDocUpdate.h:112
#30 0x0000000113fbbf48 in nsINode::ReplaceOrInsertBefore (this=0x1277e3570, aReplace=0, aNewChild=0x127dc3e60, aRefChild=0x0) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsGenericElement.cpp:4283
#31 0x00000001140e07b2 in nsINode::ReplaceOrInsertBefore (this=0x1277e3570, aReplace=0, aNewChild=0x127dc3e60, aRefChild=0x0, aReturn=0x7fff5fbfb38c) at nsINode.h:1222
#32 0x00000001140e0831 in nsINode::InsertBefore (this=0x1277e3570, aNewChild=0x127dc3e60, aRefChild=0x0, aReturn=0x7fff5fbfb38c) at nsINode.h:457
#33 0x00000001148f04a8 in nsIDOMNode_InsertBefore (cx=0x1264eae30, argc=2, vp=0x1178e3658) at dom_quickstubs.cpp:6063
#34 0x00000001001b591c in js::CallJSNative (cx=0x1264eae30, native=0x1148f0218 <nsIDOMNode_InsertBefore(JSContext*, unsigned int, jsval_layout*)>, argc=2, vp=0x1178e3658) at jscntxtinlines.h:684

When we return to nsHTMLEditor::DeleteNode, |*this| is dead, and later on in nsAutoRules constructor we try to dereference |this|, which causes us to crash.

I'm not sure if this crash is exploitable...

Comment 12

[(no longer active)]

Attachment: Patch (v1)

This patch makes nsFrameLoader::Show not recreate the editor object, but use the same object.  It also makes a few changes to nsHTMLEditor to make it safe for reuse.

Comment 13

[Boris Zbarsky [:bzbarsky]]

That doesn't seem to make this safe, though.  If you're flushing, arbitrary script can run.  So whatever is up the stack from a flush needs to be able to deal with that.

Comment 14

[(no longer active)]

(In reply to comment #13)
> That doesn't seem to make this safe, though.  If you're flushing, arbitrary
> script can run.  So whatever is up the stack from a flush needs to be able to
> deal with that.

So, seems like all of the HTML editor commands which can be executed via the nsIDOMNSHTMLDocuemnt methods already hold a strong reference to the HTML editor's object (which is the weak command context pointer.  See <http://mxr.mozilla.org/mozilla-central/source/embedding/components/commandhandler/src/nsBaseCommandController.cpp#124> to the end of this file (note the variables named |weak|).

Comment 15

[(no longer active)]

Attachment: Part 2

This patch protects the editor object from dying while executing a mutation event handler.  With this, I think we should be good to go here.

Comment 16

[Boris Zbarsky [:bzbarsky]]

How does part 2 help?  The code in the stack above doesn't run while that nsCOMPtr is on the stack, since it runs off a script runner.  Shouldn't we hold the ref in the script runner runnable?

Comment 17

[(no longer active)]

(In reply to comment #16)
> How does part 2 help?  The code in the stack above doesn't run while that
> nsCOMPtr is on the stack, since it runs off a script runner.  Shouldn't we hold
> the ref in the script runner runnable?

This doesn't directly relate to this bug, but it will potentially help in similar situations which we don't know about yet.  Note that DocumentModified is not the only thing executed by these two functions.  But you're right, I need to submit another patch for the part which is run off the script runner as well.

Comment 18

[(no longer active)]

Attachment: Part 3

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 491636 [details] [diff] [review]
Part 2

r=me, but watch out for this showing up in profiles...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 491683 [details] [diff] [review]
Part 3

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 491345 [details] [diff] [review]
Patch (v1)

r=me

Comment 22

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/64fdcad8cb11
http://hg.mozilla.org/mozilla-central/rev/406748270073
http://hg.mozilla.org/mozilla-central/rev/70116ee1ea9a