Closed Bug 673403 Opened 13 years ago Closed 13 years ago

don't rely on non empty result from nsAccessible::GetParent()

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: surkov, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

1) while you're here rename GetParent() to Parent() to correspond name conversion, that has side effect that guarantees us no one call is missed and makes reviewer work simpler
2) null check for everything that doesn't belong to children traversal
this bug leads to crashes like this (get object attributes when hide event is handled):

xul.dll!nsAccessible::GetLevelInternal()  Line 3426     C++
       xul.dll!nsAccessible::GroupPosition(int * aGroupLevel, int * aSimilarItemsInGroup, int * aPositionInGroup)  Line 1506   C++
       xul.dll!nsAccessible::GetAttributes(nsIPersistentProperties * * aAttributes)  Line 1340 C++
       xul.dll!nsAccessibleWrap::get_attributes(wchar_t * * aAttributes)  Line 1455    C++
>      AT stack trace
       user32.dll!___ClientCallWinEventProc@4()  + 0x2a bytes
       ntdll.dll!_KiUserCallbackDispatcher@12()  + 0x2e bytes
       user32.dll!_NtUserNotifyWinEvent@16()  + 0xc bytes
       xul.dll!nsAccessibleWrap::NotifyWinEvent(unsigned long event, HWND__ * hwnd, long idObjectType, long idObject)  Line 188 + 0x10 bytes   C++
       xul.dll!nsAccessibleWrap::FirePlatformEvent(AccEvent * aEvent)  Line 1602       C++
       xul.dll!nsAccessibleWrap::HandleAccEvent(AccEvent * aEvent)  Line 1537 + 0xe bytes      C++
       xul.dll!nsHyperTextAccessibleWrap::HandleAccEvent(AccEvent * aEvent)  Line 77 + 0x8 bytes       C++
       xul.dll!nsSVGCircleElement::AddRef()  Line 93 + 0xc bytes       C++
       xul.dll!nsCOMPtr_base::assign_with_AddRef(nsISupports * rawPtr)  Line 90        C++
       xul.dll!nsEventShell::FireEvent(AccEvent * aEvent)  Line 65     C++
       xul.dll!nsDocAccessible::ProcessPendingEvent(AccEvent * aEvent)  Line 1746      C++
       xul.dll!NotificationController::WillRefresh(mozilla::TimeStamp aTime)  Line 306 C++
Attached patch pass 1 (obsolete) — Splinter Review
I'll end up bit rotting this a little, but most of it should be fine.
Attachment #547906 - Flags: review?(surkov.alexander)
Comment on attachment 547906 [details] [diff] [review]
pass 1

Review of attachment 547906 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +831,5 @@
>          if (!accWrap) {
>              return nsnull;
>          }
>  
> +        nsAccessible* accParent = accWrap->Parent();

didn't you considered to fix indentation here?

::: accessible/src/base/TextUpdater.cpp
@@ +73,2 @@
>    if (!parent) {
>      NS_ERROR("No parent for text leaf!");

it's time to remove warning because if text update triggers and then node is removed from DOM, then we get this case. Sounds right?

::: accessible/src/base/nsARIAGridAccessible.cpp
@@ +829,5 @@
>    // siblings cells.
>    if (role == nsIAccessibleRole::ROLE_GRID_CELL ||
>        role == nsIAccessibleRole::ROLE_ROWHEADER ||
>        role == nsIAccessibleRole::ROLE_COLUMNHEADER) {
> +    nsAccessible *row = aAccessible->Parent();

nsAccessible* row

@@ +1095,5 @@
>  
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsAccessible *row = Parent();

nsAccessible* row

::: accessible/src/base/nsAccUtils.cpp
@@ +107,5 @@
>    if (role == nsIAccessibleRole::ROLE_OUTLINEITEM)
>      return 1;
>  
>    if (role == nsIAccessibleRole::ROLE_ROW) {
> +    nsAccessible *parent = aAccessible->Parent();

nsAccessible* parent

::: accessible/src/base/nsAccessible.cpp
@@ +2143,5 @@
>          if (view) {
>            nsIScrollableFrame *scrollFrame = do_QueryFrame(frame);
> +          nsAccessible* parent = Parent();
> +          if (scrollFrame || view->GetWidget() || !frame->GetParent()
> +              && parent)

this is wrong, no accessible parent should mean there's no node_child_of relation in parent chain I think. Operators shouldn't be on line start

@@ +3294,5 @@
>      // between parent listitem and nested list.
>  
>      // Calculate 'level' attribute based on number of parent listitems.
>      level = 0;
> +nsAccessible* parent = nsnull;

fix indentation please
and maybe it's worth to keep existing code, that allows you do
if (parent && level == 0)

@@ +3317,5 @@
>        for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) {
>          nsAccessible* sibling = parent->GetChildAt(siblingIdx);
>  
> +        nsAccessible* siblingChild =
> +          sibling->GetChildAt(sibling->GetChildCount() - 1);

could you add LastChild and FirstChild to nsAccessible, sounds like nice to have

@@ +3319,5 @@
>  
> +        nsAccessible* siblingChild =
> +          sibling->GetChildAt(sibling->GetChildCount() - 1);
> +        if (siblingChild
> +            && siblingChild->Role() == nsIAccessibleRole::ROLE_LIST)

operator to the end of line

::: accessible/src/base/nsBaseWidgetAccessible.cpp
@@ +238,2 @@
>      if (walkUpAcc && walkUpAcc->Role() == nsIAccessibleRole::ROLE_LINK &&
>          walkUpAcc->State() & states::LINKED) {

while you are here, please remove walkUpAcc from if statement

::: accessible/src/base/nsDocAccessible.cpp
@@ +1499,5 @@
>    // this reorder event is processed by parent document then events targeted to
>    // this document may be fired prior to this reorder event. If this is
>    // a problem then consider to keep event processing per tab document.
> +  nsAccessible* parent = Parent();
> +  if (!IsRoot() && parent) {

no need to check, if you like then you can assert. NotifyOfInitialUpdate can't be called for document is not in subtree

::: accessible/src/base/nsRootAccessible.cpp
@@ +698,5 @@
>          // It is a top level menuitem. Only fire a focus event when the menu bar
>          // is active.
>          return;
>        } else {
> +        nsAccessible *containerAccessible = accessible->Parent();

nsAccessible* container

@@ +705,5 @@
>          // It is not top level menuitem
>          // Only fire focus event if it is not inside collapsed popup
>          // and not a listitem of a combo box
>          if (containerAccessible->State() & states::COLLAPSED) {
> +          nsAccessible *containerParent = containerAccessible->Parent();

nsAccessible* containerParent

@@ +795,5 @@
>  
>      // If ancestor chain of accessibles is not completely visible,
>      // don't use this one. This happens for example if it's inside
>      // a background tab (tabbed browsing)
> +    nsAccessible *parent = accDoc->Parent();

nsAccessible* parent

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +407,5 @@
>      // This means we're part of another control, so use parent accessible for name.
>      // This ensures that a textbox inside of a XUL widget gets
>      // an accessible name.
> +    nsAccessible* parent = Parent();
> +    NS_ENSURE_TRUE(parent, NS_ERROR_FAILURE);

no failure, just
if (parent)
  parent->GetName(

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +386,3 @@
>      NS_ASSERTION(parent, "No parent!");
> +    if (!parent)
> +      return NS_ERROR_FAILURE;

no assertion and NS_OK please

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +622,5 @@
>  
>    // From the descendant, go up and get the immediate child of this hypertext
>    nsAccessible *childAccAtOffset = nsnull;
>    while (descendantAcc) {
> +    nsAccessible *parentAcc = descendantAcc->Parent();

nsAccessible* parent =

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +577,1 @@
>    NS_ENSURE_TRUE(parent,);

if (!parent)
  return NS_OK;

::: accessible/src/xul/nsXULMenuAccessible.cpp
@@ +331,5 @@
>      item->GetSelected(&isSelected);
>  
>      // Is collapsed?
>      PRBool isCollapsed = PR_FALSE;
> +    nsAccessible* parentAcc = Parent();

maybe rename it to parent while you're here

@@ +341,5 @@
>  
>        // Selected and collapsed?
>        if (isCollapsed) {
>          // Set selected option offscreen/invisible according to combobox state
> +        nsAccessible* grandParentAcc = parentAcc->Parent();

to grandParent

@@ +346,1 @@
>          NS_ENSURE_TRUE(grandParentAcc, state);

if (!grandParent)
  return NS_OK

@@ +643,1 @@
>      NS_ENSURE_TRUE(parent, state);

no warning please

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +1015,5 @@
>  
>    if (IsDefunct())
>      return NS_OK;
>  
> +  CallQueryInterface(mParent->Parent(), aTable);

CallQueryInterface crashes on null

@@ +1170,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
>    // "table-cell-index" attribute
> +  nsAccessible* acc = mParent->Parent();

name it grandParent please

@@ +1171,5 @@
>      return NS_ERROR_FAILURE;
>  
>    // "table-cell-index" attribute
> +  nsAccessible* acc = mParent->Parent();
> +  NS_ENSURE_TRUE(acc, NS_ERROR_FAILURE);

no warning please

@@ +1172,5 @@
>  
>    // "table-cell-index" attribute
> +  nsAccessible* acc = mParent->Parent();
> +  NS_ENSURE_TRUE(acc, NS_ERROR_FAILURE);
> +  nsCOMPtr<nsIAccessibleTable> tableAccessible = do_QueryInterface(static_cast<nsIAccessible*>(acc));

if 80 chars restriction is broken then please put do_Query on second line
Attachment #547906 - Flags: review?(surkov.alexander) → review+
> >      // Calculate 'level' attribute based on number of parent listitems.
> >      level = 0;
> > +nsAccessible* parent = nsnull;
> 
> fix indentation please
> and maybe it's worth to keep existing code, that allows you do
> if (parent && level == 0)

hoestly I think I like this better, isn't it closer to the normal style of declaring variables where you use them?

> 
> @@ +3317,5 @@
> >        for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) {
> >          nsAccessible* sibling = parent->GetChildAt(siblingIdx);
> >  
> > +        nsAccessible* siblingChild =
> > +          sibling->GetChildAt(sibling->GetChildCount() - 1);
> 
> could you add LastChild and FirstChild to nsAccessible, sounds like nice to
> have

add both and leave first child unused, or just add last child for now?
(In reply to comment #4)
> > >      // Calculate 'level' attribute based on number of parent listitems.
> > >      level = 0;
> > > +nsAccessible* parent = nsnull;
> > 
> > fix indentation please
> > and maybe it's worth to keep existing code, that allows you do
> > if (parent && level == 0)
> 
> hoestly I think I like this better, isn't it closer to the normal style of
> declaring variables where you use them?

ok, maybe it's worth to check IsBoundToParent() in the beginning (after getting ARIA level). I just don't like that "parent = Parent(); if (!parent) return 0;"

> > could you add LastChild and FirstChild to nsAccessible, sounds like nice to
> > have
> 
> add both and leave first child unused, or just add last child for now?

add both, I think, we have many places where we use GetChildAt(0) so potentially we have lot of usecases, we can start to use as we go.
Attached patch patch (obsolete) — Splinter Review
Attachment #547906 - Attachment is obsolete: true
Comment on attachment 548162 [details] [diff] [review]
patch

Review of attachment 548162 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +826,5 @@
>  AtkObject *
>  getParentCB(AtkObject *aAtkObj)
>  {
> +  if (!aAtkObj->accessible_parent) {
> +    nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj);

nit: nsAccessibleWrap* accWrap

@@ +837,2 @@
>  
> +    AtkObject *parent = nsAccessibleWrap::GetAtkObject(accParent);

AtkObject* parent

::: accessible/src/base/nsAccessible.cpp
@@ +3307,3 @@
>      }
>  
> +    if (level == 0 && (parent = Parent())) {

this is sort of crazy style, why not though. But didn't you like to check parent after PRInt32 level = nsAccUtils::GetDefaultLevel(this); and get back to previous code? So all you do here (except renames) is just add

if (!IsBoundToParent())
  return;

@@ +3313,5 @@
>        for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) {
>          nsAccessible* sibling = parent->GetChildAt(siblingIdx);
>  
> +        nsAccessible* siblingChild =
> +          sibling->GetChildAt(sibling->GetChildCount() - 1);

so you decided to not introduce Last/FirstChild methods for this?

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +1175,5 @@
>  
>    // "table-cell-index" attribute
> +  nsAccessible* grandParent = mParent->Parent();
> +  if (!grandParent)
> +    return NS_ERROR_FAILURE;

NS_OK
Attachment #548162 - Flags: review+
> 
> ::: accessible/src/base/nsAccessible.cpp
> @@ +3307,3 @@
> >      }
> >  
> > +    if (level == 0 && (parent = Parent())) {
> 
> this is sort of crazy style, why not though. But didn't you like to check
> parent after PRInt32 level = nsAccUtils::GetDefaultLevel(this); and get back
> to previous code? So all you do here (except renames) is just add

I think I'd like to keep the change to
nsAccessible* parent = this;
while (parent = parent->Parent()) {
  parent->DoSomething();
}

but I wouldn't have any issues with just assigning to parent in the if and testing IsBoundToParent() at the beginning.


> 
> if (!IsBoundToParent())
>   return;
> 
> @@ +3313,5 @@
> >        for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) {
> >          nsAccessible* sibling = parent->GetChildAt(siblingIdx);
> >  
> > +        nsAccessible* siblingChild =
> > +          sibling->GetChildAt(sibling->GetChildCount() - 1);
> 
> so you decided to not introduce Last/FirstChild methods for this?

tbh I forgot,  I'll fix
>
(In reply to comment #8)

> I think I'd like to keep the change to
> nsAccessible* parent = this;
> while (parent = parent->Parent()) {
>   parent->DoSomething();
> }
> 
> but I wouldn't have any issues with just assigning to parent in the if and
> testing IsBoundToParent() at the beginning.

ok, up to you
Attached patch patch (obsolete) — Splinter Review
final cleanup (hopefully)
Comment on attachment 548192 [details] [diff] [review]
patch

Review of attachment 548192 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with fixed

::: accessible/src/base/nsAccessible.cpp
@@ +2142,5 @@
>          nsIView *view = frame->GetViewExternal();
>          if (view) {
>            nsIScrollableFrame *scrollFrame = do_QueryFrame(frame);
> +          if (scrollFrame || view->GetWidget() || !frame->GetParent())
> +            return nsRelUtils::AddTarget(aRelationType, aRelation, Parent());

I realize AddTarget has null check but is how does it come with your relation patch?

@@ +3277,5 @@
>      // its level.
>      level = 1;
>  
> +    nsAccessible* parent = this;
> +    while ((parent = Parent())) {

should fix it too

::: accessible/src/base/nsAccessible.h
@@ +306,5 @@
>     */
>    PRBool HasChildren() { return !!GetChildAt(0); }
>  
>    /**
> +   * Return first / last / next/previous sibling of the accessible.

nit: first/last/next, no spaces

@@ +312,5 @@
>    inline nsAccessible* NextSibling() const
>      {  return GetSiblingAtOffset(1); }
>    inline nsAccessible* PrevSibling() const
>      { return GetSiblingAtOffset(-1); }
> +  inline nsAccessible* FirstChild()

const

@@ +313,5 @@
>      {  return GetSiblingAtOffset(1); }
>    inline nsAccessible* PrevSibling() const
>      { return GetSiblingAtOffset(-1); }
> +  inline nsAccessible* FirstChild()
> +  { return GetChildCount() != 0 ? GetChildAt(0) : nsnull; }

two spaces indentation

@@ +314,5 @@
>    inline nsAccessible* PrevSibling() const
>      { return GetSiblingAtOffset(-1); }
> +  inline nsAccessible* FirstChild()
> +  { return GetChildCount() != 0 ? GetChildAt(0) : nsnull; }
> +  inline nsAccessible* LastChild()

const
Attachment #548192 - Flags: review+
never mind about const
Attached patch patch to landSplinter Review
Attachment #548162 - Attachment is obsolete: true
Attachment #548192 - Attachment is obsolete: true
Comment on attachment 548196 [details] [diff] [review]
patch to land

Review of attachment 548196 [details] [diff] [review]:
-----------------------------------------------------------------

few more nits, sorry :)

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +891,5 @@
>      if (!accWrap) {
>          return -1;
>      }
>  
> +    nsAccessible *parent = accWrap->Parent();

nit: nsAccessible* parent please

::: accessible/src/base/nsDocAccessible.cpp
@@ +1503,4 @@
>    if (!IsRoot()) {
>      nsRefPtr<AccEvent> reorderEvent =
> +      new AccEvent(nsIAccessibleEvent::EVENT_REORDER, parent, eAutoDetect,
> +                   AccEvent::eCoalesceFromSameSubtree);

why do you need local variable for parent since it's used one time only

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1530,5 @@
>  
> +  if (aRelationType == nsIAccessibleRelation::RELATION_LABEL_FOR) {
> +    nsAccessible* parent = Parent();
> +    if (parent)
> +      return nsRelUtils::AddTarget(aRelationType, aRelation, parent);

but here you do Parent() testing (you get rid it for node_child_of relation)

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +625,2 @@
>    while (descendantAcc) {
> +    nsAccessible *parentAcc = descendantAcc->Parent();

nit: nsAccessible* parentAcc

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +284,5 @@
>  
>  already_AddRefed<nsIAccessible>
>  nsAccessibleWrap::GetUnignoredParent()
>  {
> +  nsAccessibleWrap *parentWrap = static_cast<nsAccessibleWrap*>(Parent());

nit: nsAccessibleWrap* parentWrap

@@ +317,5 @@
>    while (parent) {
>      if (nsAccUtils::MustPrune(parent))
>        return PR_TRUE;
>  
> +    parent = parent->Parent();

I thought you prefer ;)
nsAccessible* parent = this;
while ((parent = parent->Parent())
> ::: accessible/src/base/nsDocAccessible.cpp
> @@ +1503,4 @@
> >    if (!IsRoot()) {
> >      nsRefPtr<AccEvent> reorderEvent =
> > +      new AccEvent(nsIAccessibleEvent::EVENT_REORDER, parent, eAutoDetect,
> > +                   AccEvent::eCoalesceFromSameSubtree);
> 
> why do you need local variable for parent since it's used one time only

I'd gues its an artifact from checking that it wasn't null, but I haven't checked that.

> 
> ::: accessible/src/html/nsHTMLTableAccessible.cpp
> @@ +1530,5 @@
> >  
> > +  if (aRelationType == nsIAccessibleRelation::RELATION_LABEL_FOR) {
> > +    nsAccessible* parent = Parent();
> > +    if (parent)
> > +      return nsRelUtils::AddTarget(aRelationType, aRelation, parent);
> 
> but here you do Parent() testing (you get rid it for node_child_of relation)

I realized it wasn't needed after writing this, I gues I can rm.

> 
> @@ +317,5 @@
> >    while (parent) {
> >      if (nsAccUtils::MustPrune(parent))
> >        return PR_TRUE;
> >  
> > +    parent = parent->Parent();
> 
> I thought you prefer ;)
> nsAccessible* parent = this;
> while ((parent = parent->Parent())

 there wasn't much reason to change it and leaving it is easier
http://hg.mozilla.org/mozilla-central/rev/79bba9b9beb5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(In reply to alexander surkov from comment #0)
> 1) while you're here rename GetParent() to Parent()
Strange, because I always thought that Gecko convention was that GetParent() could return null but Parent() wasn't allowed to.
(In reply to neil@parkwaycc.co.uk from comment #18)
> (In reply to alexander surkov from comment #0)
> > 1) while you're here rename GetParent() to Parent()
> Strange, because I always thought that Gecko convention was that GetParent()
> could return null but Parent() wasn't allowed to.

Occasionally I didn't know about this convention. We had a chat (iirc Trevor, Marco and me) where we agreed to remove 'Get' from getter methods. That was a reason I asked here to rename GetParent to Parent. I saw examples in Gecko where both cases are used and I guess I thought that 'Get' methods is sort of old style. 

While NodeInfo() and GetParent() of nsIContent follows this convention but it appears nsTArray::SafeElementAt doesn't if you pass nsnull as fallback. Is it sort of exception?
Assignee: nobody → trev.saunders
Please consider applying the patch to 7. I am experiencing segfaults because of this in 6 and 7rc5 and 7rc6. They disable me from using an unpatched build (it segfaults when I create a new tab). This is on Ubuntu 10.04, nothing special. This was not the case on 11.04. If you do not want to apply the whole patch (which is ~1k lines of renames and if's) then please at least fix it this way:

In accessible/src/html/nsHTMLFormControlAccessible.cpp:
in nsHTMLTextFieldAccessible::GetNameInternal(nsAString& aName):

change 

  if (mContent->GetBindingParent())
  {
    // XXX: bug 459640
    // There's a binding parent.
    // This means we're part of another control, so use parent accessible for name.
    // This ensures that a textbox inside of a XUL widget gets
    // an accessible name.
    nsAccessible* parent = GetParent();
    parent->GetName(aName);
  }


to:

  if (mContent->GetBindingParent())
  {
    // XXX: bug 459640
    // There's a binding parent.
    // This means we're part of another control, so use parent accessible for name.
    // This ensures that a textbox inside of a XUL widget gets
    // an accessible name.
    nsAccessible* parent = GetParent();
    if(parent)
      parent->GetName(aName);
  }


This is a one-line fix. Hopefully there will be no problem getting it in, even though understandably this comes late. I hope this goes through as it is a very disabling bug... and also (I hunted this down independently from Alexander and Trevor) my first fix for FF! :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: