540967 - Windows: sporadic issue with mochitest-ipcplugins: "missing output line for total leaks"

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Phil Ringnalda (:philor)]

+++ This bug was initially created as a clone of Bug #539295 +++

Second verse, same as the first except with Windows this time:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264019073.1264020660.32626.gz#err6
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/20 12:24:33
s: win32-slave18

PROCESS-CRASH | automation.py | application crashed (minidump found)
Thread 2 (crashed)
PROCESS-CRASH | automation.py | application crashed (minidump found)
Thread 2 (crashed)
PROCESS-CRASH | automation.py | application crashed (minidump found)
Thread 2 (crashed)
TEST-UNEXPECTED-FAIL | plugin process 3056 | automationutils.processLeakLog() | missing output line for total leaks!
TEST-UNEXPECTED-FAIL | plugin process 3056 | automationutils.processLeakLog() | missing output line for total leaks!

Comment 1

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264083500.1264084951.9731.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/21 06:18:20
s: win32-slave15

Comment 2

[Benjamin Smedberg]

Attachment: Wait for processes on Windows too, rev. 1

Comment 3

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

Comment on attachment 422825 [details] [diff] [review]
Wait for processes on Windows too, rev. 1

>diff --git a/ipc/chromium/src/chrome/common/process_watcher_win.cc b/ipc/chromium/src/chrome/common/process_watcher_win.cc
>--- a/ipc/chromium/src/chrome/common/process_watcher_win.cc
>+++ b/ipc/chromium/src/chrome/common/process_watcher_win.cc
>-void ProcessWatcher::EnsureProcessTerminated(base::ProcessHandle process) {
>+void ProcessWatcher::EnsureProcessTerminated(base::ProcessHandle process
>+#ifdef CHROMIUM_MOZILLA_BUILD
>+					     , bool force
>+#endif
>+) {
>   DCHECK(process != GetCurrentProcess());
> 
>+#ifdef CHROMIUM_MOZILLA_BUILD
>+  if (!force) {
>+    WaitForSingleObject(process, INFINITE);
>+    return;
>+  }
>+#endif
>+

bent points out that you need a |CloseHandle(process)| here.  r+ with that fix.

There's a less important, more general question of how this code should behave.  It runs on the IO thread, so blocking syscalls choke off IPC for their duration.  The POSIX code only does an infinite-blocking-wait upon "MessageLoop destruction" (i.e. IO thread exit), whereas this code infinite-blocking-waits immediate on ~GeckoChildProcessHost().  Arguments can be made for both; I'm r+ing because for unit tests et al. I don't think it really matters.  However, for developer builds, I prefer the POSIX code's behavior, because hung child processes don't immediately wedge IPC in the parent.  Certainly something for a followup bug, if that.

Comment 4

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264120630.1264122636.21432.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/21 16:37:10
s: win32-slave06

Comment 5

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264206458.1264208394.13167.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/22 16:27:38
s: win32-slave16

Comment 6

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264215631.1264217356.16225.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/22 19:00:31
s: win32-slave23

Comment 7

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264218712.1264220146.14730.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/22 19:51:52
s: win32-slave34

Comment 8

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264356525.1264358196.5576.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/24 10:08:45
s: win32-slave43

Comment 9

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264336403.1264338465.32619.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/01/24 04:33:23
s: win32-slave12

Comment 10

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/38e86af96757