243809 - M17rc2 M18a1 mozilla crash at [@ nsHTMLEditor::CheckSelectionStateForAnonymousButtons]

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Louie Zhao]

Step to reproduce the crash of mozilla:

1. start gnopernicus
2. start mozilla
3. open composer window

Result: mozilla will crash

In nsHTMLEditor::CheckSelectionStateForAnonymousButtons, "GetSelectionContainer"
doesn't guarantee to return a "nsIDOMElement" successfuly. In the above case,
"focusElement" is set to null by "GetSelectionContainer", which caused mozilla
to crash when "focusElement->GetTagName".

Comment 1

[Louie Zhao]

Attachment: patch v1

Refer to 
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditor.cpp#185,
We need to check the result of "GetSelectionContainer" first before using it.

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 148654 [details] [diff] [review]
patch v1

r=[e-mail]

Comment 3

[Johnny Stenback (:jst)]

I question if not having a selection container is really an error here? Sure, we
shouldn't crash, but is returning NS_ERROR_NULL_POINTER the correct thing to do
here? Why not just NS_OK?

Comment 4

[Stephen Donner [:stephend] Not actively reading bugmail]

bug 212219   	cri   	P1   	All   	[e-mail]   	NEW   	  	Undo + Redo of
Table crashes [@ nsHTMLEditor::CheckSelect...

bug 219895 	cri 	-- 	All 	[e-mail] 	NEW 		Crash on table delete row

Anybody care if we DUP all the bugs to this, since we have a stack in this bug,
as well as a patch?

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 219895 has been marked as a duplicate of this bug. ***

Comment 6

[Jay Patel [:jay]]

*** Bug 244089 has been marked as a duplicate of this bug. ***

Comment 7

[Jay Patel [:jay]]

*** Bug 212219 has been marked as a duplicate of this bug. ***

Comment 8

[Jay Patel [:jay]]

Adding M17rc2 and topcrash keyword for tracking (from bug 244089).

Comment 9

[Louie Zhao]

I can't see the original purpose of the return value of function 
"CheckSelectionStateForAnonymousButtons" because the caller doesn't use the
return value at all except "EndUpdateViewBatch", which is used withouth checking
return value by caller also.

It's hard to judge whether NS_ERROR_NULL_POINTER or NS_OK should be used here.
Based on the current codebase, it doens't matter. Maybe the original author can
give us some hint.

Comment 10

[Scott MacGregor]

*** Bug 241242 has been marked as a duplicate of this bug. ***

Comment 11

[Scott MacGregor]

*** Bug 244092 has been marked as a duplicate of this bug. ***

Comment 12

[Scott MacGregor]

sounds like I'll want this patch on the aviary branch once it gets checked in

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 148654 [details] [diff] [review]
patch v1

@@ -203,6 +203,7 @@
   nsCOMPtr<nsIDOMElement> focusElement;
   // let's get the containing element of the selection
   nsresult res  = GetSelectionContainer(getter_AddRefs(focusElement));
+  if (!focusElement) return NS_ERROR_NULL_POINTER;

Ok, since noone spoke up with a good reason for returning an error code here,
I'd say return NS_OK here.

sr=jst with that change.

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Sorry for the delay, I was burried under urgent matters. Yeah, NS_OK is fine
too here. In fact, this code should really not been called at that time
and there is something weird far above, probably in the command updater, that
causes that call.

Comment 15

[Frank Wein [:mcsmurf]]

Comment on attachment 148654 [details] [diff] [review]
patch v1

this needs to get in 1.7, since it's a topcrasher

Comment 16

[Scott MacGregor]

*** Bug 244451 has been marked as a duplicate of this bug. ***

Comment 17

[Louie Zhao]

Patch checked in trunk.

Comment 18

[Scott MacGregor]

fixed on the aviary 1.0 branch

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 148654 [details] [diff] [review]
patch v1

a=asa (on behalf of drivers) for checkin to 1.7

Comment 20

[Frank Wein [:mcsmurf]]

Louie Zhao: Can you check this in to 1.7 branch then? Don't forget to add the
fixed1.7 afterwards, thanks!

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 244652 has been marked as a duplicate of this bug. ***

Comment 22

[Louie Zhao]

patch checked in 1.7 Branch.

Comment 23

[R.K.Aa.]

*** Bug 245006 has been marked as a duplicate of this bug. ***

Comment 24

[R.K.Aa.]

*** Bug 245187 has been marked as a duplicate of this bug. ***

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 245298 has been marked as a duplicate of this bug. ***

Comment 26

[Bernard Alleysson]

*** Bug 245378 has been marked as a duplicate of this bug. ***

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 245784 has been marked as a duplicate of this bug. ***

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on the trunk using build 2004-06-30-08 on Windows XP.

Comment 29

[Jay Patel [:jay]]

Adding M18a1 to summary for tracking.  This was a topcrasher for Mozilla 1.8
alpha 1.

Comment 30

[R.K.Aa.]

*** Bug 251478 has been marked as a duplicate of this bug. ***

Comment 31

[R.K.Aa.]

*** Bug 247791 has been marked as a duplicate of this bug. ***

Comment 32

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 239908 has been marked as a duplicate of this bug. ***