If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash when accessing tree columns in nsXULTreeAccessible.cpp

VERIFIED FIXED

Status

()

Core
Disability Access APIs
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, crash})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
Crash reports:
http://crash-stats.mozilla.com/report/list?range_unit=weeks&query_search=signature&query_type=contains&version=Firefox%253A3.0b3pre%252CSeaMonkey%253A2.0a1pre%252CSunbird%253A0.6a1%252CThunderbird%253A3.0a1pre&signature=nsXULTreeAccessible%253A%253AInvalidateCache(int%252C+int)&query=Access&range_value=1

nsTreeBoxObject::GetColumns() might not set the out param yet still return NS_OK
http://mxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBoxObject.cpp#232
(Assignee)

Comment 1

10 years ago
Created attachment 297678 [details] [diff] [review]
1) Null check cols, 2) Don't return NS_OK unless you have set the out params

I didn't check other files but in general it seems dangerous to return NS_OK without setting out params.
Attachment #297678 - Flags: superreview?(neil)
Attachment #297678 - Flags: review?(neil)

Comment 2

10 years ago
Comment on attachment 297678 [details] [diff] [review]
1) Null check cols, 2) Don't return NS_OK unless you have set the out params

>+NS_IMETHODIMP nsTreeBoxObject::GetCellAt(PRInt32 aX, PRInt32 aY, PRInt32 *aRow, nsITreeColumn** aCol,
>+                                         nsACString& aChildElt)
> {
>+  *aRow = 0;
>+  *aCol = nsnull;
Nit: aChildElt.Truncate();
Attachment #297678 - Flags: superreview?(neil)
Attachment #297678 - Flags: superreview+
Attachment #297678 - Flags: review?(neil)
Attachment #297678 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #297678 - Flags: approval1.9?

Comment 3

10 years ago
Aaron, following our discussion on IRC, I checked nsXulTreeAccessible.cpp, and this is the only function that uses mTree before checking for its validity.
Do you still want me to file an extra bug on it, or could you put that null check in when you commit this patch?
It should follow the first if block I think, so:
>+  NS_ENSURE_TRUE(mTree && mTreeView, NS_ERROR_FAILURE); 
>   nsCOMPtr<nsITreeColumns> cols;
>   nsresult rv = mTree->GetColumns(getter_AddRefs(cols));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_STATE(cols);


(Assignee)

Comment 4

10 years ago
r+=aaron on the null check.

I'll just add the null check when I check it in.

Updated

10 years ago
Attachment #297678 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 5

10 years ago
Verified using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.