556004 - ###!!! ASSERTION: Getting the link state of an unregistered Link!: 'mRegistered'

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Browse to any reasonable web page
2. Inspect the page in DOM Inspector, and show accessible nodes
3. Tab through all the links

On Google UK I always get it on the Shopping and Sign In links.

Comment 1

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> Steps to reproduce problem:
> 1. Browse to any reasonable web page
> 2. Inspect the page in DOM Inspector, and show accessible nodes
> 3. Tab through all the links
Does this require accessible nodes being on?  My bet is that some a11y code is calling Link::GetLinkState when it really wants to be calling IntrinsicState and checking the link bits.

Comment 2

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

(In reply to comment #1)
> (In reply to comment #0)
> > Steps to reproduce problem:
> > 1. Browse to any reasonable web page
> > 2. Inspect the page in DOM Inspector, and show accessible nodes
> > 3. Tab through all the links
> Does this require accessible nodes being on?

Yeah.

> My bet is that some a11y code is
> calling Link::GetLinkState when it really wants to be calling IntrinsicState
> and checking the link bits.

We do call GetLinkState... is that naughty?

Comment 3

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

Attachment: quick attempt

Just tried this really quick... we get one failure in states/test_link.html. Need to look into this (IntrinsicState) a bit more (later).

Comment 5

[alexander :surkov (:asurkov)]

Description from dupe bug 560113:

The assertion appears whenever nsIContent::GetLinkState() is called on link
that has 'eLinkState_NotLink' state
(http://mxr.mozilla.org/mozilla-central/source/content/base/src/Link.cpp#72).
It happens because mRegistered member is false when the link is not a link
(http://mxr.mozilla.org/mozilla-central/source/content/base/src/Link.cpp#122).

We get lot of assertion when accessibility is turned on. Stack trace is

>	xul.dll!mozilla::dom::Link::GetLinkState()  Line 72 + 0x23 bytes	C++
     xul.dll!nsHTMLAnchorElement::GetLinkState()  Line 423    C++
     xul.dll!nsHTMLLinkAccessible::GetStateInternal(unsigned int *
aState=0x001abbc4, unsigned int * aExtraState=0x00000000)  Line 85 + 0x1b bytes
   C++
     xul.dll!nsAccessible::GetState(unsigned int * aState=0x001abbc4, unsigned
int * aExtraState=0x00000000)  Line 1677 + 0x1c bytes    C++
     xul.dll!nsAccessibleWrap::get_accState(tagVARIANT varChild=I4 = 0,
tagVARIANT * pvarState=I4 = 0)  Line 544 + 0x1f bytes    C++

I suppose the fix could be

NS_ASSERTION(mRegistered || mLinkState != eLinkState_NotLink,
               "Getting the link state of an unregistered Link!");

However a11y expects eLinkState_Unknown as possible value to be returned.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Created an attachment (id=436173) [details]
> quick attempt
> 
> Just tried this really quick... we get one failure in states/test_link.html.
> Need to look into this (IntrinsicState) a bit more (later).

I believe it's not a11y fault and it should be fixed in link.cpp.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I believe it's not a11y fault and it should be fixed in link.cpp.

Yeah, indeed.  Unless we remove the nsIContent GetLinkState api, of course.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Or document that you have to call IsLink() first.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Or document that you have to call IsLink() first.

Either way works I think. However documentation approach doesn't sound very friendly since I would say method shouldn't assert just because it's not supposed to be called on this object.

Btw, nsIContent::IsLink() is fine but it confuses me since it has required nsIURI out argument, a11y doesn't need it and therefore it makes me think it requires unnecessary computations/constructions of uri.

So I think it's ok to call IsLink() in a11y before we call GetLinkState() but I really would like to see out argument optional if that's possible.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Yeah, indeed.  Unless we remove the nsIContent GetLinkState api, of course.
Wonder why we never did that.  We should probably do that...

(In reply to comment #9)
> Btw, nsIContent::IsLink() is fine but it confuses me since it has required
> nsIURI out argument, a11y doesn't need it and therefore it makes me think it
> requires unnecessary computations/constructions of uri.
Except that everything figures out if it is a link by generating the URI, so it's no additional work really.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch

Use IntristicState() as it was suggested and was in previous patch, enable actions/test_link that was never enabled by accident.

Comment 12

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

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

r=me, glad you got to this one. So calling IntrinsicState() doesn't cause the assertion? (The patch is good clean up either way)

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> Comment on attachment 484619 [details] [diff] [review]
> patch
> 
> r=me, glad you got to this one. 

I've tired to see these in console, too much when you debug real pages.

>So calling IntrinsicState() doesn't cause the
> assertion? (The patch is good clean up either way)

no, it doesn't use GetLinkState, and I don't recall them in console :)

Comment 14

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

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

Approved. Let's get a clean try run first.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> Comment on attachment 484619 [details] [diff] [review]
> patch
> 
> Approved. Let's get a clean try run first.

I started try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-bd4dcfc7f446

I'll wait for linux results since I tested on windows locally.

Comment 16

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/b003e775737b