1342245 - accessibility.force_disabled=-1 (force enabled) should work on Windows

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jim Mathies [:jimm]]

This pref is currently ignored on Windows. I've put together a little force enable hack to see if I can repo some of these top crashes on try. The results looks promising.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72304bd7e1eeb9fa7e1255833805b8ada41dec3a&selectedJob=79755778

Comment 1

[Jim Mathies [:jimm]]

Attachment: tmp.txt

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8840892 [details] [diff] [review]
patch

The post message bounce works around a lack of initialization for some objects checked in the GetAccessible code.

Comment 4

[Jim Mathies [:jimm]]

Attachment: debugging work

Comment 5

[Jim Mathies [:jimm]]

Attachment: flip pref

Comment 6

[David Parks [:handyman]]

Attachment: WIP Patches that improve test success with accessibility on

These very messy patches go a long way to getting mochitests running with accessibility.  They are a mixture of jimms patches in this bug, tbsaunde's work on bug 1340579 and my own fixes.  A lot of crashes have been fixed.  They include:

* Delaying sending events from child proc when the parent doc hasn't received "show" on the parent proc.  Show creates the accessible tree and IDs that otherwise cause asserts/crashes.  We could also just dump these events -- I dont know what that would do -- but right now we just waste time sending and then ignoring them.
* Calling RemoveChildDoc when a doc with a parent is shutdown instead of where we do now, which is too late.  RemoveChildDoc needs the mAccessible list to still contain the child doc.  It doesnt with the priorcode -- RemoveAccessible happens first.
* ConstructChildDocInParentProcess needs to be delayed if the parent doc hasn't been constructed in the parent proc!

Comment 7

[Jim Mathies [:jimm]]

(In reply to David Parks (dparks) [:handyman] from comment #6)
> Created attachment 8843178 [details]
> WIP Patches that improve test success with accessibility on
> 
> These very messy patches go a long way to getting mochitests running with
> accessibility.  They are a mixture of jimms patches in this bug, tbsaunde's
> work on bug 1340579 and my own fixes.  A lot of crashes have been fixed. 
> They include:
> 
> * Delaying sending events from child proc when the parent doc hasn't
> received "show" on the parent proc.  Show creates the accessible tree and
> IDs that otherwise cause asserts/crashes.  We could also just dump these
> events -- I dont know what that would do -- but right now we just waste time
> sending and then ignoring them.
> * Calling RemoveChildDoc when a doc with a parent is shutdown instead of
> where we do now, which is too late.  RemoveChildDoc needs the mAccessible
> list to still contain the child doc.  It doesnt with the priorcode --
> RemoveAccessible happens first.
> * ConstructChildDocInParentProcess needs to be delayed if the parent doc
> hasn't been constructed in the parent proc!

This is great.

When you have a chance could you push this work to try for a mochitest run?

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to David Parks (dparks) [:handyman] from comment #6)
> Created attachment 8843178 [details]
> WIP Patches that improve test success with accessibility on
> 
> These very messy patches go a long way to getting mochitests running with
> accessibility.  They are a mixture of jimms patches in this bug, tbsaunde's
> work on bug 1340579 and my own fixes.  A lot of crashes have been fixed. 
> They include:

Sorry, but I think some of this raises more questions than it answers.

> * Delaying sending events from child proc when the parent doc hasn't
> received "show" on the parent proc.  Show creates the accessible tree and
> IDs that otherwise cause asserts/crashes.  We could also just dump these
> events -- I dont know what that would do -- but right now we just waste time
> sending and then ignoring them.

I wonder if this is related to the third part.
otherwise I'm kind of skeptical of this, accessibles shouldn't be the target of other events until they have been shown.  So I'd like some explanation of what's going on.

> * Calling RemoveChildDoc when a doc with a parent is shutdown instead of
> where we do now, which is too late.  RemoveChildDoc needs the mAccessible
> list to still contain the child doc.  It doesnt with the priorcode --
> RemoveAccessible happens first.

You mean in RecvHide()?  that is sort of intentional, a document should never be the target of a hide event.  So the if you add there should never trigger, and if it does something is badly wrong because it means there is an accessible within the document that is not the document but has a parent in another document.

> * ConstructChildDocInParentProcess needs to be delayed if the parent doc
> hasn't been constructed in the parent proc!

I'd need to think about that logic more, but this problem seems correct to me.  That said I'm not so sure of how you delay events, getting the order of child doc events vs parent doc events correct is tricky.

Comment 9

[David Parks [:handyman]]

Attachment: WIP Patches that improve test success with accessibility on

Significantly improved set of WIP patches that dramatically reduce the number of test suites that fail.  Should include fixes for all top a11y crashes.

Comment 10

[David Parks [:handyman]]

Just now noticing the above comments...

(FYI, I know all this stuff is going to need a major explaining/revisiting.  Its all WIP.)

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> I wonder if this is related to the third part.
> otherwise I'm kind of skeptical of this, accessibles shouldn't be the target
> of other events until they have been shown.  So I'd like some explanation of
> what's going on.

I can't say I have the answer off the top of my head -- this is definitely what is causing the crash but I haven't dug into _why_ the events are being sent before show.
 
(Majority green!) tests: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21654c6e64cb855660c94c68e3e82c213e97984b&selectedJob=83325727

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to David Parks (dparks) [:handyman] from comment #10)
> Just now noticing the above comments...
> 
> (FYI, I know all this stuff is going to need a major explaining/revisiting. 
> Its all WIP.)
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > I wonder if this is related to the third part.
> > otherwise I'm kind of skeptical of this, accessibles shouldn't be the target
> > of other events until they have been shown.  So I'd like some explanation of
> > what's going on.
> 
> I can't say I have the answer off the top of my head -- this is definitely
> what is causing the crash but I haven't dug into _why_ the events are being
> sent before show.

fwiw I did find one case where we fired events before the actors were not ready bug 1348148, but that seems different from this.

Comment 12

[Jim Mathies [:jimm]]

Attachment: patch

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbe98d0a361
Add support for accessibility.force_disabled=-1 (force enabled) on Windows. r=aklotz

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Tom Ritter [:tjr]]

This patch breaks --disable-accessibility, as GetAccessible is not defined under that configure flag.

(What's the proper procedure to follow when a recently-accepted patch breaks something? Should the bug be reopened, or should a new bug be opened?)

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Tom Ritter [:tjr] from comment #15)
> (What's the proper procedure to follow when a recently-accepted patch breaks
> something? Should the bug be reopened, or should a new bug be opened?)

In general I would say that it should be a new bug, especially if the affected configuration is not tier-1.