Last Comment Bug 653584 - crash in nsLinkableAccessible::NativeState()
: crash in nsLinkableAccessible::NativeState()
Status: VERIFIED FIXED
: common-issue+
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla6
Assigned To: Trevor Saunders (:tbsaunde)
:
:
Mentors:
: 649409 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-28 15:38 PDT by Trevor Saunders (:tbsaunde)
Modified: 2011-07-27 08:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (806 bytes, patch)
2011-05-02 19:27 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
fix crash in nsLinkableAccessible::NativeState() (7.65 KB, patch)
2011-05-12 01:38 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
irc discussion (3.16 KB, text/plain)
2011-05-12 01:44 PDT, Trevor Saunders (:tbsaunde)
no flags Details
bug 653584 - fix crash in nsLinkableAccessible::NativeState() (7.55 KB, patch)
2011-05-12 05:44 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
patch to land (7.81 KB, patch)
2011-05-19 12:25 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (10.13 KB, patch)
2011-05-20 15:45 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (7.77 KB, patch)
2011-05-20 17:39 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (7.78 KB, patch)
2011-05-20 17:59 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (7.87 KB, patch)
2011-05-20 18:03 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
patch (7.84 KB, patch)
2011-05-20 19:22 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch to land (7.84 KB, patch)
2011-05-20 20:49 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
right patch (7.84 KB, patch)
2011-05-20 20:55 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2011-04-28 15:38:59 PDT
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) 2011-04-28 15:45:36 PDT
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) 2011-04-28 15:51:40 PDT
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) 2011-04-28 15:58:37 PDT
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 2011-04-28 19:13:13 PDT
Trevor, take an approach from bug 649409: assertion and null check.
Comment 5 Trevor Saunders (:tbsaunde) 2011-04-29 03:01:23 PDT
(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) 2011-05-02 19:27:46 PDT
Created attachment 529628 [details] [diff] [review]
patch

add assert and check to avoid crashing.
Comment 7 alexander :surkov 2011-05-02 23:13:49 PDT
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) 2011-05-12 01:38:39 PDT
Created attachment 531865 [details] [diff] [review]
fix crash in nsLinkableAccessible::NativeState()

from irc discussion store a pointer to the action accessible not the action content.
Comment 9 Trevor Saunders (:tbsaunde) 2011-05-12 01:44:38 PDT
Created attachment 531866 [details]
irc discussion
Comment 10 alexander :surkov 2011-05-12 04:22:14 PDT
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) 2011-05-12 05:44:12 PDT
Created attachment 531901 [details] [diff] [review]
bug 653584 - fix crash in nsLinkableAccessible::NativeState()

addressed comments, I'm not sure of the comment describing mActionAcc thoughts?
Comment 12 alexander :surkov 2011-05-12 07:27:54 PDT
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) 2011-05-13 06:15:12 PDT
(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 2011-05-18 21:01:10 PDT
(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) 2011-05-19 12:25:51 PDT
Created attachment 533752 [details] [diff] [review]
patch to land

David, the comments seem reasonable?
Comment 16 alexander :surkov 2011-05-19 21:17:25 PDT
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 2011-05-19 21:30:31 PDT
(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) 2011-05-20 15:45:49 PDT
Created attachment 534136 [details] [diff] [review]
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) 2011-05-20 17:39:55 PDT
Created attachment 534168 [details] [diff] [review]
patch

don't cross document boundaries when climbing the tree
Comment 20 Trevor Saunders (:tbsaunde) 2011-05-20 17:59:13 PDT
Created attachment 534174 [details] [diff] [review]
patch

fix silliness we should do walkUpAcc->IsDoc() not IsDoc().  The tests now pass without changes
Comment 21 Trevor Saunders (:tbsaunde) 2011-05-20 18:03:24 PDT
Created attachment 534176 [details] [diff] [review]
patch

restore behaviour for not checking for link or onclick on document
Comment 22 alexander :surkov 2011-05-20 18:31:28 PDT
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) 2011-05-20 19:22:00 PDT
Created attachment 534186 [details] [diff] [review]
patch
Comment 24 Trevor Saunders (:tbsaunde) 2011-05-20 20:49:47 PDT
Created attachment 534190 [details] [diff] [review]
patch to land

deal with last nits
Comment 25 Trevor Saunders (:tbsaunde) 2011-05-20 20:55:26 PDT
Created attachment 534191 [details] [diff] [review]
right patch

actually refresh patch this time
Comment 26 alexander :surkov 2011-05-21 00:13:09 PDT
landed - http://hg.mozilla.org/mozilla-central/rev/c30f7926511b
Comment 27 alexander :surkov 2011-05-24 02:30:22 PDT
*** Bug 649409 has been marked as a duplicate of this bug. ***
Comment 28 AndreiD[QA] 2011-07-27 08:18:06 PDT
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

Note You need to log in before you can comment on or make changes to this bug.