85504 - M091 Editor crash [@ nsEditorShell::GetSelectedElement]

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[greer]

M091 talkback user reports point to a bug in the composer crashing at 
nsEditorShell::GetSelectedElement.

Here are some user comments and the stack:

(31593679) Comments: (see previous Talkback submittals) - in fact the main
body of the file (rather than just the banner
     (31593513) Comments: (see previous Talkback submittals) - clicking on the
topmost banner of \%windir%\readme.htm with either left or right mouse button
causes the crash
     (31593463) Comments: (see previous Talkback submittal) just the one Moz
window open this time
     (31593404) Comments: had a couple of Moz navigator windows open
     (31545053) Comments: editing local page
     (31531837) Comments: I had a webpage open in composer and attempted to
toggle between the different views.
     (31528143) Comments: I was using composer.I loaded a webpage saved from
http://www.e-zim.com/ but nothing appeared in the composer window.  I selected a
table element and tried to see if I could put a table into the apparently blank
document at which point it crashed.
     (31507922) URL: www.city.academic.gr
     (31507922) Comments: Just poking around...
     (31473971) Comments: Opening a saved HTML file (saved by IE5) on Composer.


Stack Trace: 

         nsEditorShell::GetSelectedElement
[d:\builds\seamonkey\mozilla\editor\base\nsEditorShell.cpp  line 3471]
         XPTC_InvokeByIndex
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp 
line 139]
         XPCWrappedNative::CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp  line
1837]
         XPC_WN_CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp 
line 1242]
         js_Invoke      [d:\builds\seamonkey\mozilla\js\src\jsinterp.c  line 
809]
         js_Interpret   [d:\builds\seamonkey\mozilla\js\src\jsinterp.c  line 
2703]
         js_Invoke      [d:\builds\seamonkey\mozilla\js\src\jsinterp.c  line 
825]
         js_InternalInvoke      [d:\builds\seamonkey\mozilla\js\src\jsinterp.c  
line 897]
         JS_CallFunctionValue   [d:\builds\seamonkey\mozilla\js\src\jsapi.c  
line 3322]
         nsJSContext::CallEventHandler
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp  line 937]
         nsJSEventListener::HandleEvent
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp  line 140]
         nsEventListenerManager::HandleEventSubType
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp  line
1120]
         nsEventListenerManager::HandleEvent
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp  line
2090]
         nsXULElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp  line
3631]
         PresShell::HandleDOMEventWithTarget
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp  line 5562]
         nsPopupSetFrame::OnCreate
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsPopupSetFrame.cpp  line 653]
         nsPopupSetFrame::CreatePopup
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsPopupSetFrame.cpp  line 460]
         nsPopupSetBoxObject::CreatePopup
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsPopupSetBoxObject.cpp  line
140]
         XULPopupListenerImpl::LaunchPopup
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULPopupListener.cpp 
line 841]
         XULPopupListenerImpl::LaunchPopup
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULPopupListener.cpp 
line 689]
         XULPopupListenerImpl::PreLaunchPopup
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULPopupListener.cpp 
line 352]
         XULPopupListenerImpl::ContextMenu
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULPopupListener.cpp 
line 291]
         nsEventListenerManager::HandleEvent
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp  line
1403]
         nsXULElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp  line
3631]
         nsXULElement::HandleChromeEvent
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp  line
4595]
         GlobalWindowImpl::HandleDOMEvent
[d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp  line 619]
         nsDocument::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsDocument.cpp  line 2882]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp  line 1696]
         nsGenericHTMLElement::HandleDOMEventForAnchors
[d:\builds\seamonkey\mozilla\content\html\content\src\nsGenericHTMLElement.cpp 
line 1148]
         nsHTMLAnchorElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLAnchorElement.cpp 
line 377]
         nsGenericDOMDataNode::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericDOMDataNode.cpp  line
678]
         nsDOMDocumentType::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsDOMDocumentType.cpp  line 234]
         PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp  line 5519]
         PresShell::HandleEvent
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp  line 5441]
         nsView::HandleEvent    
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp  line
377]
         nsView::HandleEvent    
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp  line
350]
         nsView::HandleEvent    
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp  line
350]
         nsViewManager::DispatchEvent
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp  line 2051]
         HandleEvent    [d:\builds\seamonkey\mozilla\view\src\nsView.cpp  line 
68]
         nsWindow::DispatchEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp  line 716]
         nsWindow::DispatchWindowEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp  line 733]
         nsWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp  line 4197]
         ChildWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp  line 4442]
         nsWindow::ProcessMessage
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp  line 3176]
         nsWindow::WindowProc
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp  line 980]
         KERNEL32.DLL + 0x363b (0xbff7363b)
         KERNEL32.DLL + 0x24407 (0xbff94407)

Comment 1

[greer]

Adding crash, topcrash keywords for tracking. Also qawanted.

Comment 2

[rubydoo123]

reporter: please include steps on how to reproduce this

Comment 3

[greer]

I don't have a Win98 machine to repro this one, the user comments would be the
best place to start: 

(31528143) Comments: I was using composer.I loaded a webpage saved from
http://www.e-zim.com/ but nothing appeared in the composer window.  I selected a
table element and tried to see if I could put a table into the apparently blank
document at which point it crashed.

The QA contact may be able to help.

Comment 4

[Chase Tingley]

I can reproduce this on 2001060703/WinMe by following those steps: saving
http://www.e-zom.com/ to the desktop, then reopining it with composer (page was
blank), and crashing it with the table tool.

Comment 5

[rubydoo123]

reviewed and approved

Comment 6

[rubydoo123]

to clarify my last entry:

manager reviewed the need for the fix and agrees, approval for checkin to the
trunk and branch after fix has received an r= and sr=, adding nsBranch keyword

Comment 7

[Simon Fraser [no longer active]]

I think what's happening here is that the page fails to load, because the
www.e-zim.com is a frameset site, where the frames are referenced using relative
URLs. So the subframes fail to load, and this somehow bypasses the code that
editor has for frameset detection. We actually never make an editor on this
page, so all the commands should remain disabled, but somehow the user succeeds
in calling nsEditorShell::GetSelectedElement, which dereferences a null mEditor,
and thus crashes.

So there are at least two bugs here:
1. We fail to put up the 'frameset' warning when loading subframes fails.
2. On Win 98, you can execute commands that should be disabled.3. Some
nsEditorShell API calls don't checke for a null editor.

Comment 8

[Simon Fraser [no longer active]]

Attachment: nsEditorShell.cp fix

Comment 9

[Simon Fraser [no longer active]]

I just noticed that the original testcase is a non-frameset page (http://
www.city.academic.gr), tho that page does have a plugin. But I can't repro the 
crash on that page.

This patch fixes the missing subframes problem, and does two things:

1. Bullet-proofs every API method that calls methods on mEditor will a NULL
   check. GetSelectedElement happened to be the one that we crashed in because
   it's called from command updating code (e.g. when you click on the menu bar).

2. The testcase was unusual in that, because both subframes fail to load, we
   got no OnProgress notifications before the page end notifcation, so we failed
   to check for frameset tags. I added a call to the parser observer to check
   for bad tags that will always get hit now. So, with this patch, composer
   refuses the edit the page because it contains a frameset.

Akkana, can you r=? Blake, sr?

Comment 10

[Akkana Peck]

The changes look fine.  (Do you want to reformat that clause at the end now that
it has one fewer level of braces?  Or is that just not showing up in the patch?)
r=akkana

Comment 11

[Simon Fraser [no longer active]]

waterson, sr please?

Comment 12

[Chris Waterson]

Worth asserting when mEditor == 0? (Your comments on 2001-07-02 17:42 make me 
feel like this may be covering up another problem...)

sr=waterson, either way.

Comment 13

[rubydoo123]

nsBranch+

Comment 14

[Simon Fraser [no longer active]]

Random keyword shit.

Comment 15

[lchiang]

mark PDT+ to check into the branch. If possible, pls get this in for Friday am
builds.

Comment 16

[Simon Fraser [no longer active]]

Now fixed on trunk too!

Comment 17

[sujay]

Editor no longer crashes when editing http://www.e-zim.com and we display
the appropriate frame alert panel when doing File | Edit Page on a frame
doc and also Opening the file from within composer.

verified on 7/10 trunk build. removing vtrunk keyword.

verified on 7/10 branch, marking VERIFIED-FIXED.

Opened a new bug because when you browse a page with frames(http://www.e-zim.com
and then do Save As from context menu, we should throw a frame alert panel.
Instead we save it out as a blank NOFRAME page) Filed bug 90204 for this
behavior.