Closed
Bug 1406822
Opened 7 years ago
Closed 7 years ago
Fetching table cell accessibles by coords causes table breakage with handler enabled
Categories
(Core :: Disability Access APIs, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | blocking | fixed |
firefox58 | --- | fixed |
People
(Reporter: Jamie, Assigned: bugzilla)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: aes+)
Attachments
(1 file)
3.72 KB,
patch
|
Jamie
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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+
Reporter | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
NI Jim for awareness. See comment 1.
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
status-firefox58:
--- → affected
Flags: needinfo?(jmathies)
![]() |
||
Updated•7 years ago
|
Flags: needinfo?(jmathies)
Reporter | ||
Comment 4•7 years ago
|
||
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?
Assignee | ||
Comment 5•7 years ago
|
||
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.
Reporter | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•7 years ago
|
||
[Tracking Requested - why for this release]:
Regression in correctness of a11y functionality
tracking-firefox57:
--- → ?
Keywords: regression
Assignee | ||
Comment 8•7 years ago
|
||
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)
Reporter | ||
Comment 9•7 years ago
|
||
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+
Reporter | ||
Comment 10•7 years ago
|
||
FYI, I tested this patch locally and it works nicely. Thank you.
Comment 11•7 years ago
|
||
(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.
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Assignee | ||
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a973213eefdccb8209d30ea978c677932db568d7
Bug 1406822: Implement a11y::HandlerProvider::GetEffectiveOutParamIid; r=Jamie
Comment 14•7 years ago
|
||
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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 16•7 years ago
|
||
Do we need this on Beta or can it ride the 58 train?
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(aklotz)
Assignee | ||
Comment 17•7 years ago
|
||
This needs to go on beta but I can't submit my request until bug 1409541 lands.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 18•7 years ago
|
||
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+
Comment hidden (obsolete) |
Comment 21•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
backout |
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
Reporter | ||
Comment 24•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
bugherder uplift |
Comment 27•7 years ago
|
||
(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.
Description
•