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

Status: RESOLVED FIXED

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

Description

[James Teh [:Jamie]]

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.

Comment 1

[Marco Zehe (:MarcoZ)]

This is definitely a blocker for E10S a11y rollout. I can confirm the problem.

Comment 2

[James Teh [:Jamie]]

When this occurs, attempting to marshal affected cells with the handler loaded (via accChild, accNavigate or AccessibleChildren) throws RPC_X_BAD_STUB_DATA.

Comment 3

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

NI Jim for awareness. See comment 1.

Comment 4

[James Teh [:Jamie]]

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?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 6

[James Teh [:Jamie]]

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

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

[Tracking Requested - why for this release]:
Regression in correctness of a11y functionality

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Fix

This fix depends on the patch in bug 1409545. It adds the IHandlerProvider::GetEffectiveOutParamIid method, which this patch then implements specifically for a11y.

Comment 9

[James Teh [:Jamie]]

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?

Comment 10

[James Teh [:Jamie]]

FYI, I tested this patch locally and it works nicely. Thank you.

Comment 11

[Julien Cristau [:jcristau]]

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

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

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

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a973213eefdccb8209d30ea978c677932db568d7
Bug 1406822: Implement a11y::HandlerProvider::GetEffectiveOutParamIid; r=Jamie

Comment 14

[alexander :surkov (:asurkov)]

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.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a973213eefdc

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Do we need this on Beta or can it ride the 58 train?

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

This needs to go on beta but I can't submit my request until bug 1409541 lands.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8919480 [details] [diff] [review]
Fix

Fixes a blocking issue for e10s+a11y users, beta57+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2683f54b6e51

Comment 21

[Marco Zehe (:MarcoZ)]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

No uplift was ever requested in that bug, so no.

Comment 23

[Ryan VanderMeulen [: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

Comment 24

[James Teh [:Jamie]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c19fd4561474

Comment 27

[Andrei Vaida [:avaida]]

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