Send content process MSAA ID to child in ActivateA11y

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks 1 bug)

Trunk
mozilla52
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

3 years ago
This is nicer and gets rid of some problems that could happen with the current implementation if it were to lazily request the content process MSAA Id while the MessageChannel is already waiting on a sync reply.
Assignee

Comment 1

3 years ago
Posted patch Part 1 - PContent changes (obsolete) — Splinter Review
Attachment #8801893 - Flags: review?(jmathies)
Assignee

Comment 2

3 years ago
Attachment #8801897 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8801897 [details] [diff] [review]
Part 2 - MsaaIdGenerator changes

it would really be better to merge this with the other patch since they are interdependent, or restructure them so later patches only depend on earlier ones so you don't break bisection.

otherwise this seems fine.
Attachment #8801897 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8801893 [details] [diff] [review]
Part 1 - PContent changes

since Jim hasn't delt with this yet I will (I generally take the position that I can review a11y stuff in other directories when reasonable) at least until somebody yells at me for it ;)

>+ContentChild::RecvActivateA11y(const uint32_t& aMsaaID)
> {
> #ifdef ACCESSIBILITY
>+#ifdef XP_WIN

you could assert here its non zero

>+#if defined(XP_WIN) && defined(ACCESSIBILITY)
>+  uint32_t mMsaaID;

it would be good to have some comments what this and the accessor are about.  I know, but I doubt random people looking at this file do.

>     /**
>      * Start accessibility engine in content process.
>+     * (aMsaaID only used on Windows)

be nice to comment what it is maybe.
Attachment #8801893 - Flags: review?(jmathies) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> Comment on attachment 8801893 [details] [diff] [review]
> Part 1 - PContent changes
> 
> since Jim hasn't delt with this yet I will (I generally take the position
> that I can review a11y stuff in other directories when reasonable) at least
> until somebody yells at me for it ;)

No problem here! :)
Assignee

Comment 6

3 years ago
Posted patch Merged patchSplinter Review
Merged patches and addressed review comments. Carrying forward r+.
Attachment #8801893 - Attachment is obsolete: true
Attachment #8801897 - Attachment is obsolete: true
Attachment #8803038 - Flags: review+
Assignee

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/069958e96c3de4800b9d00cb7370bd0c6482f538
Bug 1310833: Modify PContentParent::SendActivateA11y to accept the content process's MSAA ID as a parameter; r=tbsaunde
Because just one set of opaque failures in a notoriously opaque suite is never enough, you also had failures in talos-other-e10s on all three versions of Windows like https://treeherder.mozilla.org/logviewer.html#?job_id=38027192&repo=mozilla-inbound
Assignee

Comment 10

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #8)
> I had to back this out for Windows VM Marionette bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=38005118&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b6556299d0

This is busted due to the fact that marionette tries to instantiate a11y from a content script, without a11y ever having been instantiated in chrome.

I need to file a bug to address this, but what exactly gets filed depends on this question: Is instantiating from content a use case that we want to support? My guess is, "no," but over to Trevor to confirm.
Flags: needinfo?(aklotz) → needinfo?(tbsaunde+mozbugs)
Assignee

Comment 11

3 years ago
Discussed on IRC.
Flags: needinfo?(tbsaunde+mozbugs)
Assignee

Updated

3 years ago
Depends on: 1312816
Assignee

Comment 12

3 years ago
This restores the synchronous GetA11yContentId() function, but it is only used when instantiated from a content-based consumer. If content a11y is requested by chrome, it still uses the ID passed down by ActivateA11y().

I attached this as a separate patch so that you wouldn't need to interdiff, but I can also merge it into the first one if you prefer.
Attachment #8805226 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8805226 [details] [diff] [review]
Part 2 - Fix the "Instantiated from content XPCOM" case

ick at half supporting starting a11y in the content process, I worry about cases where that happens by accident and it actually matters, but I guess we can live for now.
Attachment #8805226 - Flags: review?(tbsaunde+mozbugs) → review+
Assignee

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d74a682eeaebecb0cf31625add0a92821fbb1c
Bug 1310833: Modify PContentParent::SendActivateA11y to accept the content process's MSAA ID as a parameter; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f15ff10cb267d4289655b2e3cf0a0dee48e7c0b
Bug 1310833: Make nsAccessibilityService synchronously query for its MSAA content process ID if the ID is not yet present; r=tbsaunde

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8d74a682eea
https://hg.mozilla.org/mozilla-central/rev/3f15ff10cb26
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

3 years ago
Depends on: 1314310
You need to log in before you can comment on or make changes to this bug.