Closed Bug 243809 Opened 20 years ago Closed 20 years ago

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

Categories

(SeaMonkey :: Composer, defect)

Product:
SeaMonkey
Component:
Composer
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: Louie.Zhao, Unassigned)

References

Details

(Keywords: crash, fixed1.7, topcrash, Whiteboard: fixed-aviary1.0 fixed1.7)

Crash Data

Attachments

(1 file)

patch v1
737 bytes, patch
glazou
: review+
jst
: superreview+
asa
: approval1.7+
Details | Diff | Splinter Review 
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".
Attached patch patch v1 — — Splinter Review 
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.
Attachment #148654 - Flags: superreview?(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?
bug 212219   	cri   	P1   	All   	daniel@glazman.org   	NEW   	  	Undo + Redo of
Table crashes [@ nsHTMLEditor::CheckSelect...

bug 219895 	cri 	-- 	All 	composer@editor.bugs 	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?
*** Bug 219895 has been marked as a duplicate of this bug. ***
Summary: mozilla crash at nsHTMLEditor::CheckSelectionStateForAnonymousButtons → mozilla crash at [@ nsHTMLEditor::CheckSelectionStateForAnonymousButtons]
Severity: normal → critical
Keywords: crash
*** Bug 244089 has been marked as a duplicate of this bug. ***
*** Bug 212219 has been marked as a duplicate of this bug. ***
Adding M17rc2 and topcrash keyword for tracking (from bug 244089).
Keywords: topcrash
Summary: mozilla crash at [@ nsHTMLEditor::CheckSelectionStateForAnonymousButtons] → M17rc2 mozilla crash at [@ nsHTMLEditor::CheckSelectionStateForAnonymousButtons]
Flags: blocking1.7?
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.
*** Bug 241242 has been marked as a duplicate of this bug. ***
*** Bug 244092 has been marked as a duplicate of this bug. ***
sounds like I'll want this patch on the aviary branch once it gets checked in
Flags: blocking1.7? → blocking1.7+
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.
Attachment #148654 - Flags: superreview?(jst) → superreview+
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.
OS: Linux → All
Comment on attachment 148654 [details] [diff] [review]
patch v1

this needs to get in 1.7, since it's a topcrasher
Attachment #148654 - Flags: approval1.7?
*** Bug 244451 has been marked as a duplicate of this bug. ***
Patch checked in trunk.
fixed on the aviary 1.0 branch
Whiteboard: fixed-aviary1.0
Comment on attachment 148654 [details] [diff] [review]
patch v1

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #148654 - Flags: approval1.7? → approval1.7+
Louie Zhao: Can you check this in to 1.7 branch then? Don't forget to add the
fixed1.7 afterwards, thanks!
*** Bug 244652 has been marked as a duplicate of this bug. ***
patch checked in 1.7 Branch.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: fixed-aviary1.0 → fixed-aviary1.0 fixed1.7
Keywords: fixed1.7
*** Bug 245006 has been marked as a duplicate of this bug. ***
*** Bug 245187 has been marked as a duplicate of this bug. ***
*** Bug 245298 has been marked as a duplicate of this bug. ***
*** Bug 245378 has been marked as a duplicate of this bug. ***
*** Bug 245784 has been marked as a duplicate of this bug. ***
Verified FIXED on the trunk using build 2004-06-30-08 on Windows XP.
Status: RESOLVED → VERIFIED
Adding M18a1 to summary for tracking.  This was a topcrasher for Mozilla 1.8
alpha 1.
Summary: M17rc2 mozilla crash at [@ nsHTMLEditor::CheckSelectionStateForAnonymousButtons] → M17rc2 M18a1 mozilla crash at [@ nsHTMLEditor::CheckSelectionStateForAnonymousButtons]
*** Bug 251478 has been marked as a duplicate of this bug. ***
*** Bug 247791 has been marked as a duplicate of this bug. ***
*** Bug 239908 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
Crash Signature: [@ nsHTMLEditor::CheckSelectionStateForAnonymousButtons]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: