1348028 - Implement fuzzPriv.enableAccessibility() in FuzzingFunctions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: bug1238028.patch

The fuzzPriv implementation is here for reference: https://github.com/MozillaSecurity/domfuzz/blob/master/dom/extension/components/domfuzzhelperobserver.js#L116

I've verified that this flips "Accessibility / Activated: true" in about:support.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8929633 [details] [diff] [review]
bug1238028.patch

+  if (rv != NS_OK) {
is not the way to check return values.
if (NS_FAILED(rv)) {
  aRv.Throw(rv);
}

r+ for the .webidl, but I'd prefer if surkov said whether just accessing the service is enough.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8929633 [details] [diff] [review]
bug1238028.patch

Review of attachment 8929633 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/FuzzingFunctions.cpp
@@ +34,5 @@
> +{
> +  RefPtr<nsIAccessibilityService> a11y;
> +  nsresult rv;
> +
> +  rv = NS_GetAccessibilityService(getter_AddRefs(a11y));

It probably won't last long. I suppose the a11y will get shutdown as soon as all a11y objects, you requested after, are garbage collected, so that's probably the way for intermittent failures if you don't have a global variable to keep a11y alive.

Comment 4

[alexander :surkov (:asurkov)]

cc'ing Yura as he's author of most of the a11y shutdown code.

Comment 5

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

Yes, if accessibility is enabled like that, it will be disabled after garbage collection if there are no references to the accessibility service or accessible objects that it creates. The consequences depend on when/if you force garbage collection, if that happens between tests and if accessibility is intended to persist between tests.

Comment 6

[Jesse Schwartzentruber (:truber)]

Thanks! I talked to eeejay and yzen about optionally keeping a global reference to the service, and about force-disabling accessibility in the future.

For now I'm matching the old behaviour since it has found bugs and we can set `GNOME_ACCESSIBILITY=1` for enabling it persistently while fuzzing on Linux. We typically force garbage collection before closing the test tab, each fuzzed testcase gets a new tab, and we're randomly forcing garbage collection during the test too.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Jesse Schwartzentruber (:truber) from comment #6)
> Thanks! I talked to eeejay and yzen about optionally keeping a global
> reference to the service, and about force-disabling accessibility in the
> future.

It probably make sense then to return an a11y service object from enableAccessibility(), so the script may control a life cycle of a11y.

Another option, if you want to keep a11y on as long as you want, then you could add a new a11y consumer nsAccessibilityService::ServiceConsumer, for example, eFuzz and then do GetOrCreateAccessible(aFuzz). You'd have release it though when you don't longer need it.

Comment 8

[Jesse Schwartzentruber (:truber)]

Attachment: bug1238028v2.patch

Update to use NS_FAILED.

Comment 9

[Jesse Schwartzentruber (:truber)]

(In reply to alexander :surkov from comment #7)
> It probably make sense then to return an a11y service object from
> enableAccessibility(), so the script may control a life cycle of a11y.

Yeah, this sounds like an easy way of controlling lifetime from within the script.

For now, enabling temporarily until GC will behave like the XUL add-on this is replacing.

Comment 10

[Jesse Schwartzentruber (:truber)]

Comment on attachment 8930168 [details] [diff] [review]
bug1238028v2.patch

Carrying r+ from previous with Comment 2 updated.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efdeb2172fe4
Implement fuzzPriv.enableAccessibility() in FuzzingFunctions r=smaug,surkov

Comment 12

[Cristina Coroiu [:ccoroiu]]

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