552470 - Crash [@nsXULTreeItemAccessible::GetRoleInternal]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: null check.

nsCoreUtils::GetFirstSensibleColumn can return null...

https://crash-stats.mozilla.com/report/index/9aebbb6a-1093-40a4-91fe-a5ace2100311

Comment 1

[Marco Zehe (:MarcoZ)]

Is this because of a malformed XUL tree or what's going on?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Beats me.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 432603 [details] [diff] [review]
null check.


>   nsCOMPtr<nsITreeColumn> column =
>     nsCoreUtils::GetFirstSensibleColumn(mTree);
> 
>   PRBool isPrimary = PR_FALSE;
>-  column->GetPrimary(&isPrimary);
>+
>+  if (column) {
>+    column->GetPrimary(&isPrimary);
>+  }
> 

I wonder why don't we use nsITreeColumns::getPrimaryColumn()?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #3)
> (From update of attachment 432603 [details] [diff] [review])
> 
> >   nsCOMPtr<nsITreeColumn> column =
> >     nsCoreUtils::GetFirstSensibleColumn(mTree);
> > 
> >   PRBool isPrimary = PR_FALSE;
> >-  column->GetPrimary(&isPrimary);
> >+
> >+  if (column) {
> >+    column->GetPrimary(&isPrimary);
> >+  }
> > 
> 
> I wonder why don't we use nsITreeColumns::getPrimaryColumn()?

I think we should! Nice catch.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 432603 [details] [diff] [review] [details])
> > 
> > >   nsCOMPtr<nsITreeColumn> column =
> > >     nsCoreUtils::GetFirstSensibleColumn(mTree);
> > > 
> > >   PRBool isPrimary = PR_FALSE;
> > >-  column->GetPrimary(&isPrimary);
> > >+
> > >+  if (column) {
> > >+    column->GetPrimary(&isPrimary);
> > >+  }
> > > 
> > 
> > I wonder why don't we use nsITreeColumns::getPrimaryColumn()?
> 
> I think we should! Nice catch.

It won't account for hidden columns though... is this a concern?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

I guess I will just change nsXULTreeItemAccessible::GetRoleInternal and leave GetFirstSensibleColumn as is.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

New patch will come after lunch :)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> 
> It won't account for hidden columns though... is this a concern?

Yes if it can be hidden. I know it can't be hidden from UI. But it's worth to check if it can be programmatically.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: use GetPrimaryFrame

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Created an attachment (id=433366) [details]
> use GetPrimaryFrame

column? It would be nice to know if primary column can be presented and hidden. I think nsITreeColumns object should care about this but it's worth to check. Could you do this please?

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=433366) [details] [details]
> > use GetPrimaryFrame
> 
> column? It would be nice to know if primary column can be presented and hidden.
> I think nsITreeColumns object should care about this but it's worth to check.

nsITreeColumns doesn't care.

> Could you do this please?

Maybe but I'm not exactly sure what you are asking :)

Note nsXULTreeGridAccessible does the same thing I'm doing in this patch.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Ultimately the columns are looped through until IsPrimary succeeds:
PRBool IsPrimary() { return mIsPrimary; }

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Aha we are okay, thanks surkov. nsITreeColumns cares about visibility in EnsureColumns.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 433366 [details] [diff] [review]
use GetPrimaryFrame

r=me - nsITreeColumns cares the primary column is always visible so that's what we need.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Landed on central: http://hg.mozilla.org/mozilla-central/rev/126bdd9262b5