1469683 - Local junit tests fail under x86 debug builds

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Bug 1465480 regressed running gv junit tests locally under x86 debug builds.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1469683 - 1. Fix crash tests;

Specify individual sessions in crash tests (i.e.
"mainSession.waitUntilCalled" instead of "sessionRule.waitUntilCalled"),
so that the tests assert behavior on the correct session, and not
inadvertently on the cached session.

Also, under x86 debug builds, Gecko installs an "ah_crap_handler" for
SIGSEGV that waits for a long time, which causes our crash tests to time
out. Therefore, ignore crash tests under x86 debug.

Review commit: https://reviewboard.mozilla.org/r/251684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251684/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1469683 - 2. Make child crashes throw special exception;

Make child crashes throw ChildCrashException instead of the generic
RuntimeException, which can mean a variety of errors such as timeout.

Review commit: https://reviewboard.mozilla.org/r/251686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251686/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1469683 - 3. Make sure cached session is closed after crash;

If @IgnoreCrash is specified, assume there has been a crash and
therefore wait for the cached session to close at the end of the test,
so that the closing operation doesn't carry over to the next test.

Review commit: https://reviewboard.mozilla.org/r/251688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251688/

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8986316 [details]
Bug 1469683 - 1. Fix crash tests;

https://reviewboard.mozilla.org/r/251684/#review258222

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:68
(Diff revision 1)
>      @IgnoreCrash
>      @ReuseSession(false)
>      @Test fun crashContent() {
>          // This test doesn't make sense without multiprocess
>          assumeThat(sessionRule.env.isMultiprocess, equalTo(true))
> +        // Cannot test x86 debug builds due to "ah_crap_handler".

Can you expand this comment a bit, had to look up what that handler does.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:72
(Diff revision 1)
>          assumeThat(sessionRule.env.isMultiprocess, equalTo(true))
> +        // Cannot test x86 debug builds due to "ah_crap_handler".
> +        assumeThat(sessionRule.env.isDebugBuild && sessionRule.env.cpuArch == "x86",
> +                   equalTo(false))
>  
> -        sessionRule.session.loadUri(CONTENT_CRASH_URL)
> +        mainSession.loadUri(CONTENT_CRASH_URL)

Please add a comment on why we have to use the mainSession here.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt:102
(Diff revision 1)
> -
> -        // We need to make sure all sessions in a given content process
> -        // receive onCrash(). If we add multiple content processes, this
> -        // test will need fixed to ensure the test sessions go into the
> -        // same one.
> -        sessionRule.createOpenSession()
> +        // Cannot test x86 debug builds due to "ah_crap_handler".
> +        assumeThat(sessionRule.env.isDebugBuild && sessionRule.env.cpuArch == "x86",
> +                   equalTo(false))
> +
> +        // XXX we need to make sure all sessions in a given content process receive onCrash(). If we
> +        // add multiple content processes, this test will need fixed to ensure the test sessions go

+ to be

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8986317 [details]
Bug 1469683 - 2. Make child crashes throw special exception;

https://reviewboard.mozilla.org/r/251686/#review258224

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8986318 [details]
Bug 1469683 - 3. Make sure cached session is closed after crash;

https://reviewboard.mozilla.org/r/251688/#review258228

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:1406
(Diff revision 1)
>       * Internal method to perform callback checks at the end of a test.
>       */
>      public void performTestEndCheck() {
> +        if (sCachedSession != null && mIgnoreCrash) {
> +            // Make sure the cached session has been closed by crashes.
> +            while (sCachedSession.isOpen()) {

Should we add a timeout for stuck sessions?

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8986316 [details]
Bug 1469683 - 1. Fix crash tests;

https://reviewboard.mozilla.org/r/251684/#review258222

> Please add a comment on why we have to use the mainSession here.

`mainSession` is just a shortcut for `sessionRule.session`

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8986318 [details]
Bug 1469683 - 3. Make sure cached session is closed after crash;

https://reviewboard.mozilla.org/r/251688/#review258228

> Should we add a timeout for stuck sessions?

In that case `loopUntilIdle` will time out and throw an exception.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8986316 [details]
Bug 1469683 - 1. Fix crash tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251684/diff/1-2/

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8986317 [details]
Bug 1469683 - 2. Make child crashes throw special exception;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251686/diff/1-2/

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8986318 [details]
Bug 1469683 - 3. Make sure cached session is closed after crash;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251688/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2806729c61ea
1. Fix crash tests; r=esawin
https://hg.mozilla.org/integration/autoland/rev/13ff68c7707d
2. Make child crashes throw special exception; r=esawin
https://hg.mozilla.org/integration/autoland/rev/c9487350a119
3. Make sure cached session is closed after crash; r=esawin

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2806729c61ea
https://hg.mozilla.org/mozilla-central/rev/13ff68c7707d
https://hg.mozilla.org/mozilla-central/rev/c9487350a119