Closed Bug 1406822 Opened 3 years ago Closed 3 years ago

Fetching table cell accessibles by coords causes table breakage with handler enabled

Categories

(Core :: Disability Access APIs, defect, P1)

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 blocking fixed
firefox58 --- fixed

People

(Reporter: Jamie, Assigned: aklotz)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: aes+)

Attachments

(1 file)

STR:
1. Start Firefox and NVDA.
2. Open this test case:
data:text/html,<table><tr><th>a1</th><td>b1</t1></tr><tr><td>a2</td><td>b2</td></tr><tr><td>a3</td><td>b3</td></tr></table>
3. Ensure the document is focused.
4. Press control+home to move to the top of the document.
5. Move to cell b1 by pressing control+alt+rightArrow.
6. Press NVDA+f5 to refresh browse mode.
7. Press control+home to move to the top of the document.
Expected: NVDA should say "a1"
Actual: NVDA says "b2"

When you use table navigation commands in NVDA, this uses IAccessibleTable::accessibleAt to get the cell at that position. When you do this on a cell, the entire row can no longer be rendered in NVDA browse mode. If you navigate through all cells of the table using table navigation, eventually, the entire table will disappear.

This only occurs if the handler is enabled. If you disable the handler (or access the table from out-of-process where the handler currently isn't used), everything works as expected.

I haven't figured out exactly what fails yet. This is a bit tricky because I have to debug it in-process.

Other observations:
1. Once this occurs, calling IAccessible::accChild out-of-process on the root accessible fails when passed the id of affected table cells. This isn't how NVDA's buffer retrieves table cells, so this isn't directly relevant, but it's very curious nevertheless. This too does not occur with the handler disabled.
2. This doesn't seem to occur if a reference is already being held to the target cell. For example, if you're holding a reference to an accessible for cell b1 and you use IAccessibleTable::accessibleAt to retrieve cell b1, it doesn't break.
3. I have a hunch this is somehow related to ref counting, but I have no solid reasoning yet.

Impact: This seriously breaks tables when table navigation is used in NVDA. It effectively makes tables unusable for users that rely on table navigation commands.
This is definitely a blocker for E10S a11y rollout. I can confirm the problem.
Blocks: 1258839
Has STR: --- → yes
OS: Unspecified → Windows
Priority: -- → P1
Hardware: Unspecified → All
Whiteboard: aes+
When this occurs, attempting to marshal affected cells with the handler loaded (via accChild, accNavigate or AccessibleChildren) throws RPC_X_BAD_STUB_DATA.
NI Jim for awareness. See comment 1.
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
IAccessibleTable::accessibleAt returns an IUnknown, not an IAccessible or subclass thereof. When the client gets this IUnknown, it is not using the handler, since HandlerProvider::GetHandler doesn't answer to IUnknown. (That is slightly concerning in itself, since it means the handler won't get used for table cells retrieved in this way, but NVDA at least doesn't do this in-proc, so let's leave that aside for now.)

What I can't work out is why that should affect future instances of the object. For a start, this occurs even if the client releases the table cell it retrieved. Perhaps COM is trying to use the same stub for the object or something? But isn't the interceptor a different object according to COM?
This sounds to me like we need to investigate the code in the interceptor that wraps outparams, and make sure it is correctly handling this.
Aaron and I just discussed this. The plan is to make sure that the IUnknown outparam from IAccessibleTable::accessibleAt and IAccessibleTable2::cellAt gets wrapped by the handler. There are quite a few other methods in IA2 which need similar treatment. We have some ideas about why COM barfs here, but at the end of the day, these really need to be wrapped by the handler anyway, so we'll just do that rather than diagnosing the marshaling failure.

Assigning to Aaron, since I don't think I can handle the implementation of this just yet. :)
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Depends on: 1409545
[Tracking Requested - why for this release]:
Regression in correctness of a11y functionality
Keywords: regression
Attached patch FixSplinter Review
This fix depends on the patch in bug 1409545. It adds the IHandlerProvider::GetEffectiveOutParamIid method, which this patch then implements specifically for a11y.
Attachment #8919480 - Flags: review?(jteh)
Comment on attachment 8919480 [details] [diff] [review]
Fix

Review of attachment 8919480 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Ironically, this bug just kept biting me while i was reviewing this patch... :)

::: accessible/ipc/win/HandlerProvider.cpp
@@ +219,5 @@
> +REFIID
> +HandlerProvider::GetEffectiveOutParamIid(REFIID aCallIid,
> +                                         ULONG aCallMethod)
> +{
> +  if (aCallIid == IID_IAccessibleTable || aCallIid == IID_IAccessibleTable2) {

I'm not sure whether you want to do this or in a future patch, but at some point, this should be extended to cover some other interfaces/methods:
IAccessible2_2::accessibleWithCaret
IAccessibleDocument::anchorTarget
IAccessibleRelation::target (singular; targets is covered by the array data stuff)
IAccessibleTableCell::table
All of these could be treated as IAccessible2_3, although someone might call IAccessibleTableCell::table and then query to IAccessibleTable2 instead. Still, that can be dealt with separately.
IAccessible2_3::selectionRanges also has two IUnknown* within its IA2Range struct out param. Does ICallFrame::WalkFrame handle interface pointers in structs already?
Attachment #8919480 - Flags: review?(jteh) → review+
FYI, I tested this patch locally and it works nicely. Thank you.
(In reply to Marco Zehe (:MarcoZ) from comment #1)
> This is definitely a blocker for E10S a11y rollout. I can confirm the
> problem.

Marking as blocking.
(In reply to James Teh [:Jamie] from comment #9)
> I'm not sure whether you want to do this or in a future patch, but at some
> point, this should be extended to cover some other interfaces/methods:
> IAccessible2_2::accessibleWithCaret
> IAccessibleDocument::anchorTarget
> IAccessibleRelation::target (singular; targets is covered by the array data
> stuff)
> IAccessibleTableCell::table
> All of these could be treated as IAccessible2_3, although someone might call
> IAccessibleTableCell::table and then query to IAccessibleTable2 instead.
> Still, that can be dealt with separately.
> IAccessible2_3::selectionRanges also has two IUnknown* within its IA2Range
> struct out param. Does ICallFrame::WalkFrame handle interface pointers in
> structs already?

We'll take care of those in a follow-up bug.
Comment on attachment 8919480 [details] [diff] [review]
Fix

Review of attachment 8919480 [details] [diff] [review]:
-----------------------------------------------------------------

Slowly win a11y becomes a jungle with me :) I have a favor to ask of you: if you could comment code a bit more, then it'd be easier for understanding. It's easy to get lost looking at the new method trying to figure out why it handles those interfaces.
https://hg.mozilla.org/mozilla-central/rev/a973213eefdc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Do we need this on Beta or can it ride the 58 train?
Flags: needinfo?(aklotz)
This needs to go on beta but I can't submit my request until bug 1409541 lands.
Flags: needinfo?(aklotz)
Comment on attachment 8919480 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/Bug causing the regression]: a11y+e10s
[User impact if declined]: a11y users will have problems resolving table cells
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: depends on bug 1409545 and bug 1409541
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Simple patches, already verified
[String changes made/needed]: None
Attachment #8919480 - Flags: approval-mozilla-beta?
Comment on attachment 8919480 [details] [diff] [review]
Fix

Fixes a blocking issue for e10s+a11y users, beta57+
Attachment #8919480 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Ryan, are you taking along bug 1409545 as well, this one depends on the functionality added there, as Aaron specified above in the approval request.
Flags: needinfo?(ryanvm)
No uplift was ever requested in that bug, so no.
Flags: needinfo?(ryanvm)
And shockingly, backed out for bustage. In the future, approval requests for *all* required bugs being made at the same time would be appreciated to avoid wasted time and CPU cycles.

https://hg.mozilla.org/releases/mozilla-beta/rev/cae5754b8af50a9b81e92b261365d6a6d4cbffb2
(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> And shockingly, backed out for bustage. In the future, approval requests for
> *all* required bugs being made at the same time would be appreciated to
> avoid wasted time and CPU cycles.

So, as a newcomer, I'm kinda confused about this. The approval comment template asks about "[List of other uplifts needed for the feature/fix]". This leads me to think a corresponding uplift request doesn't need to be made in the dependent bugs. So, we *do* need to make an approval request in both? Should both approval comments refer to the other bug as dependent? And if they do, how will the uplifter know which order to land them in? This is not documented anywhere I can see; the uplit rules make no mention of it:
https://wiki.mozilla.org/Release_Management/Uplift_rules
It'd be good if this could be documented clearly, as otherwise, it's rather tricky to know what the requirements are.
(In reply to James Teh [:Jamie] from comment #24)

The intent of the questions is to inform the risk assessment Release Management does when deciding whether to accept the uplift or not (i.e. a small patch from one bug might actually be very high-risk because it depends on a much larger patch or series of patches from another). Each bug still requires its own approval before it can land (we have a lot of infrastructure that depends on it). And yes, we probably should have noticed that bug 1409545 was missing such a request sooner than we did - I blame lack of coffee :). Normally that is something we would notice and complain about.

As far as landing order is concerned, we just go off the order they landed on m-c unless told otherwise. You should also be aware that uplifts are done by grafting the commit that landed on m-c unless there's a clearly-labeled branch patch attached to the bug (since that question often comes up too). Hope that helps, and don't hesitate to shoot me an email or Slack/IRC ping if you have other questions!
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #18)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:  no

Setting qe-verify- based on Aaron's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.