Closed
Bug 432114
Opened 16 years ago
Closed 15 years ago
Crash [@ PL_DHashTableOperate][@ nsEditor::EndUpdateViewBatch] with DOMNodeInserted event listener removing window and frameset contenteditable
Categories
(Core :: DOM: Editor, defect, P3)
Tracking
()
VERIFIED
FIXED
People
(Reporter: martijn.martijn, Assigned: cpearce)
References
Details
(5 keywords, Whiteboard: [sg:critical?] post 1.8-branch)
Crash Data
Attachments
(5 files, 2 obsolete files)
494 bytes,
text/html
|
Details | |
5.95 KB,
text/plain
|
Details | |
794 bytes,
text/html
|
Details | |
5.24 KB,
patch
|
cpearce
:
review+
cpearce
:
superreview+
dbaron
:
approval1.9.1+
|
Details | Diff | Splinter Review |
7.52 KB,
patch
|
cpearce
:
review+
cpearce
:
superreview+
dveditz
:
approval1.9.0.9+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•16 years ago
|
||
Assignee | ||
Comment 2•16 years ago
|
||
Works for me now. Martijn can you verify that this is fixed? I guess it was fixed by bug 430624.
Reporter | ||
Comment 3•16 years ago
|
||
Still crashes for me, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050506 Minefield/3.0pre
Reporter | ||
Comment 4•16 years ago
|
||
I'm also able to get the [@ nsEditor::EndUpdateViewBatch] crash again, with a slightly different testcase, but I think it's related to this.
Reporter | ||
Comment 5•16 years ago
|
||
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
Reporter | ||
Updated•16 years ago
|
Summary: Crash [@ PL_DHashTableOperate] with DOMNodeInserted event listener removing window and frameset contenteditable → Crash [@ PL_DHashTableOperate][@ nsEditor::EndUpdateViewBatch] with DOMNodeInserted event listener removing window and frameset contenteditable
Assignee | ||
Comment 6•16 years ago
|
||
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.
Reporter | ||
Comment 7•16 years ago
|
||
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>
Assignee | ||
Comment 8•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #320297 -
Flags: superreview+
Attachment #320297 -
Flags: review?(jst)
Attachment #320297 -
Flags: review+
Reporter | ||
Comment 9•16 years ago
|
||
Can the patch be checked in some tree?
Reporter | ||
Comment 10•16 years ago
|
||
Can the patch be checked in?
Assignee | ||
Comment 11•16 years ago
|
||
I was going to add a crashtest first. I get back tomorrow, I'll look at it then.
Assignee | ||
Comment 12•16 years ago
|
||
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...
Attachment #320297 -
Attachment is obsolete: true
Reporter | ||
Comment 13•16 years ago
|
||
Maybe testcase2 suffers from bug 446483?
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.9.1?
Flags: blocking1.9.0.3?
Flags: blocking1.9.1? → wanted1.9.1+
Updated•16 years ago
|
Flags: blocking1.9.0.4? → wanted1.9.0.x+
Assignee | ||
Comment 14•16 years ago
|
||
(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.
Assignee | ||
Comment 15•16 years ago
|
||
(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
Assignee | ||
Comment 16•16 years ago
|
||
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.
Attachment #345781 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #345781 -
Flags: review? → review?(peterv)
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.9.2?
Comment 17•16 years ago
|
||
Marking sg:critical based on description in comment 6 although I'm not seeing the crash myself on a Mac.
Whiteboard: [sg:critical?]
Comment 18•15 years ago
|
||
Nominating for blocking 1.9.1 since there is a patch in progress already.
Flags: blocking1.9.1?
Peter, can we get a review here?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P3
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Comment 20•15 years ago
|
||
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.
Attachment #345781 -
Flags: superreview+
Attachment #345781 -
Flags: review?(peterv)
Attachment #345781 -
Flags: review+
Assignee | ||
Comment 21•15 years ago
|
||
As previous with review comments addressed. r+/sr+ peterv.
Attachment #345781 -
Attachment is obsolete: true
Attachment #362649 -
Flags: superreview+
Attachment #362649 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?][needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/5302e1700561
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs 191 approval]
Attachment #362649 -
Flags: approval1.9.1?
Reporter | ||
Comment 24•15 years ago
|
||
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)
Status: RESOLVED → VERIFIED
Attachment #362649 -
Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 362649 [details] [diff] [review] Patch v3 a1.9.1=dbaron
Whiteboard: [sg:critical?][needs 191 approval] → [sg:critical?][needs 191 landing]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a96758bd69bc
Flags: blocking1.9.2?
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs 191 landing] → [sg:critical?]
Updated•15 years ago
|
Flags: blocking1.9.0.8?
Updated•15 years ago
|
Flags: blocking1.9.0.8? → blocking1.9.0.8+
Comment 27•15 years ago
|
||
Chris: Can you please port this to 1.9.0? Code freeze for 1.9.0.8 is tomorrow at 11:59pm PDT.
Assignee | ||
Comment 28•15 years ago
|
||
Sure, I'll do that right now.
Assignee | ||
Comment 29•15 years ago
|
||
Patch v3 back ported to 1.9.0 branch (CVS HEAD).
Attachment #367695 -
Flags: superreview+
Attachment #367695 -
Flags: review+
Attachment #367695 -
Flags: approval1.9.0.8?
Assignee | ||
Comment 30•15 years ago
|
||
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.
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?][needs landing][needs landing 1.9.0]
Whiteboard: [sg:critical?][needs landing][needs landing 1.9.0] → [sg:critical?][needs landing 1.9.0]
Comment 31•15 years ago
|
||
Chris: please apply the patch from bug 483589 before checking this into the 1.9.0 branch.
Comment 32•15 years ago
|
||
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
Attachment #367695 -
Flags: approval1.9.0.8? → approval1.9.0.8+
Updated•15 years ago
|
Whiteboard: [sg:critical?][needs landing 1.9.0] → [sg:critical?][needs landing 1.9.0=dveditz]
Assignee | ||
Comment 33•15 years ago
|
||
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•15 years ago
|
||
I checked this and bug 483589 into the 1.9.0 branch.
Keywords: fixed1.9.0.8
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs landing 1.9.0=dveditz] → [sg:critical?]
Comment 35•15 years ago
|
||
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•15 years ago
|
||
introduced post 1.8-branch
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
Updated•15 years ago
|
Group: core-security
Comment 37•15 years ago
|
||
dveditz: duplicate bug 476975 needs to be opened, now
Comment 38•15 years ago
|
||
dveditz: ping
Comment 39•15 years ago
|
||
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.
Depends on: 510662
Comment 40•15 years ago
|
||
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...
Updated•13 years ago
|
Crash Signature: [@ PL_DHashTableOperate]
[@ nsEditor::EndUpdateViewBatch]
You need to log in
before you can comment on or make changes to this bug.
Description
•