397725 - (crashtest) Add test harness for assertions/crashes/leaks (crashtest)

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It'd be great to have a test suite of pages that are opened while checking for assertions, crashes and leaks. This would make it very simple to develop tests for such things as all you'd need to do is write a page that used to crash/assert/leak and dump it in a directory.

Comment 1

[Jesse Ruderman]

Several of us have been adding these kinds of things as reftests with "== about:blank" or "!= about:blank".  Having a separate harness would improve our sanity.

Comment 2

[Nickolay_Ponomarev]

Note also bug 368573, bug 385276 for checking for assertions in mochitests and reftests.

bug 397724, bug 393993 about leak checking in mochitests/reftests.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Jesse suggests adding a feature to reftest to just load simple pages without any visual comparison -- you still get the class="reftest-wait" feature.  I'd suggest just:

load file

in the manifest (rather than "== file1 file2").

Then we'd pick this up once we got a tinderbox checking for assertions, crashes, and leaks with reftest and mochitest.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to add load-only capability to reftest

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 283257 [details] [diff] [review]
patch to add load-only capability to reftest

Annoyingly enough, there are a handful of reftests that aren't passing on my machine, but that's now bug 386713 comment 3 and bug 398337.

However, other than those issues, reftests pass, and a sample "load file" and "skip load file" I stuck in temporarily work.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do we really want this in the main harness? I assume that for cases when you're working on layout and worrying about not causing regressions, there is no need to run these tests.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we might want a separate manifest for these tests, but, as Jesse pointed out in the meeting today, the main harness has some features that we may find useful (HTTP, reftest-wait, printing, etc.).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I landed the changes to the reftest harness.

Now we need to figure out where in the source tree we want to put the tests.  I sort of like "smoketests" as a name for them, except that's already taken.  Any better ideas?

Comment 9

[Rob Campbell [:rc] (:robcee)]

shakedown? (shakefist?)

passmuster?

plugsout?

What do they usually call those test flights when they try to get the bugs out of a new aircraft? "plugs out" was the name of the test for the Apollo mission Eagle capsules during the moon missions. During simulation, they'd run the tests hooked up to power and oxygen. Final tests would be done without the connections, under their own power supply, hence, "plugs out".

Maybe I'm trying too hard here. None of these names are exactly clear on what they're for in this context. A more boring alternative could be "assertcrashleak" or some combination of abbreviations, but then you lose clarity.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How about simply 'crashtest'?

Comment 11

[Rob Campbell [:rc] (:robcee)]

OK

Comment 12

[Rob Campbell [:rc] (:robcee)]

see also bug 399830...

Comment 13

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

Hmm, I had assumed sayrer would know about any effort like this, but I guess I was wrong. Sorry about the duplicated effort.

Comment 14

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

(Sorry sayrer, I didn't mean that to sound like a dig at you. I should have checked more thoroughly.)

Matt, we probably should use this instead, since it's already landed and because it doesn't duplicate code. Sorry about the wasted effort :-(. We just need to figure out how to set up manifests to gather tests from across the tree.

Comment 16

[Jeff Walden [:Waldo]]

All that's left to do here is hook this up to the build/tinderbox processes, right, so we can scatter tests anywhere we want in the tree and have them run automatically, right?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should probably have a master crashtest.list somewhere pointing to a crashtest.list at each place in the tree that has the tests.  (You saw the thread on dev.planning, right?)

Comment 18

[georgi - hopefully not receiving bugspam]

often get assertions while fuzzing or trying perverted testcases.

is there a way to tell which assertions are scary and may be exploitable (afaict some assertions are quite innocent)?

Comment 19

[Jesse Ruderman]

Me too.  My strategy is:

* Assertions that sound related to memory safety (e.g. buffer sizes, dangling pointers) or privileges (e.g. security checks, principals) get extra attention.

* In order to avoid filing duplicate bug reports, I search Bugzilla for mentions of the assertion, in both subjects and comments.  Sometimes, I notice that the assertion previously came up in an old security bug -- that's often a sign that it relates to security.

When I suspect that an assertion might be a security issue, I glance at the surrounding code, and use Bonsai's "blame" feature to find the bug and patch where the assertion was added.  If I can't understand the code well enough, sometimes I ask the developers involved before filing the bug, but usually I just guess and file the bug report, CCing the developers who seem to be involved.

Remember, assertions failures you can trigger always represent bugs.  The question is whether the bug is that the assertion is bogus or that the surrounding code is buggy.

Comment 20

[georgi - hopefully not receiving bugspam]

(In reply to comment #19)
> Me too.  My strategy is:
> 
> 
> Remember, assertions failures you can trigger always represent bugs.  The
> question is whether the bug is that the assertion is bogus or that the
> surrounding code is buggy.
> 

iirc i have seen something asserts, then real code does the same check and a safe codepath is hit - this is not exactly buggy but redundant.

not sure about the scope of this bug, but if the goal is hunting for assertions, redirecting stdout and stderr to file/pipe and then grep'ping for the right string may catch assertions. tee(1) may also help

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Add a main crashtests.list to which entries elsewhere can be added

So this adds the manifest in a central location, and it's enough to get started.

Except it's not.

$ ~/moz/builds/trunk/dist/MinefieldDebug.app/Contents/MacOS/firefox-bin -no-remote -P Trunk -reftest crashtests.list
...
###!!! ASSERTION: how are we getting an outer window?: 'innerWin->IsInnerWindow()', file /Users/jwalden/moz/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp, line 4955
REFTEST PASS (LOAD ONLY): data:text/html,
++DOMWINDOW == 8
###!!! ASSERTION: how are we getting an outer window?: 'innerWin->IsInnerWindow()', file /Users/jwalden/moz/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp, line 4955
REFTEST PASS (LOAD ONLY): data:text/html,PASS
...

So we assert even on the trivial cases, it seems.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 289441 [details] [diff] [review]
Add a main crashtests.list to which entries elsewhere can be added

The assertion's due to a local change I have (bug postMessage added the assertion here when it moved some code from timeout code into a common helper function) combined with bug 390275 comment 19.  We should get this in ASAP.

Comment 23

[georgi - hopefully not receiving bugspam]

(In reply to comment #21)
> 
> So we assert even on the trivial cases, it seems.
> 

i have seen trivial asserts (in reflow?) doing normal web activities.

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Buildbot config patch

This is completely untested.

This doesn't make assertions fatal for crashtests.  I see a setupEnvironment hook in ShellCommand which might be poked to do it, but I am loathe to armchair-experiment with the functionality without prior Buildbox experience.

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 289441 [details] [diff] [review]
Add a main crashtests.list to which entries elsewhere can be added

Checked in.

Now we just need the other patch here to get them running, plus some infra to run tests on debug builds, plus some tweaking to make asserts fatal, plus some processing to make leaks cause test failures, and I think that'd wrap things up!

Comment 26

[georgi - hopefully not receiving bugspam]

(In reply to comment #25)
>make asserts fatal

are you sure you want this?
i suspect this will render debug trunk unusable

Comment 27

[Jeff Walden [:Waldo]]

I meant make asserts fatal in the crashtest harness (only, for now -- others will follow over time), so any test it executes must execute without any assertions firing.  Non-debug builds of course can't change because assertions often rely on debug-only bookkeeping.  I hope we can reach a point where debug builds could have fatal assertions, but we're still a ways from the nirvana of really knowing what our code does.

Assertions indicate that the programmer never expected a certain situation to occur, and at that point you're at the mercy of whatever incidental error-checking might happen to detect the problem and respond in some maybe-appropriate manner.  You might get lucky.  The rate at which trunk hits assertions indicates you can get lucky for a long time (depending on the assertion you hit; we have some assertions which when hit basically guarantee an sg:critical bug exists).  However, the mere fact that the programmer couldn't reason correctly about program state to correctly say a state is impossible is enough in my book to say that we should treat it as a bug and do everything we can to fix the problem.

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 290941 [details] [diff] [review]
Buildbot config patch

Pretend I added testing/crashtest to MODULES_core in client.mk, too.

Comment 29

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 290941 [details] [diff] [review]
Buildbot config patch

this is nice, and you managed to finally clean up the Reftest class which has been a little awkwardly subclassed since the beginning.

I would get rid of the MozillaReftestBase class and just move the contents into MozillaReftest as it's a proper abstract class now anyway.

I've also just checked in some extra timeouts for Tunit so master.cfg should be updated to reflect that.

Other than those two things, this looks good.

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Take two

Comment 31

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 291312 [details] [diff] [review]
Take two

r=robcee

I'll try deploying this tomorrow or wednesday. I want to minimize the damage to the unittest boxes before the tree closes, but we should have a good window early tomorrow morning or wednesday.

Comment 32

[georgi - hopefully not receiving bugspam]

besides assertions valgrind probably can be used for catching memory corruptions.

valigrind is slow but usable (unless one hits GB ranges of data)

bugzilla search for "valgrind" shows it catches problems.

Comment 33

[Rob Campbell [:rc] (:robcee)]

Checking in client.mk;
/cvsroot/mozilla/client.mk,v  <--  client.mk
new revision: 1.356; previous revision: 1.355
done

cvs commit: Examining .
Checking in master.cfg;
/cvsroot/mozilla/tools/buildbot-configs/testing/unittest/master.cfg,v  <--  master.cfg
new revision: 1.10; previous revision: 1.9
done
Checking in mozbuild.py;
/cvsroot/mozilla/tools/buildbot-configs/testing/unittest/mozbuild.py,v  <--  mozbuild.py
new revision: 1.6; previous revision: 1.5
done

Comment 34

[Rob Campbell [:rc] (:robcee)]

first run appeared to work, though it might've been complaining about a missing manifest. The only output was:

REFTEST EXCEPTION: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIFileInputStream.init]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://reftest/content/reftest.js :: ReadManifest :: line 135"  data: no]program finished with exit code 0

has everything been checked in?

Comment 35

[Jeff Walden [:Waldo]]

Looks like I screwed up a path somewhere.  Try adding a dump just before that line for listURL.spec and see what's printed, should be easy enough to fix:

http://mxr.mozilla.org/mozilla/source/layout/tools/reftest/reftest.js#135

Comment 36

[Jeff Walden [:Waldo]]

Comment on attachment 291312 [details] [diff] [review]
Take two

>Index: tools/buildbot-configs/testing/unittest/mozbuild.py

>+class MozillaUnixCrashtest(MozillaCrashtest):
>+    command = ["../../objdir/dist/bin/run-mozilla.sh",
>+               "../../objdir/dist/bin/firefox",
>+               "-P",
>+               "default",
>+               "-reftest",
>+               "crashtest.list"]
>+
>+class MozillaOSXCrashtest(MozillaCrashtest):
>+    command = ["../../objdir/dist/Minefield.app/Contents/MacOS/firefox",
>+               "-console",
>+               "-P",
>+               "default",
>+               "-reftest",
>+               "crashtest.list"]
>+
>+class MozillaWin32Crashtest(MozillaCrashtest):
>+    command = [r'..\..\objdir\dist\bin\firefox.exe -P debug -reftest crashtest.list']

...and yet:

http://mxr.mozilla.org/mozilla/source/testing/crashtest/crashtests.list

Those three should be "crashtests.list", not "crashtest.list", dangit.

Comment 37

[Rob Campbell [:rc] (:robcee)]

shewt.

I'll fix locally and recheck in...

Comment 38

[Rob Campbell [:rc] (:robcee)]

Attachment: buildbot steps, take 3

fix for broken crashtest step.

Comment 39

[Rob Campbell [:rc] (:robcee)]

Checking in mozbuild.py;
/cvsroot/mozilla/tools/buildbot-configs/testing/unittest/mozbuild.py,v  <--  mozbuild.py
new revision: 1.7; previous revision: 1.6
done

Comment 40

[Ray Kiddy]

Can I suggest you can parameterize the Mac OS app name in the environment vars block. "Minefield.app" only works for now.

This seems a classic source of those bug fixes that one sees that say "Oops, forgot to change name of foobar", or "Oops, forgot to change one the foobar references", or "Changing foobar references in error messages/user doc/(whatever)". I just thought I would point out the opportunity to do it now, rather than later.