643786 - Crash [@ nsHTMLEditRules::~nsHTMLEditRules]

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: crash stack trace

Comment 2

[(no longer active)]

nsHTMLEditRules::DocumentModifiedWorker is causing a flush, which destroys the editor in the middle of the operation, under this stack:

#0	0x100b1ff5e in nsEditor::RemoveEditActionListener at nsEditor.cpp:1775
#1	0x1014e2bbf in mozInlineSpellChecker::UnregisterEventListeners at mozInlineSpellChecker.cpp:680
#2	0x1014e323c in mozInlineSpellChecker::Cleanup at mozInlineSpellChecker.cpp:595
#3	0x100b228c6 in nsEditor::PreDestroy at nsEditor.cpp:449
#4	0x100e7d4ce in nsHTMLEditor::PreDestroy at nsHTMLEditor.cpp:400
#5	0x101062b68 in nsDocShellEditorData::SetEditor at nsDocShellEditorData.cpp:191
#6	0x10103cf4d in nsDocShell::SetEditor at nsDocShell.cpp:10534
#7	0x10127b68c in nsEditingSession::TearDownEditorOnWindow at nsEditingSession.cpp:604
#8	0x10088a1d1 in nsHTMLDocument::TurnEditingOff at nsHTMLDocument.cpp:3186
#9	0x1008944c1 in nsHTMLDocument::EditingStateChanged at nsHTMLDocument.cpp:3233
#10	0x10089597a in nsHTMLDocument::SetDesignMode at nsHTMLDocument.cpp:3437
#11	0x10069e677 in nsFrameLoader::Show at nsFrameLoader.cpp:855
#12	0x10047d7d2 in nsSubDocumentFrame::ShowViewer at nsSubDocumentFrame.cpp:235
#13	0x100480487 in AsyncFrameInit::Run at nsSubDocumentFrame.cpp:152
#14	0x10062b6cc in nsContentUtils::RemoveScriptBlocker at nsContentUtils.cpp:4729
#15	0x10030ab8d in nsAutoScriptBlocker::~nsAutoScriptBlocker at nsContentUtils.h:1895
#16	0x10038bc31 in PresShell::FlushPendingNotifications at nsPresShell.cpp:4882
#17	0x10065fa04 in nsDocument::FlushPendingNotifications at nsDocument.cpp:6477
#18	0x10065f9bf in nsDocument::FlushPendingNotifications at nsDocument.cpp:6472
#19	0x10050f31e in nsComputedDOMStyle::GetPropertyCSSValue at nsComputedDOMStyle.cpp:486
#20	0x10050ed0f in nsComputedDOMStyle::GetPropertyValue at nsComputedDOMStyle.cpp:310
#21	0x100e5d83a in nsHTMLCSSUtils::GetCSSInlinePropertyBase at nsHTMLCSSUtils.cpp:571
#22	0x100e5ec90 in nsHTMLCSSUtils::GetComputedProperty at nsHTMLCSSUtils.cpp:545
#23	0x100e6b4a5 in nsHTMLEditor::FindUserSelectAllNode at nsHTMLEditor.cpp:3974
#24	0x100e6b92b in nsHTMLEditor::DeleteNode at nsHTMLEditor.cpp:3855
#25	0x100e8e80a in nsHTMLEditRules::DocumentModifiedWorker at nsHTMLEditRules.cpp:9216
#26	0x100ec6891 in nsRunnableMethodImpl<void (nsHTMLEditRules::*)(), true>::Run at nsThreadUtils.h:345
#27	0x10062b6cc in nsContentUtils::RemoveScriptBlocker at nsContentUtils.cpp:4729
#28	0x1006bf5ea in nsContentUtils::RemoveRemovableScriptBlocker at nsContentUtils.h:1505
#29	0x10067e7a5 in nsDocument::EndUpdate at nsDocument.cpp:4002
#30	0x1008975e9 in nsHTMLDocument::EndUpdate at nsHTMLDocument.cpp:2950
#31	0x10041685e in mozAutoDocUpdate::~mozAutoDocUpdate at mozAutoDocUpdate.h:66
#32	0x1006b3530 in nsINode::doRemoveChildAt at nsGenericElement.cpp:3713
#33	0x10066d70e in nsDocument::RemoveChildAt at nsDocument.cpp:3506
#34	0x1006bce2e in nsINode::RemoveChild at nsINode.h:485
#35	0x100ffcd23 in nsIDOMNode_RemoveChild at dom_quickstubs.cpp:6192
#36	0x101baafe7 in js::CallJSNative at jscntxtinlines.h:701

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Comment 4

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

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

> Fix a crash caused by the HTML editor to be destroyed

s/to be/being/

Comment 5

[(no longer active)]

Attachment: Patch (v2)

The script blocker that I added will be invoked during UPDATE_CONTENT_MODEL, and we're calling into mutation observers as a result of deleting and/or adding the bogus node, so the script blocker should be removable in order to allow the mutation events to be sent.  I found out about this problem by assertions into crashtests and reftests.

This patch switches to using a removable script blocker.

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c9387916c9c2

Comment 7

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

This fix seems wrong. If you can't deal with flushes during the call to nsHTMLEditor::DeleteNode, then adding a removable-blocker won't help since it doesn't prevent the mutation event from firing (as you note), and when the mutation event fires it can cause things to flush.

Comment 8

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

Reopening this since I don't think the fix was good.

Comment 9

[(no longer active)]

Do you have any better ideas on how to fix this?

Here's a crazy idea: can we make some nodes (like the editor bogus nodes) exceptional with regard to mutation events (and not fire them for those nodes)?

Although I'm still not sure if that's the right thing to do here.

Comment 10

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

can we remove the node off of an event? or a scriptrunner?

Comment 11

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

Attachment: Suppress DOMNodeRemoved

So I put my finger on why I don't like the patch that's here as checked in.

The problem is that it relies on scripblockers to prevent flushing from occurring. While that currently works, I mostly made flushing not happen while there are scriptblockers since flushing currently can run XBL stuff.

However when we fix that, I had planned on making flushing allowed while there are scriptblockers. The idea of scriptblockers is to delay evil scripts from running, not prevent code that we have control over from running.

But doing so would make the fix here not work.

I think that's fine for now, we will just have to revisit if/when we allow flushing while there are scriptblockers.

For now, this patch simply adds a special scriptblocker which prevents DOMNodeRemoved from firing which should fix this bug.

Comment 12

[(no longer active)]

Comment on attachment 528995 [details] [diff] [review]
Suppress DOMNodeRemoved

Can you please add a small comment to the huge assertion in MaybeFireNodeRemoved to document its intent?  I can't be the only one who has had to spend some time hunting around to figure out what it's asserting.

r=me with that.

Comment 13

[(no longer active)]

Comment on attachment 528995 [details] [diff] [review]
Suppress DOMNodeRemoved

>+   // DeleteNode below may cause a flush, which could destroy the editor
>+  nsAutoScriptBlockerSuppressNodeRemoved scriptBlocker;

Oh, and the nit that I meant to make here is obvious, isn't it?  :-)

Comment 14

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

Done and done :)

Comment 15

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

http://hg.mozilla.org/mozilla-central/rev/d70ce672b2c3

Checked in. We should be in a good state for now. However, I don't like relying on scriptblockers to prevent reflows from happening. Scriptblockers are intended to block scripts from running, not internal code from running.

The only reason I made it block reflows for now was that reflows flush the content sink which might be able to run script. However that flushing won't be needed soon in which case we could allow reflows to happen, at which point this bug would regress again :(

This isn't something we need to tend to right away, but it's something I'd like to find a solution for.