Last Comment Bug 432114 - Crash [@ PL_DHashTableOperate][@ nsEditor::EndUpdateViewBatch] with DOMNodeInserted event listener removing window and frameset contenteditable
: Crash [@ PL_DHashTableOperate][@ nsEditor::EndUpdateViewBatch] with DOMNodeIn...
Status: VERIFIED FIXED
[sg:critical?] post 1.8-branch
: crash, regression, testcase, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows XP
: P3 critical (vote)
: ---
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
: 476975 (view as bug list)
Depends on: 510662
Blocks: 386782 478869
  Show dependency treegraph
 
Reported: 2008-05-04 07:06 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
12 users (show)
roc: blocking1.9.1-
roc: wanted1.9.1+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (494 bytes, text/html)
2008-05-04 07:06 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
stack from debug build (5.95 KB, text/plain)
2008-05-04 07:07 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase2 (794 bytes, text/html)
2008-05-06 04:39 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
v1 (2.63 KB, patch)
2008-05-09 16:18 PDT, Chris Pearce (:cpearce)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
With style sheet owner fix (6.49 KB, patch)
2008-10-31 13:25 PDT, Chris Pearce (:cpearce)
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review
Patch v3 (5.24 KB, patch)
2009-02-16 18:23 PST, Chris Pearce (:cpearce)
cpearce: review+
cpearce: superreview+
dbaron: approval1.9.1+
Details | Diff | Splinter Review
Patch ported to 1.9.0 branch (7.52 KB, patch)
2009-03-16 18:05 PDT, Chris Pearce (:cpearce)
cpearce: review+
cpearce: superreview+
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-04 07:06:54 PDT
Created attachment 319272 [details]
testcase

See testcase, which crashes current trunk build.

This regressed between 2008-04-23 and 2008-04-24:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-04-23+04&maxdate=2008-04-24+09&cvsroot=%2Fcvsroot
I think a regression of bug 386782.

The data url of the iframe consists of this:
<script>
window.addEventListener('DOMNodeInserted', function() {window.frameElement.parentNode.removeChild(window.frameElement);}, true);
</script>
<frameset contenteditable="true">

Useful stack of breakpad (I also get a lot of useless stacks):
http://crash-stats.mozilla.com/report/index/a30693b8-19df-11dd-a4f1-001a4bd46e84?p=1
0  	xul.dll  	PL_DHashTableOperate  	 pldhash.c:588
1 	xul.dll 	nsTHashtable<nsBaseHashtableET<nsStringHashKey, nsCOMPtr<nsIDOMStorage> > >::GetEntry 	nsTHashtable.h:164
2 	xul.dll 	nsClassHashtable<nsUint32HashKey, nsCOMArray<nsXULTemplateResultRDF> >::Get 	nsClassHashtable.h:101
3 	xul.dll 	nsCommandManager::CommandStatusChanged 	mozilla/embedding/components/commandhandler/src/nsCommandManager.cpp:127
4 	xul.dll 	nsComposerCommandsUpdater::UpdateCommandGroup 	mozilla/editor/composer/src/nsComposerCommandsUpdater.cpp:347
5 	xul.dll 	nsComposerCommandsUpdater::UpdateDirtyState 	mozilla/editor/composer/src/nsComposerCommandsUpdater.cpp:291
6 	xul.dll 	nsComposerCommandsUpdater::NotifyDocumentStateChanged 	mozilla/editor/composer/src/nsComposerCommandsUpdater.cpp:107
7 	xul.dll 	xul.dll@0x24f93c 	
8 	xul.dll 	nsEditor::IncrementModificationCount 	mozilla/editor/libeditor/base/nsEditor.cpp:3877
9 	xul.dll 	nsEditor::DoAfterDoTransaction 	mozilla/editor/libeditor/base/nsEditor.cpp:4638
10 	xul.dll 	nsEditor::DoTransaction 	mozilla/editor/libeditor/base/nsEditor.cpp:690
11 	xul.dll 	nsEditor::InsertNode 	mozilla/editor/libeditor/base/nsEditor.cpp:1393
12 	xul.dll 	nsTextEditRules::CreateBogusNodeIfNeeded 	mozilla/editor/libeditor/text/nsTextEditRules.cpp:1332
13 	xul.dll 	nsTextEditRules::Init 	mozilla/editor/libeditor/text/nsTextEditRules.cpp:141
14 	xul.dll 	nsHTMLEditRules::Init 	mozilla/editor/libeditor/html/nsHTMLEditRules.cpp:252
15 	xul.dll 	nsHTMLEditor::InitRules 	mozilla/editor/libeditor/html/nsHTMLEditor.cpp:437
16 	xul.dll 	nsPlaintextEditor::EndEditorInit 	mozilla/editor/libeditor/text/nsPlaintextEditor.cpp:180
17 	xul.dll 	nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger 	mozilla/editor/libeditor/text/nsTextEditUtils.cpp:134
18 	xul.dll 	nsHTMLEditor::Init 	mozilla/editor/libeditor/html/nsHTMLEditor.cpp:344
19 	xul.dll 	nsEditingSession::SetupEditorOnWindow 	mozilla/editor/composer/src/nsEditingSession.cpp:492
20 	xul.dll 	nsEditingSession::MakeWindowEditable 	mozilla/editor/composer/src/nsEditingSession.cpp:208
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-04 07:07:48 PDT
Created attachment 319273 [details]
stack from debug build
Comment 2 Chris Pearce (:cpearce) 2008-05-05 11:47:53 PDT
Works for me now. Martijn can you verify that this is fixed? I guess it was fixed by bug 430624.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-05 12:14:08 PDT
Still crashes for me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050506 Minefield/3.0pre
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-06 03:50:54 PDT
I'm also able to get the [@ nsEditor::EndUpdateViewBatch] crash again, with a slightly different testcase, but I think it's related to this.
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-06 04:39:57 PDT
Created attachment 319545 [details]
testcase2

This is crashing for me, with:
http://crash-stats.mozilla.com/report/index/46042e05-1b5f-11dd-b224-0013211cbf8a?p=1
0  	xul.dll  	nsEditor::EndUpdateViewBatch  	 mozilla/editor/libeditor/base/nsEditor.cpp:4373
1 	xul.dll 	nsHTMLEditor::EndUpdateViewBatch 	mozilla/editor/libeditor/html/nsHTMLEditor.cpp:5809
2 	xul.dll 	nsEditor::EndPlaceHolderTransaction 	mozilla/editor/libeditor/base/nsEditor.cpp:943
3 	xul.dll 	nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch 	mozilla/editor/libeditor/base/nsEditorUtils.h:66
4 	xul.dll 	nsHTMLEditor::InsertBasicBlock 	mozilla/editor/libeditor/html/nsHTMLEditor.cpp:2715
5 	xul.dll 	nsHTMLEditor::SetParagraphFormat 	mozilla/editor/libeditor/html/nsHTMLEditor.cpp:2181
6 	xul.dll 	nsCOMPtr<nsIHTMLEditor>::nsCOMPtr<nsIHTMLEditor> 	nsCOMPtr.h:645
7 	xul.dll 	nsParagraphStateCommand::SetState 	mozilla/editor/composer/src/nsComposerCommands.cpp:725
Comment 6 Chris Pearce (:cpearce) 2008-05-06 10:13:26 PDT
Yeah, I can reproduce it now, I think that second testcase is the same. Can you post the contents of the iframe un-data URI'd?

The problem (with the first testcase at least) is that when we turn on contentEditable for the frameset, it adds a bogus <br> to the editor contents, which triggers the event handler, which then removes the iframe and destroys and releases everything, then we carry on and use something that was destroyed and crash.
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-06 12:10:38 PDT
Contents of testcase2 iframe content:
<html xmlns="http://www.w3.org/1999/xhtml">
<frameset contenteditable="true"/>
<script>
function doExecCommand(){
document.execCommand('insertParagraph', false, '');
}
setTimeout(doExecCommand,100);
window.addEventListener('DOMNodeRemoved', function() {window.frameElement.parentNode.removeChild(window.frameElement);}, true);
</script>
</html>
Comment 8 Chris Pearce (:cpearce) 2008-05-09 16:18:00 PDT
Created attachment 320297 [details] [diff] [review]
v1

PeterV and I worked on this one together, so asking JST for review.

These two testcases are actually two slightly different crashes.

The crash in testcase 1 is caused by nsWebShell::EnsureCommandHandler() setting WebShell's mCommandManager field, but the commandManager's init is failing, leaving the command manager in an invalid state. We then come along later and try to use its hash table, which isn't init'd, causing the crash. The fix is to only set WebShell's mCommandManager field if it was successfully inititalized.

The crash in testcase 2 is caused by the presShell being null for iframe that's been destroyed. So I just added a null check to fix this.
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-06-18 12:33:26 PDT
Can the patch be checked in some tree?
Comment 10 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-06 15:33:09 PDT
Can the patch be checked in?
Comment 11 Chris Pearce (:cpearce) 2008-08-06 18:02:27 PDT
I was going to add a crashtest first. I get back tomorrow, I'll look at it then.
Comment 12 Chris Pearce (:cpearce) 2008-08-10 22:19:31 PDT
Comment on attachment 320297 [details] [diff] [review]
v1

This is still broken. If I apply the patch, testcase2 doesn't cause a crash, but when I add testcase2 as a crashtest, it still crashes with a stack overflow on calls to nsHTMLDocument::EditingStateChanged(). I'll need to do some more work on this...
Comment 13 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-11 08:08:35 PDT
Maybe testcase2 suffers from bug 446483?
Comment 14 Chris Pearce (:cpearce) 2008-10-29 18:01:16 PDT
(In reply to comment #13)
> Maybe testcase2 suffers from bug 446483?

Unfortunately not. With or without that patch, I still get a crash in editor\libeditor\base\crashtests\430624-1.html on Linux, but only if the crashtest window is focused. Yay.
Comment 15 Chris Pearce (:cpearce) 2008-10-31 13:22:46 PDT
(In reply to comment #14)
> I still get a crash in editor\libeditor\base\crashtests\430624-1.html on Linux.

I'm off by 1, it was the next test, editor\composer\src\crastests\428844-1.html
Comment 16 Chris Pearce (:cpearce) 2008-10-31 13:25:17 PDT
Created attachment 345781 [details] [diff] [review]
With style sheet owner fix

The crash was caused by nsHTMLEditor::EnableExistingStyleSheet() calling SetDisabled() on a style sheet. The style sheet has a reference to a document, which is set in nsHTMLEditor::AddOverrideStyleSheet() initially I think, which refers to a deceased document. nsCSSStyleSheet::SetDisabled() tries to addref that, causing the crash. I don't know why this only shows up on Linux when the mouse is over the window... The fix is to change EnableExistingStyleSheet() so that it sets the style sheet's owner to be the document being edited. So we always ensure that the style sheet belongs to our document before we enable it. I've added that change to this patch.
Comment 17 Daniel Veditz [:dveditz] 2009-01-08 08:28:42 PST
Marking sg:critical based on description in comment 6 although I'm not seeing the crash myself on a Mac.
Comment 18 Brandon Sterne (:bsterne) 2009-01-23 16:36:09 PST
Nominating for blocking 1.9.1 since there is a patch in progress already.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-27 17:03:21 PST
Peter, can we get a review here?
Comment 20 Peter Van der Beken [:peterv] - away till Aug 1st 2009-02-13 12:40:53 PST
Comment on attachment 345781 [details] [diff] [review]
With style sheet owner fix

>diff --git a/docshell/base/nsWebShell.cpp b/docshell/base/nsWebShell.cpp

>+    nsCOMPtr<nsPICommandUpdater> commandUpdater = do_CreateInstance("@mozilla.org/embedcomp/command-manager;1");

Rewrap.

>diff --git a/editor/libeditor/html/nsHTMLEditor.cpp b/editor/libeditor/html/nsHTMLEditor.cpp

>@@ -3685,31 +3679,43 @@ nsHTMLEditor::EnableStyleSheet(const nsA
>   nsCOMPtr<nsICSSStyleSheet> sheet;

>+  nsCOMPtr<nsIStyleSheet> isheet = do_QueryInterface(sheet);
>+  rv = isheet->SetOwningDocument(doc);

Don't think you need isheet, just use sheet (nsICSSStyleSheet inherits from nsIStyleSheet).

> nsHTMLEditor::EnableExistingStyleSheet(const nsAString &aURL)

>+    nsCOMPtr<nsIStyleSheet> isheet = do_QueryInterface(sheet);
>+    rv = isheet->SetOwningDocument(doc);
>+    NS_ENSURE_SUCCESS(rv, rv);

Same here.
Comment 21 Chris Pearce (:cpearce) 2009-02-16 18:23:38 PST
Created attachment 362649 [details] [diff] [review]
Patch v3

As previous with review comments addressed. r+/sr+ peterv.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-02-19 02:57:34 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/5302e1700561
Comment 23 Blake Kaplan (:mrbkap) 2009-02-25 15:50:33 PST
*** Bug 476975 has been marked as a duplicate of this bug. ***
Comment 24 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-02-25 16:52:03 PST
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090224 Minefield/3.2a1pre (.NET CLR 3.5.30729)
Comment 25 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-02-25 17:07:52 PST
Comment on attachment 362649 [details] [diff] [review]
Patch v3

a1.9.1=dbaron
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-02-26 13:07:30 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a96758bd69bc
Comment 27 Samuel Sidler (old account; do not CC) 2009-03-16 15:11:09 PDT
Chris: Can you please port this to 1.9.0? Code freeze for 1.9.0.8 is tomorrow at 11:59pm PDT.
Comment 28 Chris Pearce (:cpearce) 2009-03-16 15:19:39 PDT
Sure, I'll do that right now.
Comment 29 Chris Pearce (:cpearce) 2009-03-16 18:05:16 PDT
Created attachment 367695 [details] [diff] [review]
Patch ported to 1.9.0 branch

Patch v3 back ported to 1.9.0 branch (CVS HEAD).
Comment 30 Chris Pearce (:cpearce) 2009-03-16 18:09:23 PDT
I don't think that my hg access grants me cvs check-in priviledges... So I need someone to check this into 1.9.0 for me please.
Comment 31 Peter Van der Beken [:peterv] - away till Aug 1st 2009-03-17 09:37:04 PDT
Chris: please apply the patch from bug 483589 before checking this into the 1.9.0 branch.
Comment 32 Daniel Veditz [:dveditz] 2009-03-17 13:33:43 PDT
Comment on attachment 367695 [details] [diff] [review]
Patch ported to 1.9.0 branch

Please make the change peterv requested on checkin.

Approved for 1.9.0.8, a=dveditz for release-drivers
Comment 33 Chris Pearce (:cpearce) 2009-03-17 14:09:36 PDT
When I ssh to csv.mozilla.org I get: "Permission denied (publickey,gssapi-with-mic)." So someone else will have to check this and bug 483589 in to 1.9.0.
Comment 34 Blake Kaplan (:mrbkap) 2009-03-17 15:49:48 PDT
I checked this and bug 483589 into the 1.9.0 branch.
Comment 35 Al Billings [:abillings] 2009-03-23 13:28:26 PDT
Verified for 1.9.0.8 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8pre) Gecko/2009031805 GranParadiso/3.0.8pre (.NET CLR 3.5.30729). Crash cleanly occurs in 1.9.0.7.

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090323 Shiretoko/3.5b4pre.
Comment 36 Daniel Veditz [:dveditz] 2009-04-01 16:02:58 PDT
introduced post 1.8-branch
Comment 37 Marc Bejarano 2009-04-23 14:47:24 PDT
dveditz: duplicate bug 476975 needs to be opened, now
Comment 38 Marc Bejarano 2009-05-13 11:49:16 PDT
dveditz: ping
Comment 39 Frank Wein [:mcsmurf] 2009-08-23 16:20:37 PDT
Chris: Could you possibly take a look at Bug 510662? I think the bug fix here might have caused that regression, it changed that code and it's within the regression range. See Comment 7 in that bug for the code that gets called.
Comment 40 neil@parkwaycc.co.uk 2009-08-24 04:08:30 PDT
Comment on attachment 362649 [details] [diff] [review]
Patch v3

>+    // Ensure the style sheet is owned by our document.
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(mDocWeak);
This needs to be do_QueryReferent...

Note You need to log in before you can comment on or make changes to this bug.