539823 - Ignore minidumps when they are expected from a crash test

Status: RESOLVED DUPLICATE of bug 642175

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Followup to bug 539290.

The minidumps from the crash tests in mochitest-ipcplugins result in

PROCESS-CRASH | automation.py | application crashed (minidump found)

showing up in the log.  We want to somehow flag that these should be ignored.  This might be hard, I dunno.

Comment 1

[Benjamin Smedberg]

Yeah, it's hard because while we know which PIDs are expected to crash,
* that's in a different codepath of the automation
* the minidumps don't have PIDs associated with them

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #1)
> * the minidumps don't have PIDs associated with them

Any way we could do that without too much pain?

Comment 5

[(not currently active) Ted Mielczarek]

I've mentioned this elsewhere, but I think that tests that intentionally crash should be responsible for cleaning up their own minidumps.

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to comment #5)
> I've mentioned this elsewhere, but I think that tests that intentionally crash
> should be responsible for cleaning up their own minidumps.

What I meant here is that we should provide (in the harness) a method like expectCrash() that registers for the observer service notification, notes the minidump filename, and then deletes it when the test finishes running. Then every test that intentionally crashes would call expectCrash() once per expected crash.

Comment 10

[Cameron McCormack (:heycam)]

It looks like no observer notification is sent when a plugin hangs, as opposed to when it crashes.  Is that right?  Should we do so in PluginModuleParent::ShouldContinueFromReplyTimeout, and if so, should it be a "plugin-crashed" or something else?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Why do you want a notification on hangs?  (It can easily be done, but not sure how it relates to this bug.)

Comment 12

[(not currently active) Ted Mielczarek]

Presumably because we test hanging:
http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/mochitest/test_hanging.html?force=1

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We send out an observer-service notification after crashes from kill-when-hung.  The pluginDumpID and browserDumpID will be non-"", if everything went well.  Not sure what Cameron needs beyond that.

Comment 14

[Cameron McCormack (:heycam)]

Hmm.  I was finding that that a "plugin-crashed" notification was sent for test_hanging.html.  Is it that the KillProcess call should eventually lead the crash reporter to let us know the process died?

The Part 3 patch over in bug 642175 was my attempt to make it send the notification.

Comment 15

[Cameron McCormack (:heycam)]

*was not sent

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #14)
> Hmm.  I was finding that that a "plugin-crashed" notification was sent for
> test_hanging.html.  Is it that the KillProcess call should eventually lead the
> crash reporter to let us know the process died?

That's right.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Clean up minidumps from IPC crash test.

As mentioned in bug 642175, I was wrong about the notifications.  They are being sent OK.

This patch uses the SimpleTest.expectProcessCrash() function that'll be introduced by bug 642175 to make dom/ipc/tests/test_process_error.xul clean up its minidumps.

Comment 19

[Josh Matthews [:jdm]]

Cameron, status? I keep having to link people to my blog post; it would be really fantastic for this to land if possible.

Comment 20

[Cameron McCormack (:heycam)]

It's mostly done.  I believe I'm tickling bug 608021 with my extra use of IPC via SpecialPowers, which causes a crash in Linux opt builds, though.

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=e0065ad9ad00 is a tryserver run with a deliberate plugin crash in one of mochitest-2 that is not cleaned up (the SimpleTest.expectProcessCrash call is commented out).  But the oranges on M[1345] are all bug 608021.

(The Moth orange is me not yet cleaning up the deliberate crash in dom/ipc/tests/test_process_error.xul.  This is a chrome mochitest, and the machinery I've added for checking for unexpected crash dump files after each test run doesn't work on the chrome mochitest harness, since all of that has now moved over to SpecialPowers, which of course doesn't exist in chrome.  I'm making calling SimpleTest.expectProcessCrash from a chrome mochitest just throw an exception, and to make chrome mochitests reponsible for cleaning up their own crashes.)

Comment 21

[(not currently active) Ted Mielczarek]

jmaher has been wrestling with the Mochitest/Chrome Mochitest split while trying to remove enablePrivilege usage, maybe he has some hints.

Comment 22

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

currently we share a lot of harness/tool code between mochitest plain, chrome and browser-chrome.  We would like to continue to share as much code as possible, but special powers adds some interesting challenges.  We can run special powers just fine from chrome or browser-chrome.  The problem is we would need to edit a large percentage of the individual test case files.  

Currently I am working on moving tests from mochitest-plain->mochitest-chrome so that we can divide tools that are only used in chrome tests and not implement those in specialpowers.  Once this is done, we can convert the rest of the mochitest tools (eventutils.js, etc...) to special powers and let mochitest-chrome have access to special powers functions.

Comment 23

[Cameron McCormack (:heycam)]

Thanks jmaher.  Given that there's just this one outstanding chrome mochitest that doesn't clean up its own crash dump files, I'll just make it do this manually.  At some stage it would be good to make chrome mochitests realise if there are crash dump files left around that were unexpected, and to fail the test because of that, but status quo is fine for the moment I think.

Comment 24

[Cameron McCormack (:heycam)]

And by "realise" there I mean have the test that caused the plugin/content crash fail, rather than rely on the PROCESS-CRASH line right at the end of the test run cause the failure (where it's harder to determine what caused the crash).

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 529926 [details] [diff] [review]
Clean up minidumps from IPC crash test.

Also, in case it wasn't clear, the patch here is obsolete because it uses SimpleTest.expectProcessCrash() from bug 642175, which will only work for plain mochitests now, and this process_error.xul is a chrome mochitest.

I'll attach a patch soon just fixing this one test.  But anyway, this process_error.xul test is in mochitest-chrome, not mochitest-ipcplugins mentioned in comment 0; deliberate crashes from mochitest-ipcplugins will all be handled by bug 642175.  (I should probably have attached this now-obsolete patch to bug 647213, originally...)

Comment 26

[Cameron McCormack (:heycam)]

Bug 647213 handled process_error.xul.

All the remaining crash dump cleanup will be fixed with bug 642175, so I don't see much point in keeping this bug open too.  Duping to that one.