918246 - Running crashtests with accessibility give assertions and crashes

Reporter

Description

•

11 years ago

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

Jesse Ruderman

Comment 1

•

11 years ago

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.

Martijn Wargers (dead)

Reporter

Updated

•

11 years ago

Depends on: 571613

alexander :surkov (:asurkov)

Comment 2

•

11 years ago

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

alexander :surkov (:asurkov)

Updated

•

11 years ago

OS: Mac OS X → All

Trevor Saunders (:tbsaunde)

Comment 3

•

11 years ago

(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.

Robert O'Callahan (:roc) (email my personal email if necessary)

Comment 4

•

11 years ago

(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.

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

Assignee

Comment 5

•

7 years ago

Attached patch a11y-crashtest-service-shutdown.patch — Details — Splinter Review

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)

Ryan VanderMeulen [:RyanVM]

Updated

•

7 years ago

Assignee: nobody → yzenevich

Flags: needinfo?(ryanvm)

Ryan VanderMeulen [:RyanVM]

Comment 6

•

7 years ago

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+

alexander :surkov (:asurkov)

Comment 7

•

7 years ago

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+

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

Assignee

Comment 8

•

7 years ago

(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.

Pulsebot

Comment 9

•

7 years ago

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

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

Assignee

Updated

•

7 years ago

Status: NEW → ASSIGNED

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 10

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/5624ec2b5866

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox58: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Bugzilla

Quick Search

Running crashtests with accessibility give assertions and crashes

Categories

(Core :: Disability Access APIs, defect)

Tracking

()

People

(Reporter: martijn.martijn, Assigned: yzen)

References

(
URL
)

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Comment 1

Updated

Comment 2

Updated

Comment 3

Comment 4

Comment 5

Updated

Comment 6

Comment 7

Comment 8

Comment 9

Updated

Comment 10

Attachment

General

Description

File Name

Content Type