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

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

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

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

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

(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...

add assert and check to avoid crashing.

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

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

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

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

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

(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

(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.

David, the comments seem reasonable?

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.

(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).

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:

don't cross document boundaries when climbing the tree

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

restore behaviour for not checking for link or onclick on document

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()) { }

deal with last nits

actually refresh patch this time

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

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