We have some tests that intentionally crash the test plugin. Currently, they leave the minidumps laying around, so we get confusing PROCESS-CRASH output in the logs. In the near future we would like to fix bug 642167 and upload any minidumps resulting from a test run to ftp.mo alongside the build, so developers could download them for more useful debugging. However, before we do that we should clean up these known crashes so that we aren't uploading a bunch of confusing unhelpful dumps.

OK, I understand better what's going on now. Where is the intentional plugin crash run?

(That comment was meant to be for bug 645585, but here works too...)

It's all the test_crash* tests here: http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/mochitest/ Probably the best solution would be to add a method to SimpleTest, like SimpleTest.expectPluginCrash(), and the harness could register for the "plugin-crashed" observer service notification (just like the tests do), to get the minidump filename, and then remove those files after the test finishes.

Bleh, I apparently couldn't find bug 539823 when I filed this. Probably because I think, as filed, it's wrong.

I'll take this then. (I see that e.g. test_crash_notify.xul already cleans up after itself.) I don't know what the .extra files are, but I'm assuming they should be removed too?

The .dmp files contain the stack memory etc, the .extra files just contain metadata. They should all be removed. (For each crash, there's a .dmp and .extra with the same GUID basename.)

test_crash_notify should be a good example, you can probably factor that code out into the test harness. Let me know if you need any other pointers.

There are some reftests that also deliberately crash plugins. Do you have a suggestion on a way to indicate to the reftest harness that the minidumps should be removed? I was thinking either to check for class="reftest-expect-plugin-crash" once the test has finished, or to add a new annotation into the reftest manifest.

(Actually checking it at the start of the test would be better, to give the harness a chance to register plugin crash listeners in time.)

I don't know. The harness should be able to just register the plugin crash observer unconditionally, and only remove the crash if the test signals that it was intentional, right?

(But I would recommend splitting reftest work into a separate bug, to keep the scope small here.)

I filed bug 652501 for the reftest side of this.

Some ancillary cleanups of the mochitest harness while I'm here.

Comment on attachment 529668 [details] [diff] [review] Part 3: Send a notification message when a plugin hangs You need cjones or some other IPC peer to review this.

Comment on attachment 529668 [details] [diff] [review] Part 3: Send a notification message when a plugin hangs I don't understand the need for this patch. ShouldContinueFromReplyTimeout() calls KillProcess() and then returns false. In SyncChannel::ShouldContinueFromTimeout(), a false return sets the channel state to ChannelTimeout. We either get the error notification from the IO thread first, from the KillProcess(), which results in ActorDestroy(AbnormalShutdown), or the Close() call in PluginModuleParent::CleanupFromTimeout() succeeds and we get an error notification because the channel state is ChannelTimeout (see AsyncChannel::Close()). What behavior are you observing? This might be a bad bug we need to fix ASAP.

Comment on attachment 529668 [details] [diff] [review] Part 3: Send a notification message when a plugin hangs I must have been wrong in my testing. A try server run without the additional NotifyPluginCrashed() call seemed to work fine just now.

Comment on attachment 529666 [details] [diff] [review] Part 1: Simplify mochitest logging, and other minor cleanups Review of attachment 529666 [details] [diff] [review]: ----------------------------------------------------------------- I don't really like a lot of your rewrites here, sorry. The logEnabled / log cleanup is good stuff, though. ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +18,5 @@ > > +var parentRunner = > + typeof parent != "undefined" && parent && > + (parent.TestRunner || > + parent.wrappedJSObject && parent.wrappedJSObject.TestRunner); This...is a little too clever for me. Not that the original is terribly clear, but I find this very hard to read. Can you find a happy medium? @@ +23,3 @@ > > +// Simple test to see if we are running in e10s IPC > +var ipcMode = parentRunner && parentRunner.ipcMode; This feels weird, since ipcMode can wind up being a non-boolean value. It *should* work, assuming nothing ever tests the value directly, but it feels weird. @@ +98,5 @@ > SimpleTest._tests.push(test); > }; > > SimpleTest._logResult = function(test, passString, failString) { > + var isError = !test.result == !test.todo; Isn't this the same as saying "isError = test.result == test.todo" ? @@ +100,5 @@ > > SimpleTest._logResult = function(test, passString, failString) { > + var isError = !test.result == !test.todo; > + var url = parentRunner && parentRunner.currentTestURL || ""; > + var msg = ((test.result ? passString : failString) || "") + I think worrying about whether passString / failString are not strings isn't worth it. @@ +103,5 @@ > + var url = parentRunner && parentRunner.currentTestURL || ""; > + var msg = ((test.result ? passString : failString) || "") + > + " | " + url + > + " | " + test.name + > + (test.diag ? " - " + test.diag : "") I'm not really sure that as a whole this is better than what it's replacing. I think lots of inline ternary operators and boolean short-circuiting winds up making it less readable overall. ::: testing/mochitest/tests/SimpleTest/TestRunner.js @@ +51,5 @@ > + return false; > + } catch (e) { > + return true; > + } > +})(); I don't think this is better than the original.

Comment on attachment 529667 [details] [diff] [review] Part 2: Allow mochitests to clean up plugin and IPC process crash dumps Review of attachment 529667 [details] [diff] [review]: ----------------------------------------------------------------- I like the concept, but we need to avoid adding more enablePrivilege to Mochitest, even if it's for a good cause. I have some other quibbles with the patch, see below. ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +559,5 @@ > }; > > +/** > + * Indicates to the test framework that the current test expects one or > + * more crashes (from plugins or IPC documnets), and that the minidumps from "documents" @@ +562,5 @@ > + * Indicates to the test framework that the current test expects one or > + * more crashes (from plugins or IPC documnets), and that the minidumps from > + * those crashes should be removed. > + */ > +SimpleTest.expectProcessCrash = function () { Should we call this "expectChildProcessCrash" or something slightly more descriptive? We're certainly not indicating that we expect the current process to crash... Too pedantic? @@ +564,5 @@ > + * those crashes should be removed. > + */ > +SimpleTest.expectProcessCrash = function () { > + if (parentRunner) { > + parentRunner.expectProcessCrash(); You know, looking at a lot of this code, we should just fix the harness to ensure that parentRunner is always set. (Doesn't have to happen here, though.) @@ +785,5 @@ > window.onerror = function simpletestOnerror(errorMsg, url, lineNumber) { > var funcIdentifier = "[SimpleTest/SimpleTest.js, window.onerror] "; > > // Log the message. > + SimpleTest._logInfo(funcIdentifier, "An error occurred: " + errorMsg + " at " + url + ":" + lineNumber); You're changing the semantics here, is that really what you want? (I'm not actually sure this worked in the first place, but just checking.) ::: testing/mochitest/tests/SimpleTest/TestRunner.js @@ +63,5 @@ > + Components.classes["@mozilla.org/file/directory_service;1"]. > + getService(Components.interfaces.nsIProperties); > + var crashDumpDir = directoryService.get("ProfD", Components.interfaces.nsIFile); > + crashDumpDir.append("minidumps"); > + return crashDumpDir; Can we avoid adding more enablePrivilege to the harness? We're in the (slow) process of ripping it all out. It's probably a bit awkward here, admittedly, but you can add whatever you want to SpecialPowers to make this happen: https://developer.mozilla.org/en/SpecialPowers You'll probably have to move the whole observer bit into SpecialPowers, and just provide methods like: SpecialPowers.registerProcessCrashObservers(); SpecialPowers.removeExpectedCrashDumpFiles(); @@ +289,5 @@ > + } > + TestRunner._expectedCrashDumpFiles.length = 0; > +}; > + > +TestRunner.findUnexpectedCrashDumpFiles = function() { This is probably not useful as written. If a test has an unexpected plugin crash, then you'll error here. However, you don't keep this list of files, so you'll print the same error for later tests as well. This seems useful, since it will cause us to explicitly fail tests that cause plugin/oop content crashes, but you'll need to save the list of dump files you've already seen to prevent repeating the error.

Comment on attachment 529669 [details] [diff] [review] Part 4: Make existing plugin crash mochitests clean up after themselves Review of attachment 529669 [details] [diff] [review]: -----------------------------------------------------------------

(In reply to comment #20) > I don't really like a lot of your rewrites here, sorry. The logEnabled / log > cleanup is good stuff, though. The non-log related cleanups aren't terribly important, they were more opportunistic than anything. BTW, the first hunk of this patch, var SimpleTest = SimpleTest || {}; I think now we should just do var SimpleTest = {}; The only reason we might want to do the former is to somehow protect against multiple loads of SimpleTest.py. I don't think we need to cater for that. > ::: testing/mochitest/tests/SimpleTest/SimpleTest.js > @@ +18,5 @@ > > > > +var parentRunner = > > + typeof parent != "undefined" && parent && > > + (parent.TestRunner || > > + parent.wrappedJSObject && parent.wrappedJSObject.TestRunner); > > This...is a little too clever for me. Not that the original is terribly > clear, but I find this very hard to read. Can you find a happy medium? I assume "parent" here is "window.parent", so there's no real need to check for it with typeof. Let's just do: var parentRunner; if (parent) { parentRunner = parent.TestRunner; if (!parentRunner && parent.wrappedJSObject) { parentRunner = parent.wrappedJSObject.TestRunner; } } (When would would we run into the parent.wrappedJSObject case, anyway?) > @@ +23,3 @@ > > > > +// Simple test to see if we are running in e10s IPC > > +var ipcMode = parentRunner && parentRunner.ipcMode; > > This feels weird, since ipcMode can wind up being a non-boolean value. It > *should* work, assuming nothing ever tests the value directly, but it feels > weird. It's equivalent to the existing code, which assigns either false (if there's no parent runner) or the exact value of parentRunner.ipcMode (which is only ever a Boolean, set in TestRunner), so I'm not sure where the weirdness is. What might be weird is having a variable named "ipcMode" but which is really only true/false. Maybe renaming it to "isIPC" or "hasIPCContentProcesses" or something would be clearer? > @@ +98,5 @@ > > SimpleTest._tests.push(test); > > }; > > > > SimpleTest._logResult = function(test, passString, failString) { > > + var isError = !test.result == !test.todo; > > Isn't this the same as saying "isError = test.result == test.todo" ? Only if test.result and test.todo are always Booleans. I see that SimpleTest.ok etc. already do "!!condition", so you're right we can just test for equality. > @@ +100,5 @@ > > > > SimpleTest._logResult = function(test, passString, failString) { > > + var isError = !test.result == !test.todo; > > + var url = parentRunner && parentRunner.currentTestURL || ""; > > + var msg = ((test.result ? passString : failString) || "") + > > I think worrying about whether passString / failString are not strings isn't > worth it. You're right, the only callers of _logResult either always pass a string for both passString and failString, or only pass one for passString but unconditionally have result = true and todo = false. > @@ +103,5 @@ > > + var url = parentRunner && parentRunner.currentTestURL || ""; > > + var msg = ((test.result ? passString : failString) || "") + > > + " | " + url + > > + " | " + test.name + > > + (test.diag ? " - " + test.diag : "") > > I'm not really sure that as a whole this is better than what it's replacing. > I think lots of inline ternary operators and boolean short-circuiting winds > up making it less readable overall. OK, I'll restore the previous structure. > ::: testing/mochitest/tests/SimpleTest/TestRunner.js > @@ +51,5 @@ > > + return false; > > + } catch (e) { > > + return true; > > + } > > +})(); > > I don't think this is better than the original. This was my attempt to limit the UniversalXPConnect privilege to that block of code. Since I'll be changing to using SpecialPowers I'll revert this.

(In reply to comment #21) > @@ +562,5 @@ > > + * Indicates to the test framework that the current test expects one or > > + * more crashes (from plugins or IPC documnets), and that the minidumps from > > + * those crashes should be removed. > > + */ > > +SimpleTest.expectProcessCrash = function () { > > Should we call this "expectChildProcessCrash" or something slightly more > descriptive? We're certainly not indicating that we expect the current > process to crash... Too pedantic? "expectChildProcessCrash" is fine. (I had it called "expectPluginProcessCrash" before I realised I had to handle OOP content crashes too.) > @@ +564,5 @@ > > + * those crashes should be removed. > > + */ > > +SimpleTest.expectProcessCrash = function () { > > + if (parentRunner) { > > + parentRunner.expectProcessCrash(); > > You know, looking at a lot of this code, we should just fix the harness to > ensure that parentRunner is always set. (Doesn't have to happen here, > though.) Yeah, the constant checking is kind of ugly. > @@ +785,5 @@ > > window.onerror = function simpletestOnerror(errorMsg, url, lineNumber) { > > var funcIdentifier = "[SimpleTest/SimpleTest.js, window.onerror] "; > > > > // Log the message. > > + SimpleTest._logInfo(funcIdentifier, "An error occurred: " + errorMsg + " at " + url + ":" + lineNumber); > > You're changing the semantics here, is that really what you want? (I'm not > actually sure this worked in the first place, but just checking.) These weren't being reported properly in the first place, but I wasn't exactly sure why. I found if I left those as ok(false, ...) calls that I got a bunch of test failures. I *think* it was the JS strict warnings that led me to file bug 652494; I'll investigate why exactly this wasn't working before. > ::: testing/mochitest/tests/SimpleTest/TestRunner.js > @@ +63,5 @@ > > + Components.classes["@mozilla.org/file/directory_service;1"]. > > + getService(Components.interfaces.nsIProperties); > > + var crashDumpDir = directoryService.get("ProfD", Components.interfaces.nsIFile); > > + crashDumpDir.append("minidumps"); > > + return crashDumpDir; > > Can we avoid adding more enablePrivilege to the harness? We're in the (slow) > process of ripping it all out. It's probably a bit awkward here, admittedly, > but you can add whatever you want to SpecialPowers to make this happen: > https://developer.mozilla.org/en/SpecialPowers > > You'll probably have to move the whole observer bit into SpecialPowers, and > just provide methods like: > SpecialPowers.registerProcessCrashObservers(); > SpecialPowers.removeExpectedCrashDumpFiles(); OK. (High level methods like that are preferable to exposing notification observer registration and file deletion methods?) > @@ +289,5 @@ > > + } > > + TestRunner._expectedCrashDumpFiles.length = 0; > > +}; > > + > > +TestRunner.findUnexpectedCrashDumpFiles = function() { > > This is probably not useful as written. If a test has an unexpected plugin > crash, then you'll error here. However, you don't keep this list of files, > so you'll print the same error for later tests as well. This seems useful, > since it will cause us to explicitly fail tests that cause plugin/oop > content crashes, but you'll need to save the list of dump files you've > already seen to prevent repeating the error. The unexpected plugin crash dump files are stored in TestRunner._unexpectedCrashDumpFiles so that they don't get found again after the next test has finished. Is that what you mean?

(In reply to comment #24) > > You'll probably have to move the whole observer bit into SpecialPowers, and > > just provide methods like: > > SpecialPowers.registerProcessCrashObservers(); > > SpecialPowers.removeExpectedCrashDumpFiles(); > > OK. (High level methods like that are preferable to exposing notification > observer registration and file deletion methods?) Yeah, I think we might as well just keep all the complexity on the SpecialPowers side and make the API simple. > The unexpected plugin crash dump files are stored in > TestRunner._unexpectedCrashDumpFiles so that they don't get found again > after the next test has finished. Is that what you mean? You are totally correct and I misread that. Sorry. That should be fine.

BTW, thanks for taking this on, this is really useful work!

(In reply to comment #24) > These weren't being reported properly in the first place, but I wasn't > exactly sure why. I found if I left those as ok(false, ...) calls that I > got a bunch of test failures. I *think* it was the JS strict warnings that > led me to file bug 652494; I'll investigate why exactly this wasn't working > before. It looks like in mochitest-chrome and mochitest-browser-chrome that parentRunner in SimpleTest.js is null. I think this is due to the way the scope gets set up in mochitest/browser-test.js. AIUI inside SimpleTest.js's window.error, when ok(false, ...) is called from a chrome mochitest, we find that parentRunner is null and then we don't log anything for that uncaught JS exception. All usual calls to ok/is/etc. from inside the chrome mochitest actually go to the functions defined in browser-test.js rather than the ones in SimpleTest.js, which just use dump to write out the error. The interaction of browser-test.js and SimpleTest.js is confusing. It seems like we should make browser-test.js be the "parent runner" of individual tests, and for browser-test.js to expose its test reporting interface in the same way as TestRunner.js, rather than having it poke around into the SimpleTest.js to replace its functions. I think eventually we should fix chrome mochitests so that when the window.onerror handler is called that it does result in test failure messages being output, but given the number of them that currently exist (see http://tinderbox.mozilla.org/showlog.cgi?log=Try/1305517688.1305521120.8335.gz&fulltext=1), we should do this in a separate bug. For now, I will make window.onerror check whether there is a parentRunner and drop the error message, as we do currently.

Yeah, sounds like we can fix that in a separate bug. The Mochitest harnesses are kind of a mess. :-/

Filed bug 657485 for reporting uncaught JS exception is chrome mochitests.

Is there any work left here, or does this just need to land?

This can't land yet. I have patches that now use SpecialPowers, but per bug 539823 comment 20, they run into bug 608021, causing crashes on shutdown after a test run. We'll need to wait for that bug to be fixed. I will post my WIP patches for feedback here a bit later, though.

This cleanup patch isn't impacted by the bug 608021 dependency, so it is ready for review, with previous comments addressed.

I'd like feedback on this part, which has all of the crash detection moved to SpecialPowers. (Ignore the debugging dump() calls in there for now.) This works, but I'd like to know if my extension and use of SpecialPowers is correct -- particularly the addition of the SPUniqueIDService message so that the content-side message receiver knows that a given async message sent from chrome was destined for it, and not some other SpecialPowers object that might exist (this is what I brought up on dev-platform recently). The SPPingService/spinMessageLoop is used to get the message queue on the chrome side flushed, so that any crash notification that is sent to chrome has time to be sent on to the content process before the test finishes. The checks for `typeof SpecialPowers != "undefined"` are done so that we don't get JS errors for chrome mochitests, where the SpecialPowers object doesn't exist.

Comment on attachment 529669 [details] [diff] [review] Part 4: Make existing plugin crash mochitests clean up after themselves I'll have to update this one.

I got some feedback from smaug on dev-platform regarding chrome responding asynchronously to a particular content process' SpecialPowers object. That let me eliminate the SPUniqueIDService. I think this is ready for a proper review now. I'm just waiting on a final try run: http://tbpl.mozilla.org/?tree=Try&rev=8f0219e1d4ab

When SimpleTest.expectChildProcessCrash() is called and there aren't any crash dump files left behind, this also will cause a test failure. So I've moved those calls to after the "do we have OOP plugins, if not, skip the test" checks. The additional isnot() call in test_crash_submit.xul is left behind from some testing I was doing, but I think worth keeping in there. The plugin_focus_helper.html change is to remove the unnecessary loading of SimpleTest.js, since it's just a helper file.

Comment on attachment 538682 [details] [diff] [review] Part 1: Simplify mochitest logging, and other minor cleanups. (v2) Review of attachment 538682 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/specialpowers/content/specialpowers.js @@ +210,5 @@ > + try { > + Cc["@mozilla.org/preferences-service;1"] > + .getService(Ci.nsIPrefBranch) > + .setIntPref("mochitest.ipcmode", 0); > + return false; I think you just want to ask if nsIXULRuntime.processType != PROCESS_TYPE_DEFAULT: http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULRuntime.idl#90 (this check is just "am I in a content process", right?) ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +311,5 @@ > var domutils = targetWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor). > getInterface(Components.interfaces.nsIDOMWindowUtils); > > //TODO: make this support scenarios where we run test standalone and not inside of TestRunner only > if (parent && parent.ipcWaitForFocus != undefined) { Tweak this to just"&& parent.ipcWaitForFocus", while you're here?

Comment on attachment 538857 [details] [diff] [review] Part 2: Allow mochitests to clean up plugin and IPC process crash dumps. (v2) Review of attachment 538857 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks fine. ::: build/automationutils.py @@ +113,5 @@ > foundCrash = False > dumps = glob.glob(os.path.join(dumpDir, '*.dmp')) > for d in dumps: > log.info("PROCESS-CRASH | %s | application crashed (minidump found)", testName) > + print "Crash dump filename: " + d Is this just for debugging? ::: testing/mochitest/specialpowers/content/specialpowers.js @@ +279,5 @@ > + }); > + return crashDumpFiles; > + }, > + > + spinMessageLoop: function(aCallback) { I think jdm was looking to do something like this in bug 621363 to make prefs work reliably. Probably worth convening with him to talk about it. Unfortunate that it forces you into an asynchronous callback-based model, though. :-( ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +564,5 @@ > > +/** > + * Indicates to the test framework that the current test expects one or > + * more crashes (from plugins or IPC documents), and that the minidumps from > + * those crashes should be removed. API-wise, should we make this indicate the number of expected crashes, just in case things are extra crashy? Does that seem worthwhile?

jdm: see review comments above

Comment on attachment 538858 [details] [diff] [review] Part 3: Make existing plugin crash mochitests clean up after themselves. (v2) Review of attachment 538858 [details] [diff] [review]: -----------------------------------------------------------------

I'm not really sure why you're registering on-demand listeners like SPPingService - I don't see what the benefit is. Beyond that, the way spinMessageLoop is implemented is interesting, in that it doesn't actually do what the name says. It's really like executeSoon but with a guaranteed IPC round trip occurring before the callback function is run. However, I still don't understand why it's necessary, so please enlighten me?

(In reply to comment #38) > ::: testing/mochitest/specialpowers/content/specialpowers.js > @@ +210,5 @@ > > + try { > > + Cc["@mozilla.org/preferences-service;1"] > > + .getService(Ci.nsIPrefBranch) > > + .setIntPref("mochitest.ipcmode", 0); > > + return false; > > I think you just want to ask if nsIXULRuntime.processType != > PROCESS_TYPE_DEFAULT: > http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULRuntime. > idl#90 > > (this check is just "am I in a content process", right?) I believe that's what the check means, yeah; it attempts to set a pref, and this will fail if you're in a content process. (I just copied it over from TestRunner.js.) That call looks simpler, so I'll use that instead. > ::: testing/mochitest/tests/SimpleTest/SimpleTest.js > @@ +311,5 @@ > > var domutils = targetWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor). > > getInterface(Components.interfaces.nsIDOMWindowUtils); > > > > //TODO: make this support scenarios where we run test standalone and not inside of TestRunner only > > if (parent && parent.ipcWaitForFocus != undefined) { > > Tweak this to just"&& parent.ipcWaitForFocus", while you're here? OK. (In reply to comment #39) > ::: build/automationutils.py > @@ +113,5 @@ > > foundCrash = False > > dumps = glob.glob(os.path.join(dumpDir, '*.dmp')) > > for d in dumps: > > log.info("PROCESS-CRASH | %s | application crashed (minidump found)", testName) > > + print "Crash dump filename: " + d > > Is this just for debugging? I did include it initially for debugging, but I think it is useful to keep in there. When a test failure is reported because of unexpected crash dump files, the filenames are logged with a TEST-INFO. Showing the crash dump filename before running the minidump analyser at least lets you know which test's crash dump this is. > ::: testing/mochitest/specialpowers/content/specialpowers.js > @@ +279,5 @@ > > + }); > > + return crashDumpFiles; > > + }, > > + > > + spinMessageLoop: function(aCallback) { > > I think jdm was looking to do something like this in bug 621363 to make > prefs work reliably. Probably worth convening with him to talk about it. > Unfortunate that it forces you into an asynchronous callback-based model, > though. :-( Yeah, it's not a huge problem though. > ::: testing/mochitest/tests/SimpleTest/SimpleTest.js > @@ +564,5 @@ > > > > +/** > > + * Indicates to the test framework that the current test expects one or > > + * more crashes (from plugins or IPC documents), and that the minidumps from > > + * those crashes should be removed. > > API-wise, should we make this indicate the number of expected crashes, just > in case things are extra crashy? Does that seem worthwhile? That's a good idea, yeah, though it would need me to go through the tests and find out how many crashes are generated at the moment. Should I do that here or as part of a separate bug?

(In reply to comment #42) > I'm not really sure why you're registering on-demand listeners like > SPPingService - I don't see what the benefit is. Good point, since each test needs to use that, we may as well register it up front with the other message listeners. I'll change that. > Beyond that, the way > spinMessageLoop is implemented is interesting, in that it doesn't actually > do what the name says. It's really like executeSoon but with a guaranteed > IPC round trip occurring before the callback function is run. Yeah, maybe the name isn't accurate then. Maybe "flushIPCMessageQueue" or something. > However, I still don't understand why it's necessary, so please enlighten me? At the end of an individual test, we need to know whether any plugin process crashes occurred, and the notifications for those crashes get sent to the chrome process. At the time the content process calls TestRunner.finishTest(), the crash will definitely have happened, but the actual processing of the that notification message, dispatch of the IPC message to content, and the processing of that IPC message by content might not have happened yet. So we make TestRunner.finishTest() flush the IPC message queue so that content is guaranteed to have processed that crash message.

(In reply to comment #43) > > API-wise, should we make this indicate the number of expected crashes, just > > in case things are extra crashy? Does that seem worthwhile? > > That's a good idea, yeah, though it would need me to go through the tests > and find out how many crashes are generated at the moment. Should I do that > here or as part of a separate bug? Feel free to do it in a followup, you've done enough work here!

I am working on adding specialpowers to chrome and a11y, there is a check in: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#236 which will fail to be useful. What can we do to make this work as expected once I add specialpowers to mochitest-chrome?

actually, I find that when running chrome/dom/plugins/test/test_crash_submit.xul, I pass the 10 tests, but fail to generate a crash dump as per this error message: This test did not leave any crash dumps behind, but we were expecting some! if I do not allow this test to run (because it is designed not to in mochitest-chrome now) by hacking out the check in "TestRunner._expectingProcessCrash = true", my tests pass, but test_crash_submit.xul runs 0 tests.

Once you make that change, will SpecialPowers be available everywhere that TestRunner.js is used? If so, you can just remove that check. There are a few places in TestRunner.js that check `typeof SpecialPowers != "undefined"`, and they should all work once SpecialPowers is available (and you could then remove those checks, too).

with my current set of patches it will be everywhere but browser-chrome. I can look to see if browser chrome will work fine by adding in specialpowers and then we can clean up those if conditions.

So with the changes in this bug, unexpected exceptions in _all_ mochitests tests no longer cause the test to fail? Could we not restrict it to just chrome mochitests or something? This bit me today: a test that really should have been failing kept passing, until I realized that someone had completely broken the mochitest fail-on-unexpected-error behavior.... > These weren't being reported properly in the first place, but I wasn't exactly > sure why. "reported properly"? They triggered a test failure, which was the whole point of that code!

I filed bug 667155.

(In reply to comment #52) > So with the changes in this bug, unexpected exceptions in _all_ mochitests > tests no longer cause the test to fail? Before this change, uncaught exceptions caused test failures in all mochitests except for chrome mochitests. After the change, this was also the case for plain mochitests, erroneously. > > These weren't being reported properly in the first place, but I wasn't exactly > > sure why. > > "reported properly"? They triggered a test failure, which was the whole > point of that code! Only for chrome mochitests, which is what I was referring to.