Closed Bug 1365073 Opened 3 years ago Closed 3 years ago

Obsolete unique id generated by sdnAccessible::get_nodeInfo

Categories

(Core :: Disability Access APIs, enhancement)

Unspecified
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

Details

(Whiteboard: [aes+][JAWS])

Attachments

(1 file, 1 obsolete file)

If there is no accessible associated with the ISimpleDOMNode, we do the old negation method.

Not only is that method no longer used, it is also bad for pointers in the 3GB+ range.

While watching a11y event traces I have seen strange child IDs that should not be possible under the new scheme, so I suspect I was seeing an occurrence of this bug.
While this is not strictly an e10s bug, it impacts e10s JAWS testing enough such that it should block.
Whiteboard: [aes+][JAWS]
Summary: Obsolte unique id generated by sdnAccessible::get_nodeInfo → Obsolete unique id generated by sdnAccessible::get_nodeInfo
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8867933 - Flags: review?(tbsaunde+mozbugs)
Attachment #8867933 - Attachment is obsolete: true
Attachment #8867933 - Flags: review?(tbsaunde+mozbugs)
Attachment #8867934 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8867934 [details] [diff] [review]
Make sdnAccessible use the same child ID scheme as AccessibleWrap when it does not have an associated accessible (r2)

> AccessibleWrap::~AccessibleWrap()
> {
>   if (mID != kNoID) {
>-    sIDGen.ReleaseID(this);
>+    sIDGen.ReleaseID(WrapNotNull(this));

it would have been nice to split this api change to its own patch.

>+/* static */ void
>+AccessibleWrap::AssignChildIDTo(NotNull<sdnAccessible*> aSdnAcc)
>+{
>+  aSdnAcc->SetUniqueID(sIDGen.GetID());
>+}
>+
>+/* static */ void
>+AccessibleWrap::ReleaseChildID(NotNull<sdnAccessible*> aSdnAcc)
>+{
>+  sIDGen.ReleaseID(aSdnAcc);

honestly I'd rather just make sIDGen global that doesn't really seem like a layering violation its just a handy spot to dump it, so seems fine to expose raw access.

>   nsCOMPtr<nsINode> mNode;
>   RefPtr<AccessibleWrap> mWrap;
>+  Maybe<uint32_t> mUniqueId;

that's kind of inefficient since None isn't packed as the int being 0, but its cleaner and I don't really care about memory usage of this.
> };
> 
> } // namespace a11y
> } // namespace mozilla
> 
> #endif // mozilla_a11y_sdnAccessible_h_
Attachment #8867934 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9717295b3da5b3227e19cf90f9b4a6670a7e0292
Bug 1365073: Modify sdnAccessible to always generate unique IDs using AccessibleWrap's scheme even when it is not associated with an accessible; r=tbsaunde
https://hg.mozilla.org/mozilla-central/rev/9717295b3da5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Mark 54 won't fix as this is late for Beta54.
You need to log in before you can comment on or make changes to this bug.