428844 - Crash [@ nsEditingSession::TearDownEditorOnWindow] on reload with contenteditable and xslt

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build on reload.

This seems to have regressed between 2008-02-19 and 2008-02-20:
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-02-19+04&maxdate=2008-02-20+09&cvsroot=%2Fcvsroot
I guess a regression from bug 412920.

http://crash-stats.mozilla.com/report/index/f360bf28-09a4-11dd-af7f-001cc45a2ce4
0  	xul.dll  	nsEditingSession::TearDownEditorOnWindow  	 mozilla/editor/composer/src/nsEditingSession.cpp:576
1 	xul.dll 	nsEditingSession::StartDocumentLoad 	mozilla/editor/composer/src/nsEditingSession.cpp:1022
2 	xul.dll 	nsEditingSession::OnStateChange 	mozilla/editor/composer/src/nsEditingSession.cpp:804
3 	xul.dll 	xul.dll@0x81c1ab

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - only tear down if QI succeeds

Ok, so what's happening here is that we're assuming that all editable documents are html documents. We've got an nsXMLDocument here, and we're QI'ing it to an htmlDoc, and not checking that the QI succeeds before calling nsHTMLDocument::TearingDownEditor() on it, and it's crashing. We should only call TearingDownEditor() if we've got an html doc, and editing is on.

Comment 2

[Peter Van der Beken [:peterv]]

Martijn, can you file a bug that XSLT should create an HTML document for this testcase?

Comment 3

[Martijn Wargers (dead)]

Isn't that basically the same as bug 61675?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - v1 with crashtest

Same as patch v1, but with crashtest. r+/sr+ Peterv.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 316134 [details] [diff] [review]
Patch - v1 with crashtest

Low risk patch, fixes a crash, patch includes crashtest.

Comment 6

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

Comment on attachment 316134 [details] [diff] [review]
Patch - v1 with crashtest

a1.9=beltzner

Comment 7

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

mozilla/editor/composer/src/nsEditingSession.cpp 	1.59
mozilla/editor/composer/src/crashtests/428844-1-inner.xhtml 	1.1
mozilla/editor/composer/src/crashtests/428844-1.html 	1.1
mozilla/editor/composer/src/crashtests/crashtests.list 	1.1
mozilla/testing/crashtest/crashtests.list 	1.36

Comment 8

[Boris Zbarsky [:bzbarsky]]

The test added in this bug doesn't seem to really test what it wants to (in particular, the test ends before the subframe has a chance to be reloaded)....

Chris, do you want a separate bug on that?  Or is the .src set not really needed?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #8)
> The test added in this bug doesn't seem to really test what it wants to (in
> particular, the test ends before the subframe has a chance to be reloaded)....
> 
> Chris, do you want a separate bug on that?  Or is the .src set not really
> needed?

May as well file a separate bug, since this one was closed almost 3 years ago. We may be able to re-enable this test while we're at it, and resolve bug 471185 too.

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to comment #8)
> > The test added in this bug doesn't seem to really test what it wants to (in
> > particular, the test ends before the subframe has a chance to be reloaded)....
> > 
> > Chris, do you want a separate bug on that?  Or is the .src set not really
> > needed?
> 
> May as well file a separate bug, since this one was closed almost 3 years ago.
> We may be able to re-enable this test while we're at it, and resolve bug 471185
> too.

Boris, feel free to assign the new bug to me!  :-)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Filed bug 627624.