Closed Bug 397725 (crashtest) Opened 12 years ago Closed 12 years ago

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

Categories

(Testing :: Reftest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sicking, Assigned: Waldo)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
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.
Note also bug 368573, bug 385276 for checking for assertions in mochitests and reftests.

bug 397724, bug 393993 about leak checking in mochitests/reftests.
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 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.
Attachment #283257 - Flags: review?(jwalden+bmo)
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.
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.).
Attachment #283257 - Flags: review?(jwalden+bmo) → review+
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?
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.
How about simply 'crashtest'?
Hmm, I had assumed sayrer would know about any effort like this, but I guess I was wrong. Sorry about the duplicated effort.
(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.
Duplicate of this bug: 399830
Blocks: 363696
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?
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?)
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)?
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.
(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
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 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.
Attachment #289441 - Flags: review?(sayrer)
(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.
Attachment #289441 - Flags: review?(sayrer) → review+
Attachment #289441 - Flags: approval1.9?
Attached patch Buildbot config patch (obsolete) — Splinter Review
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.
Attachment #290941 - Flags: review?(rcampbell)
Attachment #289441 - Flags: approval1.9? → approval1.9+
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!
(In reply to comment #25)
>make asserts fatal

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


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.
Assignee: nobody → jwalden+bmo
Comment on attachment 290941 [details] [diff] [review]
Buildbot config patch

Pretend I added testing/crashtest to MODULES_core in client.mk, too.
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.
Attachment #290941 - Flags: review?(rcampbell) → review-
OS: Windows XP → All
Hardware: PC → All
Attached patch Take twoSplinter Review
Attachment #290941 - Attachment is obsolete: true
Attachment #291312 - Flags: review?
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.
Attachment #291312 - Flags: review? → review+
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.
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
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?
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 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.
shewt.

I'll fix locally and recheck in...
fix for broken crashtest step.
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
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: Add test harness for assertions/crashes/leaks → Add test harness for assertions/crashes/leaks (crashtest)
Alias: crashtest
Component: Testing → Reftest
Product: Core → Testing
QA Contact: testing → reftest
You need to log in before you can comment on or make changes to this bug.