Closed Bug 1342245 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: aes+)

Attachments

(1 file, 6 obsolete files)

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
Attached file tmp.txt (obsolete) —
Attached patch patch (obsolete) — Splinter Review
Attachment #8840891 - Attachment is obsolete: true
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.
Attachment #8840892 - Flags: review?(aklotz)
Attachment #8840892 - Flags: review?(aklotz) → review+
Attached patch debugging work (obsolete) — Splinter Review
Attached patch flip pref (obsolete) — Splinter Review
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!
(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?
(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.
Significantly improved set of WIP patches that dramatically reduce the number of test suites that fail.  Should include fixes for all top a11y crashes.
Attachment #8843178 - Attachment is obsolete: true
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
(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.
Attached patch patchSplinter Review
Attachment #8840892 - Attachment is obsolete: true
Attachment #8841724 - Attachment is obsolete: true
Attachment #8841725 - Attachment is obsolete: true
Attachment #8846735 - Attachment is obsolete: true
Attachment #8852026 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbe98d0a361
Add support for accessibility.force_disabled=-1 (force enabled) on Windows. r=aklotz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ccbe98d0a361
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?)
Flags: needinfo?(jmathies)
(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.
No longer blocks: 1351720
Depends on: 1351720
Flags: needinfo?(jmathies)
You need to log in before you can comment on or make changes to this bug.