Closed Bug 918246 Opened 7 years ago Closed 2 years ago

Running crashtests with accessibility give assertions and crashes

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: martijn.martijn, Assigned: yzen)

References

()

Details

Attachments

(1 file)

In bug 917061 I tried to enable the one accessibility crashtest, but with a try run, I ran into crashes and assertions: https://tbpl.mozilla.org/?tree=Try&rev=f9128379e6ef
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/docshell/base/crashtests/430628-1.html | assertion count 1 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/editor/libeditor/html/crashtests/643786-1.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/editor/libeditor/html/crashtests/768748.html | assertion count 1 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/editor/txmgr/tests/crashtests/407072-1.html | assertion count 1 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/514104-1.xul | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/615781-1.xhtml | assertion count 1 is more than expected 0 assertions
Bug 855410 - Intermittent layout/base/crashtests/615781-1.xhtml | Exited with code 1 during test run | application crashed [@ (anonymous namespace)::ClearAttrCache(nsAString_internal const&, MiscContainer*&, void*)] REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/style/crashtests/416461-1.xul | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/base/src/crashtests/265161-1.xul | assertion count 1 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/base/src/crashtests/381862.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/base/src/crashtests/508927-1.xul | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/base/src/crashtests/508927-2.xul | assertion count 24 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/base/src/crashtests/514300-1.xul | assertion count 7 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/grid/crashtests/321073-1.xul | assertion count 1 is more than expected 0 assertions
TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/tree/crashtests/380217-1.xul | Exited with code -11 during test run
PROCESS-CRASH | file:///builds/slave/test/build/tests/reftest/tests/layout/xul/tree/crashtests/380217-1.xul | application crashed [@ nsCOMPtr<nsINodeInfo>::operator->() const]
Return code: 256
Running *some* of the crashtests with accessibility enabled, because they happen to come after this test, doesn't seem right.

Long-term, it would be nice for all our test suites to pass with accessibility enabled, possibly after more progress is made on bug 571613.

I think that crashtest should be moved to mochitest-a11y for now.
Depends on: 571613
https://tbpl.mozilla.org/php/getParsedLog.php?id=28007606&tree=Try&full=1#error1

a XUL tree crash, we need somebody from layout to look, probably Robert will advice
OS: Mac OS X → All
(In reply to Jesse Ruderman from comment #1)
> Running *some* of the crashtests with accessibility enabled, because they
> happen to come after this test, doesn't seem right.
> 
> Long-term, it would be nice for all our test suites to pass with
> accessibility enabled, possibly after more progress is made on bug 571613.

yeah, but we probably don't want to run all our test suites with it on all the time given it not being on is probably the more common case.

> I think that crashtest should be moved to mochitest-a11y for now.

we've certainly put other things in mochitest-a11y that are really just crash / assertion tests.  I think I though about doing that with this test at one point and decided it was probably a duplicate of tests we already had or just useless, but apparently it crashes now so I guess I was wrong.
(In reply to alexander :surkov from comment #2)
> https://tbpl.mozilla.org/php/getParsedLog.
> php?id=28007606&tree=Try&full=1#error1
> 
> a XUL tree crash, we need somebody from layout to look, probably Robert will
> advice

It's not obvious from the log what the problem is.
Also adding Ryan since he has a try run with the patch that shows it in action:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9951824e7c7966bd3366b1110e482a6f659f19b4&group_state=expanded
Flags: needinfo?(ryanvm)
Attachment #8921290 - Flags: review?(surkov.alexander)
Assignee: nobody → yzenevich
Flags: needinfo?(ryanvm)
Comment on attachment 8921290 [details] [diff] [review]
a11y-crashtest-service-shutdown.patch

I also ran a crashtest on top of this patch from a bug that's known to still be affected and confirmed that everything works correctly with respect to catching failures in it without causing later issues. This looks good to me!
Attachment #8921290 - Flags: feedback+
Comment on attachment 8921290 [details] [diff] [review]
a11y-crashtest-service-shutdown.patch

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

cool to have a11y crashtests alive!

::: accessible/tests/crashtests/471493.xul
@@ +9,5 @@
>    <![CDATA[
> +    function addA11yLoadEvent(accService, win, func) {
> +      function waitForDocLoad() {
> +        setTimeout(() => {
> +            let targetDocument = win ? win.document : document;

you need neither win argument nor targetDocument variable, correct?

::: accessible/tests/crashtests/crashtests.list
@@ +1,5 @@
> +load 448064.xhtml
> +load 471493.xul
> +# shutdown_a11y.xul MUST be the last test in the list because it is responsible
> +# for shutting down accessibility service affecting later tests.
> +load shutdown_a11y.xul

might be nice to have more explicit name which says we unitialize the test suite, for example, last_test_to_unload_testsuite.xul

::: accessible/tests/crashtests/shutdown_a11y.xul
@@ +8,5 @@
> +  <script type="application/javascript">
> +  <![CDATA[
> +    function shutdown() {
> +      !SpecialPowers.Services.appinfo.accessibilityEnabled &&
> +        dump("### Error : Accessibility is expected to be enabled.\n");

nit: a new line after it. I'm curious if there's more standardish mechanism to assert in tests.

@@ +9,5 @@
> +  <![CDATA[
> +    function shutdown() {
> +      !SpecialPowers.Services.appinfo.accessibilityEnabled &&
> +        dump("### Error : Accessibility is expected to be enabled.\n");
> +      // Force garbage collection.

it'd be good to have a small comment why you need triple gc stuff; if that's the magic gestures, then say so :)

@@ +24,5 @@
> +          data === "0" && document.documentElement.removeAttribute("class");
> +        };
> +        SpecialPowers.Services.obs.addObserver(observe, "a11y-init-or-shutdown");
> +      } else {
> +        document.documentElement.removeAttribute("class");

does it happen?
Attachment #8921290 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #7)
> Comment on attachment 8921290 [details] [diff] [review]
> a11y-crashtest-service-shutdown.patch
> 
> Review of attachment 8921290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> cool to have a11y crashtests alive!
> 
> ::: accessible/tests/crashtests/471493.xul
> @@ +9,5 @@
> >    <![CDATA[
> > +    function addA11yLoadEvent(accService, win, func) {
> > +      function waitForDocLoad() {
> > +        setTimeout(() => {
> > +            let targetDocument = win ? win.document : document;
> 
> you need neither win argument nor targetDocument variable, correct?

sure

> 
> ::: accessible/tests/crashtests/crashtests.list
> @@ +1,5 @@
> > +load 448064.xhtml
> > +load 471493.xul
> > +# shutdown_a11y.xul MUST be the last test in the list because it is responsible
> > +# for shutting down accessibility service affecting later tests.
> > +load shutdown_a11y.xul
> 
> might be nice to have more explicit name which says we unitialize the test
> suite, for example, last_test_to_unload_testsuite.xul

done

> 
> ::: accessible/tests/crashtests/shutdown_a11y.xul
> @@ +8,5 @@
> > +  <script type="application/javascript">
> > +  <![CDATA[
> > +    function shutdown() {
> > +      !SpecialPowers.Services.appinfo.accessibilityEnabled &&
> > +        dump("### Error : Accessibility is expected to be enabled.\n");
> 
> nit: a new line after it. I'm curious if there's more standardish mechanism
> to assert in tests.

doesn't seem like there is. I borrowed what's been used in other crash/ref tests.

> 
> @@ +9,5 @@
> > +  <![CDATA[
> > +    function shutdown() {
> > +      !SpecialPowers.Services.appinfo.accessibilityEnabled &&
> > +        dump("### Error : Accessibility is expected to be enabled.\n");
> > +      // Force garbage collection.
> 
> it'd be good to have a small comment why you need triple gc stuff; if that's
> the magic gestures, then say so :)

done

> 
> @@ +24,5 @@
> > +          data === "0" && document.documentElement.removeAttribute("class");
> > +        };
> > +        SpecialPowers.Services.obs.addObserver(observe, "a11y-init-or-shutdown");
> > +      } else {
> > +        document.documentElement.removeAttribute("class");
> 
> does it happen?

it might. We account for all possibilities, in case we somehow gc'ed before this test ran and we cleaned up our xpcom bits.
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5624ec2b5866
ensure accessibility service is shutdown after a11y crashtests. r=surkov
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/5624ec2b5866
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.