941402 - Support intentional MOZ_RELEASE_ASSERT in the test harness

Status: NEW

(Testing :: Mochitest, defect)

Description

[Bobby Holley (:bholley)]

In bug 928636, I'm adding MOZ_RUNTIME_ASSERT, which is an assertion that fires in release builds.

We plan to use this in security-sensitive situations, so it's important to have test coverage. I tried to do this by re-implementing the Crash() function in dom/plugins/test/testplugin/nptest.cpp with MOZ_RUNTIME_ASSERT(false). But this caused the assertion to show up in TBPL logs.

Ed requested in bug 928636 comment 13 that this happen with an accompanying test harness change. The simplest would probably be to have SimpleTest.expectChildProcessCrash do the right thing for assertions as well, though I don't really know what that would look like.

bsmedberg and jorendorff both think this is important to do, which adds some weight here. Once bug 928636 lands, I'll NEEDINFO someone here.

Comment 1

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

the idea of making SimpleTest.expectChildProcessCrash handle assertions is good.  The worse piece of it is that we would be searching for assertions instead of crashes, but I think that it better than adding in a new check SimpleTest.expectRuntimeAssertions.  

Please put adequate comments in so we understand that function supports runtime assertions as well.

Comment 2

[Bobby Holley (:bholley)]

To be more clear, this needs an owner other than me. Maybe Ed can find one?

Comment 3

[Ed Morley [:emorley]]

(In reply to Bobby Holley (:bholley) from comment #0)
> In bug 928636, I'm adding MOZ_RUNTIME_ASSERT, which is an assertion that
> fires in release builds.
> 
> We plan to use this in security-sensitive situations, so it's important to
> have test coverage. I tried to do this by re-implementing the Crash()
> function in dom/plugins/test/testplugin/nptest.cpp with
> MOZ_RUNTIME_ASSERT(false). But this caused the assertion to show up in TBPL
> logs.
> 
> Ed requested in bug 928636 comment 13 that this happen with an accompanying
> test harness change. The simplest would probably be to have
> SimpleTest.expectChildProcessCrash do the right thing for assertions as
> well, though I don't really know what that would look like.
> 
> bsmedberg and jorendorff both think this is important to do, which adds some
> weight here. Once bug 928636 lands, I'll NEEDINFO someone here.

Jonathan, would you be able to find an owner from the a-team for this? :-)

Comment 4

[Jonathan Griffin (:jgriffin)]

Ted, do you have a sense for how much work this is?

Comment 5

[(not currently active) Ted Mielczarek]

I don't really know. The issue here is not that it's hard to ignore the return code of the child process (it's not). The issue is that this assert prints something to stdout/stderr which TBPL then highlights in the log. We don't have any way to suppress output from a child process, so I don't know that we can fix this in the test harness.

I suspect the way to fix this would be to change TBPL to not highlight these assertion messages, and then change the test harness to detect when one of these happens and print an additional failure message that TBPL can highlight.

Comment 6

[Jonathan Griffin (:jgriffin)]

The TBPL fix is easy too.  All the highlighting for desktop unit tests is controlled by mozharness, and it's easy to change what causes that highlighting.

Comment 7

[Jonathan Griffin (:jgriffin)]

It looks like we should be able to reproduce this problem by changing http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/nptest.cpp#79 to use MOZ_RUNTIME_ASSERT(false), and then running http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_crashing.html

Comment 8

[Bobby Holley (:bholley)]

(In reply to Jonathan Griffin (:jgriffin) from comment #7)
> It looks like we should be able to reproduce this problem by changing
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/
> nptest.cpp#79 to use MOZ_RUNTIME_ASSERT(false), and then running
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/
> test_crashing.html

That is correct.

Comment 9

[Bobby Holley (:bholley)]

Note that I think that we want some way to flag whether the assertion is expected or not. I'm happy to bundle it up with SimpleTest.expectChildProcessCrash() if that's the simplest. But in the general case I think these messages should still show up.

Comment 10

[Dan Minor [:dminor]]

Proposed solution:

1) Remove "Assertion failed" from tbpl highlighting at [1]. The "Assertion failed" will still be in the logfile, just not highlighted in the summary. Since an unexpected assertion fail should cause a process crash immediately after "Assertion failed" is printed to the log, I don't think removing it from the highlighting will hide any problems.

2) Add a specialized version of expectChildProcessCrash called expectChildProcessAssertion. This will print to the logfile that assertions are expected to fail for the next test and then call expectChildProcessCrash. If someone notices the "Assertion failed" in the log, hopefully they will also notice a few lines up a statement saying that assertions are expected to fail.

If we do still want "Assertion failed" in the summary, then another alternative would be to highlight the message from expectChildProcessAssertion so that it shows up in the summary as well.

bholley, does this meet your requirements?

[1] https://hg.mozilla.org/webtools/tbpl/file/25897506339a/php/inc/GeneralErrorFilter.php#l42

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to Dan Minor [:dminor] from comment #10)
> If we do still want "Assertion failed" in the summary, then another
> alternative would be to highlight the message from
> expectChildProcessAssertion so that it shows up in the summary as well.

Speaking from experience, we don't want to do this. People will not read everything in the summary, and they will misattribute failures.

Comment 12

[Phil Ringnalda (:philor)]

Removing "Assertion failed" from tbpl highlighting means removing it from tbpl suggestions while starring, which means that we will only see "TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/reftest/tests/content/media/test/crashtests/495794-1.html | Exited with code -6 during test run" and we will throw it at bug 890797 whether it's exiting after an "Assertion failed: (r == 0)" or another assertion or from any possible other thing that results in a -6, and will mean removing it from what tbpl spams the bug with so the only way anyone would ever notice that we were misstarring things would be to look at the log for every single thing starred in any assertion failure intermittent bug.

Comment 13

[Ed Morley [:emorley]]

Comment 12 reflects my concerns too, however the second paragraph of #2 in comment 10 should mean we can still have the assertion showing in the TBPL summary. Could I suggest something like:
TEST-UNEXPECTED-FAIL | test_filepath | Assertion failed: Foo bar
...so we just match the existing failure regexp and can just remove the "Assertion failed:" entry from TBPL's GeneralErrorFilter.php?

Comment 14

[Bobby Holley (:bholley)]

I don't feel like I should be the one driving this or making decisions.

My requirements are simple: let us |MOZ_RELEASE_ASSERT(false)| in nptest.cpp, and verify the crash in automation. Everything else is between the a-team and the sheriffs.

Comment 15

[Dan Minor [:dminor]]

(In reply to Ed Morley (Away 30th Nov until 11th Dec) [:edmorley UTC+0] from comment #13)
> Comment 12 reflects my concerns too, however the second paragraph of #2 in
> comment 10 should mean we can still have the assertion showing in the TBPL
> summary. Could I suggest something like:
> TEST-UNEXPECTED-FAIL | test_filepath | Assertion failed: Foo bar
> ...so we just match the existing failure regexp and can just remove the
> "Assertion failed:" entry from TBPL's GeneralErrorFilter.php?

That makes sense, but would it be ok to put it in place just for mochitests, or would every harness have to be modified to start generating this failure string? I'm guessing the latter.

Comment 16

[Dan Minor [:dminor]]

This try run shows a potential solution: https://tbpl.mozilla.org/?tree=Try&rev=b72e2f898206

I've added a SimpleTest.expectChildProcessAssertion() method which writes to the log that an assertion is expected. I've added an output handler to runtests.py which looks for failed assertions and writes a TEST-UNEXPECTED-FAIL unless the message from expectChildProcessAssertion is seen first.

In the try run, the crash() function causes a failed assertion instead of an actual crash. The test_crashing.html test in mochitest 3 is using the expectChildProcessAssertion and does not get flagged as orange. The bc and other suites do not call expectChildProcessAssertion and so are orange.

The failed assertion still shows up in the summary for mochitest 3, but that can be fixed by removing "Assertion failure" from the tbpl highlighting as mentioned above.

Any thoughts?

Comment 17

[(not currently active) Ted Mielczarek]

That looks very sensible, and matches my expectations. We'd just need to fix TBPL's highlighting before adding any tests that used this.

philor: if we removed TBPL's highlighting of the assertion line, and relied on the new TEST-UNEXPECTED-FAIL line, would this suffice?

Comment 18

[Phil Ringnalda (:philor)]

I'd think so, but I'm better at looking at things that exist than I am at imagining things that don't.

Comment 19

[Dan Minor [:dminor]]

Attachment: Remove tbpl highlighting for "Assertion failure"

No plan to land this until the new error message is in place.

Comment 20

[Dan Minor [:dminor]]

Attachment: Add expectChildProcessAssertion to SimpleTest.js and output handler to runtests.py

Comment 21

[Ed Morley [:emorley]]

Comment on attachment 8345262 [details] [diff] [review]
Remove tbpl highlighting for "Assertion failure"

Unless I'm misunderstanding, these assertions may be present in our other (non-mochitest) test suites too; we'll need to add the same logging output for them before removing from TBPL. Also, we'll ideally need to uplift the other patches to the release branches, since the TBPL patch here will affect all trees.

Comment 22

[Dan Minor [:dminor]]

(In reply to Ed Morley (Away 30th Nov until 11th Dec) [:edmorley UTC+0] from comment #21)
> Comment on attachment 8345262 [details] [diff] [review]
> Remove tbpl highlighting for "Assertion failure"
> 
> Unless I'm misunderstanding, these assertions may be present in our other
> (non-mochitest) test suites too; we'll need to add the same logging output
> for them before removing from TBPL. Also, we'll ideally need to uplift the
> other patches to the release branches, since the TBPL patch here will affect
> all trees.

If we have to update all of the harnesses we'll need to change:
* automation.py (reftest/crashtest)
* xpcshell (may be covered by automation.py)
* jit-tests
* cpp unittests
* talos
* jetpack
* any future test harnesses

It would be nice to do this in just one place. I asked edmorley about modifying TBPL and it sounds like this wouldn't be an easy change to do there and it is eol anyway. Mozharness is not used for every platform, so it is not really an option.

Another possibility is Treeherder. Basically we want to make error highlighting conditional on an earlier part of the log file, e.g. don't highlight the next "Assertion failure" line in a log if you have seen "Expecting assertion failure" earlier in the log. Would this be fairly straightforward?

Comment 23

[Jonathan Eads ( :jeads )]

I'm not sure I completely understand what you need but it sounds like it could be accomplished by modifying the log parser in treeherder, which should be very doable. For reference https://github.com/mozilla/treeherder-service/blob/master/treeherder/log_parser/parsers.pyx . With a bit more direction from dminor or edmorley or anyone who understands this stuff better than me, I think we can modify the log parser to accomplish this.

Comment 24

[Ed Morley [:emorley]]

We should not handle this kind of thing downstream, the test harnesses (or mozharness) should provide non-misleading output. Whilst the list of harnesses that need adjusting in comment 22 is large, that doesn't mean it it's going to be any simpler than changing both TBPL and treeherder, and it definitely would be less correct to do the latter.

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 8345263 [details] [diff] [review]
Add expectChildProcessAssertion to SimpleTest.js and output handler to runtests.py

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

This looks good, modulo the other outstanding issues we've discussed here.

Comment 26

[Dan Minor [:dminor]]

Still working on this. Recent try run: https://tbpl.mozilla.org/?tree=Try&rev=344e3f1514cd, looks like I broke jsreftests.

Comment 27

[Dan Minor [:dminor]]

I'm no longer actively working on this, so I'm unassigning myself in case someone else is interested.