Local junit tests fail under x86 debug builds

RESOLVED FIXED in Firefox 62

Status

defect
P1
normal
RESOLVED FIXED
11 months ago
5 months ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
mozilla62
x86
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments)

Bug 1465480 regressed running gv junit tests locally under x86 debug builds.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

11 months ago
mozreview-review
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
Attachment #8986316 - Flags: review?(esawin) → review+

Comment 5

11 months ago
mozreview-review
Comment on attachment 8986317 [details]
Bug 1469683 - 2. Make child crashes throw special exception;

https://reviewboard.mozilla.org/r/251686/#review258224
Attachment #8986317 - Flags: review?(esawin) → review+

Comment 6

11 months ago
mozreview-review
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?
Attachment #8986318 - Flags: review?(esawin) → review+
Assignee

Comment 7

11 months ago
mozreview-review-reply
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`
Assignee

Comment 8

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

11 months ago
Pushed by nchen@mozilla.com:
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2806729c61ea
https://hg.mozilla.org/mozilla-central/rev/13ff68c7707d
https://hg.mozilla.org/mozilla-central/rev/c9487350a119
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

5 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.