Closed Bug 556004 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: surkov)

References

()

Details

(Keywords: assertion)

Attachments

(1 file, 1 obsolete file)

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.
(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.
Component: Places → Disability Access APIs
Product: Toolkit → Core
QA Contact: places → accessibility-apis
(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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch quick attempt (obsolete) (deleted) — Splinter Review
Just tried this really quick... we get one failure in states/test_link.html. Need to look into this (IntrinsicState) a bit more (later).
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.
(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.
> 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.
Or document that you have to call IsLink() first.
(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.
(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.
Blocks: 571613
Attached patch patch (deleted) — Splinter Review
Use IntristicState() as it was suggested and was in previous patch, enable actions/test_link that was never enabled by accident.
Assignee: nobody → surkov.alexander
Attachment #436173 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #484619 - Flags: review?(marco.zehe)
Attachment #484619 - Flags: review?(bolterbugz)
Attachment #484619 - Flags: review?(marco.zehe) → review+
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)
Attachment #484619 - Flags: review?(bolterbugz) → review+
(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 :)
Attachment #484619 - Flags: approval2.0?
Comment on attachment 484619 [details] [diff] [review] patch Approved. Let's get a clean try run first.
Attachment #484619 - Flags: approval2.0? → approval2.0+
(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/surkov.alexander@gmail.com-bd4dcfc7f446 I'll wait for linux results since I tested on windows locally.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: