1258931 - --disable-accessibility makes it impossible to run reftests

Status: VERIFIED FIXED

(Testing :: Reftest, defect)

Description

[Mats Palmgren (inactive)]

STR:
1. Add "ac_add_options --disable-accessibility" to .mozconfig
2. rebuild
3. ./mach reftest layout/reftests/css-grid/reftest.list

ACTUAL RESULTS
reftests are not run

EXPECTED RESULTS
reftests are run

This used to work until very recently.

ADDITIONAL INFORMATION
In the console output I see these error messages:

1458701394331   Marionette      DEBUG   Marionette enabled via command-line flag
1458701394754   Marionette      ERROR   Error on starting server: TypeError: Ci.nsIAccessibleStates is undefined
TypeError: Ci.nsIAccessibleStates is undefined

Comment 1

[David Baron :dbaron:]

Do you know if it's a regression from https://hg.mozilla.org/mozilla-central/rev/8f862d5425a3 ?

Comment 2

[Mats Palmgren (inactive)]

It's not, this is the changeset that broke it:
https://hg.mozilla.org/mozilla-central/rev/06baa0fa499b

Comment 3

[Andreas Tolfsen ❲:ato❳]

I'm not quite sure what the accessibility feature is, but certainly Marionette uses it at https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/accessibility.js#L41 to define states for accessibility checks.  What would cause Ci.nsIAccessibleStates to be undefined?

I can't see this being a regression from https://hg.mozilla.org/mozilla-central/rev/8f862d5425a3 as it used nsIAccessibleStates before that, but I'm not sure Marionette has ever been tested with --disable-accessibility.  If it is a problem that Marionette uses nsIAccessibleStates we shold ni? :yzen who has done work on this.

Comment 4

[Andreas Tolfsen ❲:ato❳]

I guess :ahal's patch tries to import Marionette in order to be able to install add-ons at runtime, which is needed in the world of signed add-ons.  When --disable-accessibility is specified, Ci.nsIAccessibleStates becomes undefined and the import of Marionette fails because it uses the enum in testing/marionette/accessibility.js.

Comment 5

[Andrew Halberstadt [:ahal]]

Yep. This means mochitests are also broken with --disable-accessibility (and bug 1251782 was filed for those). Reading comments over there it looks like it was tentatively WONTFIX'ed. So there are two options:

1) Make marionette work with --disable-accessibility (this might be hard, I'm not sure)
2) Provide a better error message and call it a day

I guess it depends on why people want to test with --disable-accessibility in the first place, and how important that use case is. Mats, could you help us figure out your use case here? Why are you running with --disable-accessibility? (Not saying that doing so is wrong or bad, just want to understand)

Comment 6

[David Burns :automatedtester]

Mats

Can you explain why you need to build with --disable-accessibility? Marionette does use accessibility features in it so it is required. With --disable-accessibility we move away from what the end product is built like.

Before I see about handling this case I need to understand why you're using this build flag

Comment 7

[Andreas Tolfsen ❲:ato❳]

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> 1) Make marionette work with --disable-accessibility (this might be hard,
> I'm not sure)

It’s not impossible, but I’d argue it’s a bad idea for the same reasons that #ifdefs often are harmful.  A need to run the tests on custom builds imposes unnecessary maintenance burden.  This is especially true if --disable-accessibility isn’t something we actually ship to users.

Comment 8

[Mats Palmgren (inactive)]

I and many other Gecko developers are using --disable-accessibility to reduce the time
it takes to cycle through "write code -> recompile -> run tests".  It's important for
productivity that this cycle time is as low as possible.  Turning off as many features
as you possible can helps with that.  The accessibility code base is a rather large
piece that isn't necessary for what I'm working on (layout).
http://mxr.mozilla.org/mozilla-central/source/accessible/

A secondary benefit is that it enforces layer boundaries between areas of code, so that
we don't accidentally introduce layout code that depends on accessibility code for example.
Granted, this part doesn't depend on reftests working.

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

I believe this is fixed with bug 1312816

Comment 10

[Mats Palmgren (inactive)]

I've verified it works now.  Thanks!

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Great \o/