Closed Bug 794984 Opened 12 years ago Closed 10 years ago

[mozprocess] mozprocess should not always cross the streams (redirect stderr to stdout)

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: gps, Assigned: parkouss)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Add ability to separate stderr from stdout
24.59 KB, patch
ahal
: review+
Details | Diff | Splinter Review 
class ProcessHandlerMixin(object):
    def run(self, timeout=None, outputTimeout=None):
        self.didTimeout = False
        self.startTime = datetime.now()
        self.proc = self.Process(self.cmd,
                                 stdout=subprocess.PIPE,
                                 stderr=subprocess.STDOUT,
                                 cwd=self.cwd,
                                 env=self.env,
                                 ignore_children = self._ignore_children,
                                 **self.keywordargs)

        self.processOutput(timeout=timeout, outputTimeout=outputTimeout)

Always crossing the streams and redirecting stderr to stdout is not always what's desired. Could mozprocess gain the ability to maintain separate output buffers for stderr and stdout?
So, not surprisingly, this is now leading to bad things: bug 839275 . I'd want to mark this as blocking that, but would want to make sure before making that call.  In any case, we should probably fix this ASAP. I'll take it when I can (read: don't be surprised if its a few weeks) or free beer or beverage of your choice if you beat me to it.
See Also: → 839275
Blocks: 881421
Summary: mozprocess should not always cross the streams (redirect stderr to stdout) → [mozprocess] mozprocess should not always cross the streams (redirect stderr to stdout)
Hi,

I could be interested to work on this. There is my proposal:

 - Read each process channel in a thread, like described here (multiplatform, non-blocking): http://stackoverflow.com/questions/375427/non-blocking-read-on-a-subprocess-pipe-in-python
 - In another thread, get lines, call callbacks - manage the lifecycle of the reading threads

This must be noted that it will work best for line-based output (I think it the case here anyway). Also, there will be at least two threads for reading, and three when stderr is also required without automatic merge of the stream (done by subprocess module).

The fact that there is one thread for callbacks may be good for bug 875508 if I understand well.

What do you think ? May I work on this with this proposal ?
I started to implement it, and I would like to have your feedback on this. :)

Also, I pushed to try on some machines, and so far it is green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d3097a9bdc0
Attachment #8533284 - Flags: feedback?(k0scist)
Comment on attachment 8533284 [details] [diff] [review]
mozprocess should not always cross the streams

Review of attachment 8533284 [details] [diff] [review]:
-----------------------------------------------------------------

I no longer work at Mozilla or am active with the project, but the basic approach here looks sane.
Attachment #8533284 - Flags: feedback?(k0scist) → feedback+
(In reply to Jeff Hammel from comment #4)
> Comment on attachment 8533284 [details] [diff] [review]
> mozprocess should not always cross the streams
> 
> Review of attachment 8533284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I no longer work at Mozilla or am active with the project, but the basic
> approach here looks sane.

Ok, thanks for the answer Jeff!
I don't know who to ask for a feedback/review for this, William, can you help me ?
Flags: needinfo?(wlachance)
(In reply to Julien Pagès from comment #6)
> I don't know who to ask for a feedback/review for this, William, can you
> help me ?

Hmm, good question. I will try pinging a few people and get back to you.
Flags: needinfo?(wlachance)
Comment on attachment 8533284 [details] [diff] [review]
mozprocess should not always cross the streams

Review of attachment 8533284 [details] [diff] [review]:
-----------------------------------------------------------------

ahal's going to look at this.
Attachment #8533284 - Flags: review?(ahalberstadt)
Comment on attachment 8533284 [details] [diff] [review]
mozprocess should not always cross the streams

Review of attachment 8533284 [details] [diff] [review]:
-----------------------------------------------------------------

First, thanks for this patch, it is a really good step in the right direction! It has better separation of concerns and makes better use of threads than the old version (it even fixes bug 875508 as a side effect!).

That being said, this is also a large change to a very heavily used library, both inside and outside the tree. I see it passes the mozbase tests on try, but mozprocess is also used in some way or another by every one of our test jobs. So I'd like to see a run with the syntax `try: -a` first. We can't be too careful when it comes to testing mozprocess changes.

There are a few implementation details I'd like to switch around, and I'm tempted to say let's break backwards compatibility in favour of a cleaner API, but I won't say that yet :). Thanks for sticking with it!

::: testing/mozbase/mozprocess/mozprocess/processhandler.py
@@ +42,5 @@
>      :param kill_on_timeout: when True, the process will be killed when a timeout is reached. When False, the caller is responsible for killing the process. Failure to do so could cause a call to wait() to hang indefinitely. (Defaults to True.)
> +    :param processOutputLine: function or list of functions to be called for
> +        each line of output produced by the process (defaults to an empty
> +        list).
> +    :param processStdoutOutputLine:function or list of functions to be called

nit: no space after the ':'

@@ +45,5 @@
> +        list).
> +    :param processStdoutOutputLine:function or list of functions to be called
> +        for each line of standard output - stdout - produced by the process
> +        (defaults to an empty list).
> +    :param processStderrOutputLine:function or list of functions to be called

Same as above.

@@ +602,5 @@
>                   ignore_children = False,
>                   kill_on_timeout = True,
>                   processOutputLine=(),
> +                 processStdoutOutputLine=(),
> +                 processStderrOutputLine=(),

I think cases where people strictly want stdout and not stderr will be rare (if I'm wrong we can revisit this assumption later). So please remove 'processStdoutOutputLine'. The rule can be 'if processStderrOutputLine, then separate the streams, else redirect stderr to stdout'.

I'm not crazy about this new API (or the old one for that matter), but any solution I think of that also maintains backwards compatibility is equally confusing. I'm kind of sort of tempted towards breaking backwards compatibility, but I'd need to see how hard updating everything would be. I'm going to think about it a bit, I may ask you to change it again in a later review :)

@@ +678,5 @@
> +
> +        if self._separate_stderr:
> +            stderr = subprocess.PIPE
> +        else:
> +            stderr = subprocess.STDOUT

Instead of self._separate_stderr, just set self._stderr directly above.

@@ -832,5 @@
>                               "use ProcessHandler.wait() instead"
>          return self.wait(timeout=timeout)
>  
> -
> -    ### Private methods from here on down. Thar be dragons.

Where did all this code go? Is it replaced by _read_stream? If so I highly doubt that's going to work, at least probably not in windows. Mozprocess is the result of fixing years worth of edge cases, so a green try run isn't enough to make me feel confident that we aren't regressing something.

That being said, it would be really amazing if all this actually *was* unnecessary...

@@ +811,5 @@
> +    def __add__(self, lst):
> +        return CallableList(list.__add__(self, lst))
> +
> +class ProcessReader(object):
> +    def __init__(self, stdout_callback=None, stderr_callback=None,

I like that you pulled this logic into another class, good separation of concerns. I'd suggest making this class inherit from threading.Thread though and only being tied to a single stream. Then the processhandler object would have two readers, stdout_reader and stderr_reader (both of which could be None). This would follow the single responsibility principle a bit better.

That would mean a total of 4 threads if both are specified. 1 for pulling lines off the pipe + 1 for executing callbacks x 2 streams. Side note, this also fixes bug 875508, nice! :)

@@ +819,5 @@
> +        self.stderr_callback = stderr_callback or (lambda line: True)
> +        self.finished_callback = finished_callback or (lambda: True)
> +        self.timeout_callback = timeout_callback or (lambda: True)
> +        self.timeout = timeout
> +        self.timeout_line = timeout_line

timeout_line is confusing, please call it output_timeout for consistency

@@ +832,5 @@
> +        return thread
> +
> +    def _read_stream(self, stream, queue, callback):
> +        for line in iter(stream.readline, b''):
> +            queue.put((callback, line))

nit: (line, callback) feels more natural to me

@@ +836,5 @@
> +            queue.put((callback, line))
> +        stream.close()
> +
> +    def start(self, proc):
> +        start_time = time.time()

I appreciate you trying to be as accurate as possible, but I don't think keeping track of the start_time here is all that necessary. Just calling time.time() in _read is good enough.

@@ +844,5 @@
> +                                                       proc.stdout,
> +                                                       queue,
> +                                                       self.stdout_callback)
> +        else:
> +            stdout_reader = None

nit: drop the else clause and set stdout_reader = None above the if statement.

@@ +851,5 @@
> +                                                       proc.stderr,
> +                                                       queue,
> +                                                       self.stderr_callback)
> +        else:
> +            stderr_reader = None

Same as above.

@@ +852,5 @@
> +                                                       queue,
> +                                                       self.stderr_callback)
> +        else:
> +            stderr_reader = None
> +        self._thread_handler = threading.Thread(name='ProcessReader',

I wonder if we should have two threads for processing the callbacks. I guess the common case is that people will expect lines to get processed in order, so this is probably fine.

::: testing/mozbase/mozprocess/tests/manifest.ini
@@ +15,5 @@
>  [test_mozprocess_wait.py]
>  [test_mozprocess_output.py]
>  [test_mozprocess_params.py]
> +[test_process_reader.py]
> +skip-if = false

nit: remove this skip-if line

::: testing/mozbase/mozprocess/tests/test_process_reader.py
@@ +6,5 @@
> +def run_python(str_code, stdout=subprocess.PIPE, stderr=subprocess.PIPE):
> +    cmd = [sys.executable, '-c', str_code]
> +    return subprocess.Popen(cmd, stdout=stdout, stderr=stderr)
> +
> +class TestProcessReader(unittest.TestCase):

Thanks for the test!
Attachment #8533284 - Flags: review?(ahalberstadt) → review-
(In reply to Andrew Halberstadt [:ahal] from comment #9)
> @@ +811,5 @@
> > +    def __add__(self, lst):
> > +        return CallableList(list.__add__(self, lst))
> > +
> > +class ProcessReader(object):
> > +    def __init__(self, stdout_callback=None, stderr_callback=None,
> 
> I like that you pulled this logic into another class, good separation of
> concerns. I'd suggest making this class inherit from threading.Thread though
> and only being tied to a single stream. Then the processhandler object would
> have two readers, stdout_reader and stderr_reader (both of which could be
> None). This would follow the single responsibility principle a bit better.
> 
> That would mean a total of 4 threads if both are specified. 1 for pulling
> lines off the pipe + 1 for executing callbacks x 2 streams. Side note, this
> also fixes bug 875508, nice! :)

Oh crap sorry, please disregard this comment, I meant to delete it! I changed my mind after writing this and realized your implementation is a bit cleaner.
Thanks for the review and the encouraging comments!

> That being said, this is also a large change to a very heavily used library,
> both inside and outside the tree. I see it passes the mozbase tests on try,
> but mozprocess is also used in some way or another by every one of our test
> jobs. So I'd like to see a run with the syntax `try: -a` first. We can't be
> too careful when it comes to testing mozprocess changes.

Yes, sure.

> There are a few implementation details I'd like to switch around, and I'm
> tempted to say let's break backwards compatibility in favour of a cleaner
> API, but I won't say that yet :). Thanks for sticking with it!

I thought the same way. :) I tried to keep compatibility and at the same time clean it a bit.

> Where did all this code go? Is it replaced by _read_stream? If so I highly
> doubt that's going to work, at least probably not in windows. Mozprocess is
> the result of fixing years worth of edge cases, so a green try run isn't
> enough to make me feel confident that we aren't regressing something.

I agree with you. the fact is that I do not really understand all the code
on the windows part, but I'm pretty confident. One thing I know for sure is that
_normalize_newline is not needed - there is a test case for that that passed.
Also I am thinking that all this code was to read in a synchronous way, and be able
to stop it at some time (when timeout is reached). Now that these functionalities are
separated in threads (read will continue to be done in any case - except on a process kill -
but callbacks won't fire to indicate it without blocking) I think that it could just work.

I will fix the nits and improvements, then launch a "try -a" and will come back to you.
Hi Andrew,

I think that the push try is finished. Not sure how to read this:

 425 success 4 failed 9 busted 0 exception 0 retry 1 running 0 pending 0 coalesced 

(the 1 running is probably blocked somewhere, but I can not stop it from the ui. It indicates:
1302 mins overdue, typically takes ~ 64 mins)

I leave the analyse to you, tell me if I can be of any help.
Flags: needinfo?(ahalberstadt)
I think you're ok, I did some retriggers to be sure. In the meantime, please upload the next patch and we'll go through the next review iteration.
Flags: needinfo?(ahalberstadt)
So, there it is. Note that I did not apply your last *nit*

diff --git a/testing/mozbase/mozprocess/tests/manifest.ini b/testing/mozbase/mozprocess/tests/manifest.ini
--- a/testing/mozbase/mozprocess/tests/manifest.ini
+++ b/testing/mozbase/mozprocess/tests/manifest.ini
@@ -10,8 +10,10 @@ disabled = bug 877864
 [test_mozprocess_kill.py]
 [test_mozprocess_kill_broad_wait.py]
 disabled = bug 921632
 [test_mozprocess_misc.py]
 [test_mozprocess_poll.py]
 [test_mozprocess_wait.py]
 [test_mozprocess_output.py]
 [test_mozprocess_params.py]
+[test_process_reader.py]
+skip-if = false

because I think it is not a nit, given that we have;

# does not currently work on windows
# see https://bugzilla.mozilla.org/show_bug.cgi?id=790765#c51

[DEFAULT]
# bug https://bugzilla.mozilla.org/show_bug.cgi?id=778267#c26
skip-if = (os == "win")

at the beginning of the manifest.ini file. Please tell me if I am wrong and I will update it then.
Assignee: nobody → j.parkouss
Attachment #8533284 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8536576 - Flags: review?(ahalberstadt)
Comment on attachment 8536576 [details] [diff] [review]
mozprocess should not always cross the streams

Review of attachment 8536576 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is almost there! I just have a few minor comments left, so please fix those and I think next time we'll be good to go.

::: testing/mozbase/mozprocess/mozprocess/processhandler.py
@@ +42,5 @@
>      :param kill_on_timeout: when True, the process will be killed when a timeout is reached. When False, the caller is responsible for killing the process. Failure to do so could cause a call to wait() to hang indefinitely. (Defaults to True.)
> +    :param processOutputLine: function or list of functions to be called for
> +        each line of output produced by the process (defaults to an empty
> +        list).
> +    :param processStderrOutputLine: function or list of functions to be called

This parameter is a mouthful, maybe rename this to processStderrLine? Also we should mention that if it is not specified stderr will be sent to the processOutputLine handlers, and if it is specified stderr won't be sent there.

@@ +776,1 @@
>              # Thread.join() blocks the main thread until outThread is finished

nit: update this comment, outThread no longer exists

@@ +894,5 @@
> +            self.finished_callback()
> +
> +    @property
> +    def thread(self):
> +        return self._thread_handler

Why bother with a self._thread_handler and a thread @property? Just get rid of of this and rename self._thread_handler to self.thread.

@@ +898,5 @@
> +        return self._thread_handler
> +
> +    def join(self):
> +        if self._thread_handler:
> +            self._thread_handler.join()

This function doesn't seem to be used anywhere.

@@ +902,5 @@
> +            self._thread_handler.join()
> +
> +    def is_alive(self):
> +        if self._thread_handler:
> +            return self._thread_handler.is_alive()

This should probably return False instead of None in the event that the if statement is false.

::: testing/mozbase/mozprocess/tests/manifest.ini
@@ +15,5 @@
>  [test_mozprocess_wait.py]
>  [test_mozprocess_output.py]
>  [test_mozprocess_params.py]
> +[test_process_reader.py]
> +skip-if = false

Please remove the skip-if = false, it doesn't do anything.

::: testing/mozbase/mozprocess/tests/test_process_reader.py
@@ +6,5 @@
> +def run_python(str_code, stdout=subprocess.PIPE, stderr=subprocess.PIPE):
> +    cmd = [sys.executable, '-c', str_code]
> +    return subprocess.Popen(cmd, stdout=stdout, stderr=stderr)
> +
> +class TestProcessReader(unittest.TestCase):

I think some tests in here likely overlap with some of the existing ones, but I guess since this isn't using proclaunch, it doesn't hurt to have some overlap with the two different methods. P.s I like this way better :)
Attachment #8536576 - Flags: review?(ahalberstadt) → review-
Good review :)
Attachment #8536576 - Attachment is obsolete: true
Attachment #8537456 - Flags: review?(ahalberstadt)
So just to be sure I did not break anything when fixing the patch, I ran another push try (not a "try: -a", it seems expensive):

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=03ef7b10602d
Comment on attachment 8537456 [details] [diff] [review]
mozprocess should not always cross the streams

Review of attachment 8537456 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks great! I want to do some local testing with mochitests first (e.g force crashes, timeouts, etc) just to be safe before flagging checkin-needed though.
Attachment #8537456 - Flags: review?(ahalberstadt) → review+
I tried killing the browser mid-run, causing timeouts etc, and everything seems to work fine. I'm still a little bit worried about windows, but your try: -a run didn't have any problems, so I went ahead and pushed (with a more verbose commit message):
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a15054f99f
Backed out for "causing" Mulet mochitest-5 crashes. The crashes hit in 3/19 runs on this push and had 25 green on the previous push.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4775749&repo=mozilla-inbound

I also suspect the below mochitest-dt oranges to be from this patch, but those retriggers are still running. I'll report back in here when I can say for sure.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4770043&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a6a1984946
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> I also suspect the below mochitest-dt oranges to be from this patch, but
> those retriggers are still running. I'll report back in here when I can say
> for sure.

Confirmed.
Good news and bad news. The Mulet crash is still sticking around post-backout, so you're off the hook for that one. Bad news - the devtools failures appear to have gone away post-backout, so you're still on the hook for those.
Is somebody looking into re-landing this?
Flags: needinfo?(j.parkouss)
Flags: needinfo?(ahalberstadt)
Hi,

I'm not - not really sure about what to do next. But if I can be of any help, please tell me. :)
Flags: needinfo?(j.parkouss)
(In reply to Chris Manchester [:chmanchester] from comment #24)
> Is somebody looking into re-landing this?

Yeah, I definitely don't want to see this languish :). Just haven't had time yet due to holidays etc..

(In reply to Julien Pagès from comment #25)
> Hi,
> 
> I'm not - not really sure about what to do next. But if I can be of any
> help, please tell me. :)

Trying to reproduce locally would be a good next step. Looks like all the failures happen in devtools/netmonitor, so the following command should run them:
./mach mochitest -f plain --e10s browser/devtools/netmonitor/test

Also wouldn't hurt to double check that it is actually this patch that caused them and not some freak coincidence.. here's a new try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59ae915bfe49

(I think it'll run what we want.. try syntax seems to be somewhat of a crap shoot these days)
Flags: needinfo?(ahalberstadt)
I think the command line is actually:
./mach mochitest -f browser-chrome --e10s browser/devtools/netmonitor/test

But I'm having problems running specific directories of browser-chrome tests (I can run all of them though).
Ah, the 'mochitest' command doesn't support devtools. You need to run:
./mach mochitest-devtools --e10s browser/devtools/netmonitor/test

However (predictably) I can't reproduce this locally. Wondering if this is linux specific (vs linux64). Here's a new try run with both linux and linux64 on debug and opt:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3813e0279a9

I'm not too confident I'll be able to figure this out.
As you supposed, it broke on Linux opt, and so far is good on the Linux x64 opt. (I saw that on your treeherder link). I can have a look tomorrow, trying to reproduce it on my 32 bit ubuntu at work.

Do you know which python version is installed on this Linux opt ? Maybe it is related.

I'm not really confident too to figure this out. :(
Also locally it runs well for me too (./mach mochitest-devtools --e10s browser/devtools/netmonitor/test) on my archlinux 64 (well I have two errors, but with and without the patch so it is not related).

uname -a:
Linux parkouss 3.18.2-2-ARCH #1 SMP PREEMPT Fri Jan 9 07:37:51 CET 2015 x86_64 GNU/Linux

python2 -V
Python 2.7.9
Looks like this happens on Linux64 too, just not as frequently. Since this patch should theoretically speed up mozprocess, my theory is that it is simply exposing a pre-existing race condition, either in those tests or in the mochitest harness somewhere.

The good news is that it seems highly reproducible on try. What we'll want to do now is look at the tests/harness, try to guess the problem and push fixes to try. I'll try to look into this when I have some spare time, though if you feel up to it Julien, feel free to ping me if you get stuck/need help.
Here's a new push that turns on debugger logging and calls SimpleTest.requestCompleteLog():
https://treeherder.mozilla.org/#/jobs?repo=try&revision=078067dacd1c

Of note are lots of "unsafe usage of CPOW" warnings and eventually this:
"JavaScript error: , line 0: Error: operation not possible on dead CPOW"

Also weird is the fact that there are tons of TEST-PASS logs that contain messages like:
"The tooltip status is incorrect."

I'll file a devtools bug, maybe someone there can help us understand what is happening.
Depends on: 1124330
Hello Andrew,

I may have found the issue here. :)

I played some more with this code (in a really different context, python 3 and ssh) and find an occasionnal deadlock caused by this strange line:

+for line in iter(stream.readline, b''):

I must admit that I have originally copy pasted this from the link mentionned in comment 2. I must also admit that I do not uderstand what this code does;;; (shame on me to have put something I did not understood!)

Anyway, I replaced this strange code with something simpler:

+for line in stream:

And I got no deadlock or strange behaviour now (for my personnal use case). So I re-ran two trys, and the two runs ran fine!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a64c2a930d7c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9cdbe27132

(the build in the first one failed because I forgot to add the test file to the patch, this is not related)

Maybe you would like to try the patch a bit more ? I'm pretty confident that this will work now.
Flags: needinfo?(ahalberstadt)
Awesome, thanks for digging into this!

(In reply to Julien Pagès from comment #33)
> I must admit that I have originally copy pasted this from the link
> mentionned in comment 2. I must also admit that I do not uderstand what this
> code does;;; (shame on me to have put something I did not understood!)

It's ok, I'll admit I don't understand what that line was doing either. Mozprocess is kind of scary because each line of code handles subtle edge cases that aren't easy to understand. We got there through lots of trial and error over many years. This patch cleans mozprocess up a lot, so if we can get it landed, it will make future changes much easier :)

> Anyway, I replaced this strange code with something simpler:
> 
> +for line in stream:

Is this the only change? If not can you upload a new patch? I just want to make sure I test exactly what will be landed.

> Maybe you would like to try the patch a bit more ? I'm pretty confident that
> this will work now.

Sounds good, please upload a new patch. I want to run it through another full try run, something else could have changed that might cause a regression somewhere else. Hopefully not, but it's better to know ahead of time before being backed out.
Flags: needinfo?(ahalberstadt) → needinfo?(j.parkouss)
> > +for line in stream:
> 
> Is this the only change? If not can you upload a new patch? I just want to
> make sure I test exactly what will be landed.

Yeah, this is the only change. I send you a new patch so you can try this more. I hope it will works!
Attachment #8537456 - Attachment is obsolete: true
Flags: needinfo?(j.parkouss)
Attachment #8567936 - Flags: review?(ahalberstadt)
Comment on attachment 8567936 [details] [diff] [review]
Add ability to separate stderr from stdout

Thanks, here's a try: -a:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb2f5a2102f7

If all goes well, I'll land it tomorrow.
Attachment #8567936 - Flags: review?(ahalberstadt) → review+
I think we're good to go here. The M-oth failures were a little worrisome, but RyanVM said we were unlucky and pushed on top of a bad parent that has since been backed out. The only other failure I couldn't identify was that Wr one, but there are similar bugs on file and re-triggers came back green. I'm inclined to believe it is also unrelated.
https://hg.mozilla.org/mozilla-central/rev/9c40ffbcf6b3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
This seems to break "mach mochitest" with the --debugger argument.  At least
the way I am using it.  At 230777:c49df0e00b59, this worked the same as it
always did.  At 230778:9c40ffbcf6b3 (this commit), Valgrind output -- that
is, output from the --debugger= program -- does not appear incrementally
as it used to.  It does appear when I do control-C, which suggests some change
in buffering or some such (change in streams).

FWIW, the command I am running is

DISPLAY=:1.0 G_SLICE=always-malloc MOZ_DISABLE_NONLOCAL_CONNECTIONS=0 /
  ./mach mochitest-plain --debugger=/home/sewardj/VgTRUNK/mozhx/Inst/bin/valgrind /
  --debugger-args="--fair-sched=yes --smc-check=all-non-file /
    --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc.supp --error-limit=no /
    --trace-children=yes --child-silent-after-fork=yes  /
    --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java /
    --num-transtab-sectors=24 --tool=memcheck --freelist-vol=500000000 /
    --redzone-size=256 --gen-suppressions=no --px-default=allregs-at-mem-access /
    --px-file-backed=unwindregs-at-mem-access --stats=yes --partial-loads-ok=yes /
    --show-mismatched-frees=no --read-inline-info=yes --fullpath-after=-CURR/ /
    --num-callers=32 --track-origins=no --show-leak-kinds=definite" /
  dom/canvas/test/webgl-mochitest/test_hidden_alpha.html
(In reply to Julian Seward [:jseward] from comment #40)
> It does appear when I do control-C, which suggests some change
> in buffering or some such (change in streams).

What platform is this on? Could you paste the full command line you used?
Flags: needinfo?(jseward)
Oh heh, not sure how I glossed over the command there :)
Flags: needinfo?(jseward)
I'm not able to reproduce on linux. I see valgrind output with the command:
./mach mochitest-plain --debugger=valgrind

I'll file a new bug, we can take the conversation there.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: