653584 - crash in nsLinkableAccessible::NativeState()

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110428 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110428 Firefox/6.0a1

https://crash-stats.mozilla.com/report/index/bp-8513f64d-8814-4359-b55b-59de12110428

I don't have much time for this till the weekend, but it looks like actionAccessible is somehow null.  I'm not sure if that's related to bug 634218 or not yet.

Reproducible: Always

Comment 1

[Trevor Saunders (:tbsaunde)]

that link was bad it should be https://crash-stats.mozilla.com/report/index/bp-8513f64d-8814-4359-b55b-59de12110428

Comment 2

[Trevor Saunders (:tbsaunde)]

hm so copying from the  url bar just doesn't work,  hopefully this does its crash report 8513f64d-8814-4359-b55b-59de12110428

and p.s. I see shift still hates me

Comment 3

[Trevor Saunders (:tbsaunde)]

well, that was better but if bugzilla would auto link this it would be nice its crash report bp-8513f64d-8814-4359-b55b-59de12110428

Comment 4

[alexander :surkov (:asurkov)]

Trevor, take an approach from bug 649409: assertion and null check.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to comment #4)
> Trevor, take an approach from bug 649409: assertion and null check.

sure, I've thought aboutthat but I'd like to know why actionAcc was null, since if we're asserting it shouldn't be then it would seem something else is wrong...

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: patch

add assert and check to avoid crashing.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 529628 [details] [diff] [review]
patch

Review of attachment 529628 [details] [diff] [review]:

::: accessible/src/base/nsBaseWidgetAccessible.cpp
@@ +120,5 @@
   PRUint64 states = nsAccessibleWrap::NativeState();
   if (mIsLink) {
     states |= states::LINKED;
     nsAccessible* actionAcc = GetActionAccessible();
+    NS_ASSERTION(actionAcc, "no action accessible");

please give some explanation in message why it can't be null

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: fix crash in nsLinkableAccessible::NativeState()

from irc discussion store a pointer to the action accessible not the action content.

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: irc discussion

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 531865 [details] [diff] [review]
fix crash in nsLinkableAccessible::NativeState()

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

I think I'd like to look at next patch

::: accessible/src/base/nsBaseWidgetAccessible.cpp
@@ +131,5 @@
>    nsAccessible::GetValue(aValue);
>    if (!aValue.IsEmpty())
>      return NS_OK;
>  
> +  return mActionAcc ? mActionAcc->GetValue(aValue) : NS_ERROR_NOT_IMPLEMENTED;

you got rid mIsLink check

@@ +206,3 @@
>  
> +      if (isLink)
> +        return mActionAcc->GetAnchorURI(aAnchorIndex);

fix identation here, use IsHyperLink() instead of local variable, it's inline and assertion is for debug only

@@ +225,5 @@
>    mIsLink = PR_FALSE;
>    mIsOnclick = PR_FALSE;
>  
> +  nsAccessible* walkUpAcc = GetAccService()->GetAccessibleInWeakShell(mContent,
> +                                                                      mWeakShell);

this is 'this'.

@@ +232,5 @@
>      mIsOnclick = PR_TRUE;
>      return;
>    }
>  
> +  while ((walkUpAcc = walkUpAcc->GetParent())) {

actually you don't need walkUpAcc before this point

::: accessible/src/base/nsBaseWidgetAccessible.h
@@ +107,5 @@
>  protected:
>    // nsAccessible
>    virtual void BindToParent(nsAccessible* aParent, PRUint32 aIndexInParent);
>  
> +  nsAccessible* mActionAcc;

perhaps a small comment what action accessible is

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: bug 653584 - fix crash in nsLinkableAccessible::NativeState()

addressed comments, I'm not sure of the comment describing mActionAcc thoughts?

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 531901 [details] [diff] [review]
bug 653584 - fix crash in nsLinkableAccessible::NativeState()

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

::: accessible/src/base/nsBaseWidgetAccessible.cpp
@@ +189,5 @@
>  
>  void
>  nsLinkableAccessible::Shutdown()
>  {
> +  mActionAcc = nsnull;

it makes sense to do mIsLink = PR_FALSE; and mIsOnClick = PR_FALSE; just to be on safe side

@@ +206,4 @@
>                     "nsIAccessibleHyperLink isn't implemented.");
>  
> +      if (mActionAcc->IsHyperLink())
> +        return mActionAcc->GetAnchorURI(aAnchorIndex);

fix indentation please, this whole bock has wrong identation

@@ +241,4 @@
>        return;
>      }
>  
> +    if (nsCoreUtils::HasClickListener(walkUpAcc->GetContent())) {

actually that's not the same, when you crawl the parent chain, then you check the click listener on document, now you do that on body element.

::: accessible/src/base/nsBaseWidgetAccessible.h
@@ +111,2 @@
>    /**
> +   * accessible that is actually actionable

perhaps, parent accessible that provides an action for this linkable accessible

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to comment #12)
> Comment on attachment 531901 [details] [diff] [review] [review]
> bug 653584 - fix crash in nsLinkableAccessible::NativeState()
> 
> Review of attachment 531901 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsBaseWidgetAccessible.cpp
> @@ +189,5 @@
> >  
> >  void
> >  nsLinkableAccessible::Shutdown()
> >  {
> > +  mActionAcc = nsnull;
> 
> it makes sense to do mIsLink = PR_FALSE; and mIsOnClick = PR_FALSE; just to
> be on safe side

sure

> @@ +241,4 @@
> >        return;
> >      }
> >  
> > +    if (nsCoreUtils::HasClickListener(walkUpAcc->GetContent())) {
> 
> actually that's not the same, when you crawl the parent chain, then you
> check the click listener on document, now you do that on body element.

hmm, I'm not sure I understand this it looks to me like we used to look at everything in the content parent change to see if it had a click listener, and now we look at the content for each accessible in the accessible parent chain to see if it has a listener.  are these two sets of content elements not necessarily the same? 

> ::: accessible/src/base/nsBaseWidgetAccessible.h
> @@ +111,2 @@
> >    /**
> > +   * accessible that is actually actionable
> 
> perhaps, parent accessible that provides an action for this linkable

sure
> accessible

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> > > +    if (nsCoreUtils::HasClickListener(walkUpAcc->GetContent())) {
> > 
> > actually that's not the same, when you crawl the parent chain, then you
> > check the click listener on document, now you do that on body element.
> 
> hmm, I'm not sure I understand this it looks to me like we used to look at
> everything in the content parent change to see if it had a click listener,
> and now we look at the content for each accessible in the accessible parent
> chain to see if it has a listener.  are these two sets of content elements
> not necessarily the same? 

we did: iterate through DOM, check click listener (in particular you did a check on document node), now you iterate through accessibles and check click listener on content node (in particular you check on body element but don't do this on document node). Actually in many cases that is great (examples like <body onclick="bla();">) but that's not equivalent to what we had. In general all logic here is somehow broken (for example, the case of click listener on non accessible element in parent chain). Maybe it's worth to keep what you suggest but you should add a comment so we can find something better later.

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: patch to land

David, the comments seem reasonable?

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 533752 [details] [diff] [review]
patch to land

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

::: accessible/src/base/nsBaseWidgetAccessible.cpp
@@ +190,5 @@
>  void
>  nsLinkableAccessible::Shutdown()
>  {
> +	mIsLink = PR_FALSE;
> +	mIsOnclick = PR_FALSE;

fix identation

@@ +234,5 @@
>    }
>  
> +  // XXX this doesn't cover all content that is the parent of this accessibles
> +  // content, for example it doesn't cover the document, but does get the body
> +  // element

maybe something like this:

// XXX: The logic looks broken since the click listener may be registered on non accessible node in parent chain but this node is skipped when tree is traversed. On the another hand, click listener associated with document accessible can be registered on body element or document node, we don't check them both.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> maybe something like this:
> 
> // XXX: The logic looks broken since the click listener may be registered on
> non accessible node in parent chain but this node is skipped when tree is
> traversed. On the another hand, click listener associated with document
> accessible can be registered on body element or document node, we don't
> check them both.

the last part is not valid since we never checked the document node itself (we have nsIContent restriction for the check).

Comment 18

[Trevor Saunders (:tbsaunde)]

Attachment: patch

fix the tests to pass, it turns out that the test harness has a xul:browser that has a onclick handler registered.  The tests can probably be fixed in a better way, but I'm not sure what it would be.

Review:

Comment 19

[Trevor Saunders (:tbsaunde)]

Attachment: patch

don't cross document boundaries when climbing the tree

Comment 20

[Trevor Saunders (:tbsaunde)]

Attachment: patch

fix silliness we should do walkUpAcc->IsDoc() not IsDoc().  The tests now pass without changes

Comment 21

[Trevor Saunders (:tbsaunde)]

Attachment: patch

restore behaviour for not checking for link or onclick on document

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 534176 [details] [diff] [review]
patch

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

::: accessible/src/base/nsBaseWidgetAccessible.cpp
@@ +232,5 @@
>      mIsOnclick = PR_TRUE;
>      return;
>    }
>  
> +  // // XXX: The logic looks broken since the click listener may be registered

double // //

@@ +243,1 @@
>  

again IsDocument(), not sure how it works

I prefer
while ((walkUpAcc = walkUpAcc->GetParent()) && !walkUpAcc->IsDoc()) {
}

Comment 23

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 24

[Trevor Saunders (:tbsaunde)]

Attachment: patch to land

deal with last nits

Comment 25

[Trevor Saunders (:tbsaunde)]

Attachment: right patch

actually refresh patch this time

Comment 26

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/c30f7926511b

Comment 28

[AndreiD[QA]]

Build: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0
The changes made in m-c and landed (comment 26) are visible in hg, under beta (i.e. http://hg.mozilla.org/releases/mozilla-beta/file/f3e82fad65b2/accessible/src/base/nsBaseWidgetAccessible.cpp).
Setting this as verified for Firefox 6 Beta