1145444 - Buffer overflows in ASan bc e10s tests don't turn the tree orange

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

See bug 1145443.  For some reason, this isn't turning the tree orange, which is bad.  Something about how ASan is reporting errors is not being detected by Tree Herder.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I've hidden this because there's currently a buffer overflow on trunk.

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

This is particularly weird because it does output UNEXPECTED-FAIL:


SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/2d/convolver.h:121 operator->
[Child 2984] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1597
[Child 2984] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1597
SUMMARY: AddressSanitizer: SEGV /builds/slave/m-in-l64-asan-0000000000000000/build/src/memory/mozalloc/mozalloc_abort.cpp:33 mozalloc_abort(char const*)
125 ERROR TEST-UNEXPECTED-FAIL | dom/html/test/browser_bug592641.js | application terminated with exit code 1

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Is this something you could look at Chris?  This is probably something like bug 1044206.

We're emitting the TEST-UNEXPECTED-FAIL here and somehow Mozharness or whatever doesn't realize it is an error:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1804

Comment 4

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Is this something you could look at Chris?  This is probably something like
> bug 1044206.
> 
> We're emitting the TEST-UNEXPECTED-FAIL here and somehow Mozharness or
> whatever doesn't realize it is an error:
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.
> py#1804

Sure.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f812713f903

Comment 6

[Chris Manchester (limited bugmail, email directly)]

(In reply to Chris Manchester [:chmanchester] from comment #5)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f812713f903

Ok, that worked. Here's a try run with all the mochitests in case of unintended effects elsewhere:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68db34c6633c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Can we add a test for this?

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: Patch with a fix

This patch probably looks backwards. The comment around the MochitestFormatter in runtests.py explains the situation: we need to dump "TEST-UNEXPECTED-" with no line prefix in order to alert mozharness when something other than a test failure needs to turn a job orange, but at some point it was requested that "ERROR" get pre-pended so doing "log.error" without "TEST-UNEXPECTED-" would get highlighted by a regex in TBPL.

However, this isn't usually the only thing that would turn a job orange in failure scenarios like these: when the application ends abruptly due to a crash that gets reported through mozcrash we get a "PROCESS-CRASH" line, or the harness process provides a non-zero exit code, or no summary text is found in the log, any one of which is enough to turn the job orange.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8580834 [details] [diff] [review]
Patch with a fix

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

this patch looks great.  Is there a chance that we could have a way to verify this?  a unit test perhaps?  I suspect the real way to test this is outside the scope of mochitests.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 8580834 [details] [diff] [review]
> Patch with a fix
> 
> Review of attachment 8580834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this patch looks great.  Is there a chance that we could have a way to
> verify this?  a unit test perhaps?  I suspect the real way to test this is
> outside the scope of mochitests.

I think this would take something like standing up selftests to run on every checkin and all the platforms we care about and then writing tests that get the browser to do all the things I mentioned in comment 8.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Can you help coordinate an appropriate time to land this given the buffer overflow on trunk?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

We backed out the patch that caused it, so land away.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5444d8c669fc

Comment 14

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Thanks for the fix!

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5444d8c669fc

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ef4054f3ec1
https://hg.mozilla.org/releases/mozilla-beta/rev/1efc8c39543c
https://hg.mozilla.org/releases/mozilla-release/rev/6040c3cbd3fe

Comment 17

[Andrew McCreight (out of office until 8/21) [:mccr8]]

This isn't e10s-specific.