1231981 - Test TURN server for mochitest in CI

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Byron Campen [:bwc]]

Building on bug 1231977 (probably), we need to build a test TURN server.

Comment 1

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review commit: https://reviewboard.mozilla.org/r/36405/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36405/

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/1-2/

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review33103

::: testing/tools/iceServer/iceServer.py:49
(Diff revision 2)
> +        result = (result << 8) + byte

Not sure if we need to worry about overflows on 32bit Python here?
If I understand it correct that could be prevented by explicitly making |result| of type long.

::: testing/tools/iceServer/iceServer.py:62
(Diff revision 2)
> +    lastBit = startBit - numBits + 1

What happens if lastBit turns negative, because numBits > (startBit +1)?

::: testing/tools/iceServer/iceServer.py:97
(Diff revision 2)
> +        length = parseInt(buf[2:4])

Not sure, but it might be helpful to store the parse length in the instance for future reference.

::: testing/tools/iceServer/iceServer.py:102
(Diff revision 2)
> +        self.data = buf[self.__attrHeaderSize:self.__attrHeaderSize+length]
> +
> +        while length % 4:
> +            length += 1 # ignore padding

Not sure I like the idea that the parser on purpose ignores remaining bytes in the buffer beyond the current attribute.

::: testing/tools/iceServer/iceServer.py:104
(Diff revision 2)
> +        while length % 4:
> +            length += 1 # ignore padding

In a strict mode we should raise an exception if we find something else then zero's in the padding area.

::: testing/tools/iceServer/iceServer.py:115
(Diff revision 2)
> +        self.cookie = 0x2112A442

The magic cookie value should probably go up into the list of global constants, or is there any scenario where a StunMessage instance modifies this value?

::: testing/tools/iceServer/iceServer.py:144
(Diff revision 2)
> +        buf = buf[0:minBufSize]

Are you ignoring trailing bytes here on purpose?
Depending on the type of testing a strict mode which complains or rejects these kind of problems might be helpful.

::: testing/tools/iceServer/iceServer.py:219
(Diff revision 2)
> +            return None

This should raise an Exception. Otherwise you just get strange errors when trying to serializeInt(None).

::: testing/tools/iceServer/iceServer.py:233
(Diff revision 2)
> +            return

Exception?

::: testing/tools/iceServer/iceServer.py:266
(Diff revision 2)
> +            return None

Exception please

::: testing/tools/iceServer/iceServer.py:335
(Diff revision 2)
> +class UdpNetworkHandler(NetworkHandler):

Just curious: are we limited in using third party libraries when using Python code in CI, or what else motivated you to avoid using something like twisted for the networking parts?

::: testing/tools/iceServer/iceServer.py:407
(Diff revision 2)
> +        UdpNetworkHandler.__init__(self, makeUdpSocket((v4Address, 0), socket.AF_INET))

Is this on purpose limited to v4?

::: testing/tools/iceServer/iceServer.py:554
(Diff revision 2)
> +# TODO: Firefox isn't setting up permissions for all the stuff it sends
> +# indications to. Looking at the spec, this is a bug

Is this "just" the fact that we send only a one shot permissions request right before the data indication, or something else?

::: testing/tools/iceServer/iceServer.py:581
(Diff revision 2)
> +        response.addNonce("foobajoobaaa")

Can we make this random instead?

::: testing/tools/iceServer/iceServer.py:623
(Diff revision 2)
> +turnUser="foo"
> +turnPass="bar"
> +turnRealm="mozilla.invalid"

I guess we make this later configurable

::: testing/tools/iceServer/iceServer.py:639
(Diff revision 2)
> +    dummySocket = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)

Not sure if we have to worry about hosts in CI w/o IPv6 setup?

::: testing/tools/iceServer/iceServer.py:651
(Diff revision 2)
> +    print ("iceServers:[{username:\"" + turnUser + "\",credential:\"" + turnPass + "\",url:\"turn:localhost\"}," +

How about announcing as a STUN server as well?
I think once this lands we should test with all possible combinations.

::: testing/tools/iceServer/iceServer.py:654
(Diff revision 2)
> +    while True:

This doesn't strike me as the easiest way to get later extended with different behavior on different port numbers. But we can probably re-factor that later.

::: testing/tools/iceServer/iceServer.py:655
(Diff revision 2)
> +        read, write, excpt = select.select(servers, filter(NetworkHandler.hasWrites, servers), servers, 0.1)

select? Yikes. Well I guess this is not going to be high performance, high throughput any way...

Comment 4

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/36405/#review33103

> Not sure if we need to worry about overflows on 32bit Python here?
> If I understand it correct that could be prevented by explicitly making |result| of type long.

From what I read, Python automatically converts to long as needed. Empirically, I have seen no problem with unpacking of transaction ids.

> What happens if lastBit turns negative, because numBits > (startBit +1)?

I can validate this.

> Not sure I like the idea that the parser on purpose ignores remaining bytes in the buffer beyond the current attribute.

We could run into problems verifying message integrity or fingerprint if something padded with something other than zeroes. But, firefox doesn't do this, so we should be ok.

> In a strict mode we should raise an exception if we find something else then zero's in the padding area.

Sure, why not. That'll guard against the problem I just described.

> The magic cookie value should probably go up into the list of global constants, or is there any scenario where a StunMessage instance modifies this value?

I'll make it a constant.

> Are you ignoring trailing bytes here on purpose?
> Depending on the type of testing a strict mode which complains or rejects these kind of problems might be helpful.

Yes, because this might have come over TCP.

> This should raise an Exception. Otherwise you just get strange errors when trying to serializeInt(None).

Sure.

> Exception?

Sure.

> Exception please

Sure.

> Just curious: are we limited in using third party libraries when using Python code in CI, or what else motivated you to avoid using something like twisted for the networking parts?

I blew a half-hour trying to figure out how to get the source ip/port out of Twisted, with no luck. SocketServer was a little better, but made it very difficult to get send buffering to work at all, and even without that I had to do a lot of monkeying around in the internals/overriding stuff/using the UDPServer to handle TCP traffic. Once everything was done, I decided it was ugly enough to abandon.

> Is this on purpose limited to v4?

As there is no way to request a V6 allocation in the base spec, yes. Maybe at some point it would be worth implementing RFC 6156.

> Is this "just" the fact that we send only a one shot permissions request right before the data indication, or something else?

This comment was mistaken. Have removed in my working copy. The permission code was wrong (permissions ignore port).

> Can we make this random instead?

Yeah, why not.

> I guess we make this later configurable

Or we could randomize, since it outputs the necessary iceServers:, I guess it depends.

> Not sure if we have to worry about hosts in CI w/o IPv6 setup?

I'll wrap this in a try.

> How about announcing as a STUN server as well?
> I think once this lands we should test with all possible combinations.

Probably makes sense. The codepaths for STUN are different.

> This doesn't strike me as the easiest way to get later extended with different behavior on different port numbers. But we can probably re-factor that later.

Not sure what you mean here.

> select? Yikes. Well I guess this is not going to be high performance, high throughput any way...

Is there a faster way to do this in python?

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/2-3/

Comment 6

[Byron Campen [:bwc]]

So, where is going to be the best place to run this thing? It is probably best to run/kill once per webrtc mochitest (can we use the .ini tags for this, maybe?).

Comment 7

[Byron Campen [:bwc]]

Also, who do I ask for review on the testing/automation side of things? ted?

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review33561

LGTM

Comment 9

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/36405/#review33103

> Not sure what you mean here.

Once we are happy with having this up and running as a properly working STUN/TURN server I imagine that we are going to want special instances of this which are mis-behaving on purpose, like delay answers, no answer for X packets, don't answer over UDP etc. And this endless loop in main doesn't make that easier. But as I said that is easy enough to refactor later.

> Is there a faster way to do this in python?

I was hoping for poll(), but the poll() implementation apparently depends on the OS, thus you would need an abstraction layer to figure out what is supported by the host OS. Probably not worth writing something like that right now if we don't get it for free through a lib like Twisted.

Comment 10

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #6)
> So, where is going to be the best place to run this thing? It is probably
> best to run/kill once per webrtc mochitest (can we use the .ini tags for
> this, maybe?).

My guess is that we would need to start it before our CPP unit tests start, and also before our mochitests start, right?
And we will tear it down once our tests are over. So the question is basically how can we start and stop something as test fixtures?

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/3-4/

Comment 12

[Andrew Halberstadt [:ahal]]

So to make sure I understand.. You want to run some python code before and after a specific test or group of tests. Unfortunately there's not a great way to do that.

Likely you'll need to use message passing over the structured logger. Grep for "buffering_on" under testing/mochitest to see an example of a pre-existing custom message like this. A big caveat, is that the logger can't pass messages from python back to JS. So the tests would need to do some sort of polling to figure out when the server is ready/shutdown.

Another alternative is to roll your own websocket based protocol and just have the harness kick start the server process at the beginning. This option might be easier if you have to do this for several different harnesses.

Comment 13

[Byron Campen [:bwc]]

(In reply to Andrew Halberstadt [:ahal] from comment #12)
> So to make sure I understand.. You want to run some python code before and
> after a specific test or group of tests. Unfortunately there's not a great
> way to do that.

   We'd want to launch a python process before a set of tests, and then kill that process once the tests are done.

> Likely you'll need to use message passing over the structured logger. Grep
> for "buffering_on" under testing/mochitest to see an example of a
> pre-existing custom message like this. A big caveat, is that the logger
> can't pass messages from python back to JS. So the tests would need to do
> some sort of polling to figure out when the server is ready/shutdown.
>
> Another alternative is to roll your own websocket based protocol and just
> have the harness kick start the server process at the beginning. This option
> might be easier if you have to do this for several different harnesses.


   That seems a little more involved than I was hoping; would it be easier if we ran the server at the beginning of the entire suite, and killed at the end, but only if the suite contains a test tagged 'webrtc'?

Comment 14

[Andrew Halberstadt [:ahal]]

I guess we could do that.. I'd sort of prefer not to though. Plus your proposal may cause intermittents in the other tests and end up being even more work in the long run (don't underestimate how flaky mochitests are :p). Passing messages via the logger isn't terribly hard assuming you can poll for the server from the test. Is that no possible?

Alternatively we could make the webrtc tests into their own flavor or subsuite, then only run the server if we are running that job. They'd have their own unique symbol in treeherder that way. Though this takes a bit of effort as well.

Comment 15

[Byron Campen [:bwc]]

(In reply to Andrew Halberstadt [:ahal] from comment #14)
> I guess we could do that.. I'd sort of prefer not to though. Plus your
> proposal may cause intermittents in the other tests and end up being even
> more work in the long run (don't underestimate how flaky mochitests are :p).
> Passing messages via the logger isn't terribly hard assuming you can poll
> for the server from the test. Is that no possible?

   To be honest, I'm not wild about (ab)using the structured logger to launch processes. It would also be really nice to be able to have some two-way communication (the TURN server spits out some JSON that I would like to use to configure stuff in the test-case, ideally, and it would be very nice to have any output from the TURN server in the logging).

> 
> Alternatively we could make the webrtc tests into their own flavor or
> subsuite, then only run the server if we are running that job. They'd have
> their own unique symbol in treeherder that way. Though this takes a bit of
> effort as well.

   This seems somewhat promising, but does it give us the console output from the TURN server?

   You mentioned the idea of a custom websocket dealie earlier; are you talking about a really tiny python websocket server that is run at the beginning of the test suite, and when we open a websocket to it, it runs the TURN server and pipes us the output?

Comment 16

[Nils Ohlmeier [:drno]]

From my point of view once we have the TURN server integrated we will write a lot of tests (or change existing tests) which will have a dependency on the server being available.
To me the natural solution for such a dependency of tests is that the test framework provides a setup function. And if any error, like failing to start a server, results in the tests getting skipped or the test getting marked as failed (test result depends on preferences).

Now by letting the logging framework start something I don't see how the test would know if the required server is actually up and running. That smells to like intermittent tests results (depending on reliably we can start the server, and if starting the server and test timing wise works out).

You could now require that every single test which has a dependency on external services needs to check for their availability, but that is IMHO just pushing the job of test framework down into the tests and will result in lots of duplicated code.

I understand that most teams in Mozilla, and especially not teams with code in mozilla central, don't have such external dependencies. But would still be possible to extend our test frameworks to handle these things in a sane way like most other test frameworks I have seen would do it (e.g. have a [Setup] section in the .ini file)?
We can also offer our support in writing this, as long as such effort is not going to be rejected once we try to land it.

BTW this does not only affect mochitest. I think we have an even bigger desire to have the TURN server available for our CPP unit tests.

Comment 17

[Andrew Halberstadt [:ahal]]

A lot of test directories have a sort of setup/teardown between tests, but it's all in JS and isn't built-in to the harness (usually these files are called head.js). I would love for the harness to be able to support what you want, your efforts definitely wouldn't be rejected. This would actually be my preferred solution. I didn't suggest this approach because it is the most difficult path, and I wasn't sure who would have the resources to work on it. I'm happy to provide guidance and reviews though if this was something you wanted to try.

With that being said, here's how mochitest works (I'm less familiar with CPP):
1. INI manifests are processed and converted into a list of tests
2. Python process starts gecko
3. A dedicated thread processes output (i.e the structured logs)
4. Main thread blocks until Gecko shuts down

Your problem basically boils down to how to communicate from JS to Python. Currently stdout is the only way to do this, but this is uni-directional. A better way would be over sockets (I think there's actually a bug on file somewhere to send the logs over a socket rather than over stdout). No matter how we slice it (whether we use manifests, or build something into the harness, or include a setup file next to the tests), this will be the biggest technical hurdle.

Comment 18

[Andrew Halberstadt [:ahal]]

To highlight my point a bit more clearly..

The python side is dumb. It just launches Gecko and then waits for it to shutdown again. It pushes the tests down a hill and is powerless to do anything about them until they roll to a stop.

Comment 19

[Byron Campen [:bwc]]

So, let's talk about using a python websocket server to launch the TURN server and bridge stuff like stdout.

It looks like the harness code is already running a websocket server in python for the duration of the test suite; how hard would it be to teach this to do the following when a specific URL (say, /turnserver) is hit?

1. Launch the TURN server.
2. Send all stdout/stderr to the websocket client.
3. Ignore writes from the websocket client (we _could_ send this to stdin, but unless we have a good reason I think we should avoid this)
4. When the websocket is closed, kill the process.
5. If the process ends, close the websocket (after flushing writes)

If it would be hard to add this functionality to the existing websocket server, I'm pretty sure I can whip up something purpose-built with twisted pretty quickly.

Comment 20

[Andrew Halberstadt [:ahal]]

We could possibly subvert the existing websocket server, but I don't know much about it, or if it would have an impact on existing tests. The other downside to this approach is that it wouldn't be very portable to e.g CPP unittests if that's something you wanted.

To clarify, is this server going to be started once at the start and left running for the duration of the webrtc tests? If so, then maybe we should just spin off a new mochitest flavor and have the harness start it up as part of the setup. I can help with this if we go this route.

Another question.. Are you using mochitest out of necessity or out of familiarity? It might also be worth looking into using the marionette test suite instead. Those tests are written in python (though they can execute arbitrary JS in either chrome or content) and have proper setUp and tearDown methods between each test. It would definitely be a lot easier to start/stop a python server with those.

Comment 21

[Byron Campen [:bwc]]

(In reply to Andrew Halberstadt [:ahal] from comment #20)
> We could possibly subvert the existing websocket server, but I don't know
> much about it, or if it would have an impact on existing tests. The other
> downside to this approach is that it wouldn't be very portable to e.g CPP
> unittests if that's something you wanted.

   Good point.

> 
> To clarify, is this server going to be started once at the start and left
> running for the duration of the webrtc tests? If so, then maybe we should
> just spin off a new mochitest flavor and have the harness start it up as
> part of the setup. I can help with this if we go this route.
> 

   Yes, this is the intent. A new mochitest flavor is appealing in some ways, I guess it depends on how hard it is to do. FWIW, this websocket server is very simple and lightweight (I can attach it).

> Another question.. Are you using mochitest out of necessity or out of
> familiarity? It might also be worth looking into using the marionette test
> suite instead. Those tests are written in python (though they can execute
> arbitrary JS in either chrome or content) and have proper setUp and tearDown
> methods between each test. It would definitely be a lot easier to start/stop
> a python server with those.

   I am unsure, as I have not worked with marionette. I suspect that we've just never had much reason to switch, since we have no real need for driving the UI.

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/4-5/

Comment 23

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Re-asking review, rewrote using twisted, and a few other changes.

Comment 24

[Byron Campen [:bwc]]

Attachment: Simple websocket-based ICE server launcher

Comment 25

[Nils Ohlmeier [:drno]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review34783

::: testing/tools/iceServer/iceServer.py:384
(Diff revisions 4 - 5)
> -        return origLen - len(data)
> +    lastindication = None

What is this?
Doesn't seem to be used any where. If you need it I would move it into the self. space up in __init__()

::: testing/tools/iceServer/iceServer.py:676
(Diff revisions 4 - 5)
> -            "{url:\"stun:localhost\"}," +
> +    allocationPruner.start(1)

Shouldn't be sufficient to run the pruning every 5 or 10 seconds?

Comment 26

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/36405/#review34783

> What is this?
> Doesn't seem to be used any where. If you need it I would move it into the self. space up in __init__()

That's dead code, thanks for spotting.

> Shouldn't be sufficient to run the pruning every 5 or 10 seconds?

I was thinking that we'd want fairly fast expiration in case we wanted to test it, although we could get away with checking the expiry whenever something tries to use the allocation, and a less frequent pruning loop.

Comment 27

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/5-6/

Comment 28

[Andrew Halberstadt [:ahal]]

Comment on attachment 8726761 [details]
Simple websocket-based ICE server launcher

This looks reasonable to me, though I'm not familiar with twisted, so can't say whether it's accurate. This could potentially go in testing/tools as place it can be re-used. The dependency on twisted will also need to be dealt with.

I think it would still be easier to spin off a new flavor or subsuite and launch the ICE server when that is run.

Comment 29

[Nils Ohlmeier [:drno]]

(In reply to Andrew Halberstadt [:ahal] from comment #28)
> I think it would still be easier to spin off a new flavor or subsuite and
> launch the ICE server when that is run.

Sounds good. But I assume a new flavor would only cover the mochitests, right?

Maybe we can take care of the CPP unit test part in separate ticket, but as we so far seem to run all CPP unit tests in one big chunk I'm assuming that we are not going to split the WebRTC CPP unit tests out. Therefore we would still need a solution to start the dependency for that. And I guess some socket based control would/could be still a good solution.

Comment 30

[Andrew Halberstadt [:ahal]]

Ah yes, that's a good point. CPP doesn't have a concept of flavors or subsuites.. so it would be significantly more work to do that there.

Comment 31

[Byron Campen [:bwc]]

Using a websocket thingie wouldn't help the CPP (or gecko gtest) tests anyway. So what needs to be done to create a new mochitest flavor? Will running a single test-case manually run the fixture too?

Comment 32

[Byron Campen [:bwc]]

Before we lose track of it, the websocket thing does offer some advantages:

- A crash in the TURN server is detected easily by the test code (the websocket drops)
- The mochitest gets the output from the TURN server (useful because the first thing it outputs is the iceServers config to use)
- The mochitest could send commands to the TURN server; this could be useful later on.
- Each mochitest gets a pristine TURN server; could cut down on oranges.

Comment 33

[Andrew Halberstadt [:ahal]]

Though it might be good to make a new flavor/subsuite either way, so we can run the websocket thing only when necessary. I think for this case a subsuite is our best bet. Here's how they work.

1. Add a subsuite key to every test and/or DEFAULT section in a manifest that you want to be included in this subsuite. Doing this automatically removes the test from the default set (it'll no longer be run as part of regular mochitests). E.g:
https://dxr.mozilla.org/mozilla-central/source/dom/push/test/mochitest.ini#2

2. Somewhere in the mochitest harness, add code that runs either the TURN server or the websocket thing behind an "if options.subsuite == 'webrtc'" block.

3. Create a new job (in taskcluster or buildbot) that passes in --subsuite=webrtc, and give it a unique symbol (possibly 'wr'?)

4. Running locally should *just work*. Tests in this subsuite should automatically be picked up if a containing path is used:
./mach mochitest path/to/dir/containing/webrtc/tests

Alternatively the entire subsuite can be run with:
./mach mochitest --flavor plain --subsuite webrtc

Comment 34

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/6-7/

Comment 35

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review commit: https://reviewboard.mozilla.org/r/39525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39525/

Comment 36

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Is this roughly what we're looking for?

Comment 37

[Andrew Halberstadt [:ahal]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Yeah! I only looked at the stuff under testing/mochitest, but it looks great.

Comment 38

[Byron Campen [:bwc]]

Ok, I have some cleanup to do, and can then put it up for review on Monday. What needs to be done to get our python dependencies in place?

Comment 39

[Andrew Halberstadt [:ahal]]

Are the dependencies just the python files for the server? Typically it's just copying files to the relevant directories in the objdir. You can do this with the TEST_HARNESS_FILES variable in moz.build.

Comment 40

[Byron Campen [:bwc]]

I was referring to stuff like twisted.

Comment 41

[Byron Campen [:bwc]]

Also, what do I do to get these new subsuites running in CI? I'm guessing I would need to touch stuff somewhere in h.m.o/build? I see buildbot stuff, but my understanding is that things are migrating to taskcluster; should I still be using the buildbot-configs repo?

Comment 42

[Andrew Halberstadt [:ahal]]

(In reply to Byron Campen [:bwc] from comment #40)
> I was referring to stuff like twisted.

Right, that's a bit more tricky. Because the test machines can't hit the network, it needs to be uploaded to an internal pypi server:
http://puppetagain.pub.build.mozilla.org/data/python/packages/

I think you need to file bug under Release::Engineering and ask someone to do it (ping #releng for more info). Then we'll need to update the mozharness script to install twisted into the virtualenv that the test machines use from that pypi server. For the local path we can get mach to install twisted from the official pypi if it isn't available.

An alternative is to check twisted into the tree under /python.. but we try to avoid doing that unless it's a package that's so ubiquitous it can be useful everywhere.


(In reply to Byron Campen [:bwc] from comment #41)
> Also, what do I do to get these new subsuites running in CI? I'm guessing I
> would need to touch stuff somewhere in h.m.o/build? I see buildbot stuff,
> but my understanding is that things are migrating to taskcluster; should I
> still be using the buildbot-configs repo?

Yes, things are migrating to taskcluster, but very slowly. If you only need to run this on linux64 you can ignore buildbot. Otherwise you'll need to deal with buildbot-configs and taskcluster. See the buildbot-configs/mozilla-tests/config.py file. It's kind of horrible, but there are examples you can copy. Try to follow the MOCHITEST_PUSH config as an example (these tests are variants of mochitest-plain right?).

Comment 43

[Byron Campen [:bwc]]

(In reply to Andrew Halberstadt [:ahal] from comment #42)
> (In reply to Byron Campen [:bwc] from comment #40)
> > I was referring to stuff like twisted.
> 
> Right, that's a bit more tricky. Because the test machines can't hit the
> network, it needs to be uploaded to an internal pypi server:
> http://puppetagain.pub.build.mozilla.org/data/python/packages/

  Hey, twisted seems to be there already. There are some other things we would need though. I've opened a bug.

> I think you need to file bug under Release::Engineering and ask someone to
> do it (ping #releng for more info). Then we'll need to update the mozharness
> script to install twisted into the virtualenv that the test machines use
> from that pypi server. For the local path we can get mach to install twisted
> from the official pypi if it isn't available.

  What files need to be touched here?

> An alternative is to check twisted into the tree under /python.. but we try
> to avoid doing that unless it's a package that's so ubiquitous it can be
> useful everywhere.

   We have a few more things than twisted, so we probably don't want to do this.

> (In reply to Byron Campen [:bwc] from comment #41)
> > Also, what do I do to get these new subsuites running in CI? I'm guessing I
> > would need to touch stuff somewhere in h.m.o/build? I see buildbot stuff,
> > but my understanding is that things are migrating to taskcluster; should I
> > still be using the buildbot-configs repo?
> 
> Yes, things are migrating to taskcluster, but very slowly. If you only need
> to run this on linux64 you can ignore buildbot. Otherwise you'll need to
> deal with buildbot-configs and taskcluster. See the
> buildbot-configs/mozilla-tests/config.py file. It's kind of horrible, but
> there are examples you can copy. Try to follow the MOCHITEST_PUSH config as
> an example (these tests are variants of mochitest-plain right?).

   We need this across the board, and from my limited look around there's going to be a _lot_ of stuff that needs to be touched. Further, is it even possible to test changes in buildbot-configs on try? This is looking like an awful lot of trouble to me. I can see the value in having our own mochitest flavor, since that will make it really easy to select our tests on try, but perhaps that's something that we should put off until taskcluster is fully deployed? If it makes it easier to swallow, I can modify the websocket doodad that I wrote to be more extensible, suitable for a general purpose launcher, so other test suites can use it if necessary.

Comment 44

[Byron Campen [:bwc]]

I should note that mochitest-push isn't run on all platforms, so it is an incomplete example.

Comment 45

[Nils Ohlmeier [:drno]]

How about we land your TURN server code here. And then tackle the remaining CI problems in new tickets?

Comment 46

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/7-8/

Comment 47

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/1-2/

Comment 48

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review commit: https://reviewboard.mozilla.org/r/40229/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40229/

Comment 49

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/39525/#review36875

Not a full review. Just the pieces I noticed when trying to understand the new Turn server startup logic.

::: dom/media/tests/mochitest/pc.js:1808
(Diff revision 2)
> +  var enableHttpProxy = enable => new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv(
> +        {'set': [['media.peerconnection.disable_http_proxy', !enable]]},
> +        resolve);
> +  });

I would prefer if you would simply resolve the promise right away if disabling the http proxy is not needed.
That way you can skip the SpecialPowers.pushPrefEnv() call and just run with the default value.

::: dom/media/tests/mochitest/pc.js:1853
(Diff revision 2)
>        startNetworkAndTest()
> +        .then(() => setupIceServerConfig(fixtureOptions.useIceServer))

Wouldn't it make sense to roll setupIceServerConfig() into the startNetworkAndTest()?
I would then rename startNetworkAndTest() into something like networkDepenciesReady(), because its current name is actually mis-leading - it never starts the test itself.

Comment 50

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/39525/#review36875

> I would prefer if you would simply resolve the promise right away if disabling the http proxy is not needed.
> That way you can skip the SpecialPowers.pushPrefEnv() call and just run with the default value.

We still need to set it back to false after we have run a test that set it to true; this is set on a test-by-test basis.

> Wouldn't it make sense to roll setupIceServerConfig() into the startNetworkAndTest()?
> I would then rename startNetworkAndTest() into something like networkDepenciesReady(), because its current name is actually mis-leading - it never starts the test itself.

I think I'd prefer to have it be separate to keep the function size down.

Comment 51

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review commit: https://reviewboard.mozilla.org/r/40453/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40453/

Comment 52

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/2-3/

Comment 53

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/1-2/

Comment 54

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/2-3/

Comment 55

[Byron Campen [:bwc]]

Ok, I have this working locally with the mochitests in bug 1231975. In CI, it is successfully installing twisted, but as expected the other deps fail to install. I think this is as far as I can go until bug 1256676 is resolved.

Comment 56

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/8-9/

Comment 57

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/1-2/

Comment 58

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/3-4/

Comment 59

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/3-4/

Comment 60

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/4-5/

Comment 61

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review commit: https://reviewboard.mozilla.org/r/40615/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40615/

Comment 62

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

It seems that there's a ton of options here, from doing nothing and just letting the imports fail, to detecting and installing stuff that is missing. Additionally, there's the question of whether we require the webrtc python dependencies to be installed for all mochitest runs, or just ones that contain some webrtc tests.

As a straw-man proposal, I've opted to print some easily visible diagnostics and fail when we detect some modules are missing, and require the webrtc dependencies on all runs.

Comment 63

[Andrew Halberstadt [:ahal]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review37285

I don't understand what is going on in this file so mostly reviewed for style. I don't like holding things up just for style nits, so use your best judgement on whether to drop or fix. Each comment has multiple infractions, but I only mentioned them once.

::: testing/tools/iceServer/iceServer.py:1
(Diff revision 9)
> +# vim: set ts=4 et sw=4 tw=80

Missing license block

::: testing/tools/iceServer/iceServer.py:52
(Diff revision 9)
> +XOR_MAPPED_ADDRESS = 0x0020
> +SOFTWARE = 0x8022
> +ALTERNATE_SERVER = 0x8023
> +FINGERPRINT = 0x8028
> +
> +def unpackUInt(bytesBuf):

nit: prefer underscores over camel case for function/variable names

::: testing/tools/iceServer/iceServer.py:92
(Diff revision 9)
> +    dest = dest << numBits
> +    mask = (1 << numBits) - 1
> +    dest += source & mask
> +    return dest
> +
> +class StunAttribute:

Classes should inherit from object in python 2, e.g `class StunAttribute(object):`

::: testing/tools/iceServer/iceServer.py:103
(Diff revision 9)
> +#     |                         Value (variable)                ....
> +#     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +    __attrHeaderFmt = [2,2]
> +    __attrHeaderSize = reduce(operator.add, __attrHeaderFmt)
> +
> +    def __init__(self, attrType = 0, buf = bytearray()):

no spaces around '=' in function signatures

::: testing/tools/iceServer/iceServer.py:135
(Diff revision 9)
> +                raise ValueError("Non-zero padding")
> +            length += 1
> +
> +        return length
> +
> +class StunMessage:

Would be nice to include some docstrings on the classes at least

::: testing/tools/iceServer/iceServer.py:360
(Diff revision 9)
> +    def close(self):
> +        self.port.stopListening()
> +        self.port = None
> +
> +class StunHandler:

nit: two spaces between classes

::: testing/tools/iceServer/iceServer.py:524
(Diff revision 9)
> +            print ("Dropping send indication; no allocation for tuple {}"
> +                    .format(self.getAllocationTuple()))
> +            return
> +
> +        peerAddress = indication.getXorAddress(XOR_PEER_ADDRESS)
> +        if not peerAddress:
> +            print "Dropping send indication, missing XOR-PEER-ADDRESS"

Use print("foobar") consistently, also no space between print and (.

Comment 64

[Andrew Halberstadt [:ahal]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

https://reviewboard.mozilla.org/r/40453/#review37291

::: testing/mochitest/moz.build:157
(Diff revision 2)
> +TEST_HARNESS_FILES.testing.mochitest.iceServer += [
> +    '/testing/tools/iceServer/iceServer.py',
> +]
> +
> +TEST_HARNESS_FILES.testing.mochitest.websocketToProcessBridge += [
> +    '/testing/tools/websocketToProcessBridge/websocketToProcessBridge.py',
> +]

These camelcased directory names are inconsistent with everything else under /testing.. maybe call it 'iceserver' and 'websocketbridge' ? Or use dashes if you must.

::: testing/mochitest/runtests.py:810
(Diff revision 2)
> +        """Create a websocket server that can launch various processes that"""
> +        """JS needs (eg; ICE server for webrtc testing)"""

These should be a single docstring:
"""Create a websocket server that can launch
various processes that...
"""

::: testing/mochitest/runtests.py:831
(Diff revision 2)
> +            self.log.info("TEST-UNEXPECTED-FAIL | runtests.py | Timed out while"
> +                          " waiting for websocket/process bridge startup.")

self.log.error("runtests.py | Timed out while...")

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:1
(Diff revision 2)
> +from twisted.internet import protocol, reactor
> +import txws

Missing license block.

nit: keep 3rd party imports separate from stdlib imports down below.

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:12
(Diff revision 2)
> +# Handles the spawned process (I/O, process termination)
> +class ProcessSide(protocol.ProcessProtocol):

Might as well make this a docstring, same for SocketSide below.

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:17
(Diff revision 2)
> +# Handles the spawned process (I/O, process termination)
> +class ProcessSide(protocol.ProcessProtocol):
> +    def __init__(self, socketSide):
> +        self.socketSide = socketSide
> +
> +    def outReceived(self, data):

nit: use underscores over camel case

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:43
(Diff revision 2)
> +            try:
> +                self.processSide = ProcessSide(self)
> +                reactor.spawnProcess(self.processSide,
> +                                     commands[data][0],
> +                                     commands[data],
> +                                     usePTY=True)
> +            except:
> +                self.shutdown()

This looks like it could potentially result in silent failures. Can we at least reraise after self.shutdown() so the main process has a chance of pulling the traceback out of the output? Not sure what twisted does with exceptions.

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:58
(Diff revision 2)
> +        if self.processSide:
> +            self.processSide.shutdown()
> +
> +    def shutdown(self):
> +        self.transport.loseConnection()
> +        self.processSide = None

I assume "connectionLost" will get called before we get to this line?

Comment 65

[Andrew Halberstadt [:ahal]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://reviewboard.mozilla.org/r/40229/#review37293

::: testing/config/marionette_requirements.txt:1
(Diff revision 5)
> --r mozbase_requirements.txt
> +-r mochitest_requirements.txt

Let's create a single new requirements file called "iceserver_requirements.txt" in testing/config. And in it can contain everything, e.g:

ipaddr
passlib
twisted
twxs

Then we can add a second `-r iceserver_requirements.txt` to the main marionette_requirements.txt. We can separate these deps out in the future if we need to.

::: testing/mozharness/scripts/android_emulator_unittest.py:156
(Diff revision 5)
> +        requirements = os.path.join(dirs['abs_test_install_dir'],
> +                                    'config',
> +                                    'mochitest_requirements.txt')

Taking my above suggestion, requirements can be a list of files here. So you can append both "mozbase_requirements.txt" and "iceserver_requirements.txt" in it. No need for one to include the other. In fact, if you didn't want to include iceserver_requirements.txt in marionette_requirements.txt, you could do something similar in desktop_unittest.py.

Comment 66

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/40453/#review37299

::: testing/mochitest/runtests.py:853
(Diff revision 2)
>          if not options.httpdPath:
>              options.httpdPath = os.path.join(options.utilityPath, "components")
>  
>          self.startWebServer(options)
>          self.startWebSocketServer(options, debuggerInfo)
> +        self.startWebsocketToProcessBridge(options)

The plan was still to make a new subsuite right? If so this should be behind an `if options.subsuite == "webrtc"`

Comment 67

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/40615/#review37301

Better than checking for the required packages, is to get the mach command to install these automatically. Do a dxr for `install_pip_package` for an example how.

Comment 68

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40453/#review37291

> This looks like it could potentially result in silent failures. Can we at least reraise after self.shutdown() so the main process has a chance of pulling the traceback out of the output? Not sure what twisted does with exceptions.

The intent here is for the websocket client to notice that something has gone wrong, because the connection has closed prematurely, but not catching the exception is probably better.

> I assume "connectionLost" will get called before we get to this line?

No. This is called when the processSide has either died, or was never successfully established, and doesn't need to be shut down. (This is reciprocal; the first of the two to go away calls shutdown on the other)

Comment 69

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40453/#review37299

> The plan was still to make a new subsuite right? If so this should be behind an `if options.subsuite == "webrtc"`

Perhaps eventually, but not now. Since the websocket/process bridge is a more generic tool, it probably doesn't make sense to hide it behind such a check even if we did have the subsuite.

Comment 70

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40615/#review37301

I'll do that.

Comment 71

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40453/#review37291

> nit: use underscores over camel case

All of these functions are inherited. I can change the variables though.

Comment 72

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40453/#review37291

> No. This is called when the processSide has either died, or was never successfully established, and doesn't need to be shut down. (This is reciprocal; the first of the two to go away calls shutdown on the other)

I will rename these to be more clear.

Comment 73

[Andrew Halberstadt [:ahal]]

(In reply to Byron Campen [:bwc] from comment #71)
> https://reviewboard.mozilla.org/r/40453/#review37291
> 
> > nit: use underscores over camel case
> 
> All of these functions are inherited. I can change the variables though.

It's ok then, as long as the functions are camelCase might as well keep the variables consistent. Feel free to drop that issue.

Comment 74

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40229/#review37293

> Let's create a single new requirements file called "iceserver_requirements.txt" in testing/config. And in it can contain everything, e.g:
> 
> ipaddr
> passlib
> twisted
> twxs
> 
> Then we can add a second `-r iceserver_requirements.txt` to the main marionette_requirements.txt. We can separate these deps out in the future if we need to.

I was trying to keep the iceserver requirements (webrtc-specific) separate from the websocketprocessbridge requirements (more generic), but I guess it could be argued that websocketprocessbridge requires the iceserver stuff because iceserver is on the "menu". So, I can just put everything in a requirements file for websocketprocessbridge.

Comment 75

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40229/#review37293

> Taking my above suggestion, requirements can be a list of files here. So you can append both "mozbase_requirements.txt" and "iceserver_requirements.txt" in it. No need for one to include the other. In fact, if you didn't want to include iceserver_requirements.txt in marionette_requirements.txt, you could do something similar in desktop_unittest.py.

I think it makes sense for there to be a new requirements file for mochitest (but without marionette's dependencies), since it is confusing for MochitestDesktop to pull in a marionette_requirements.txt, and since mochitest now has additional requirements on top of mozbase that need to be known in multiple places. In fact, I'm still not clear what's required by mochitest but not marionette.

Comment 76

[Andrew Halberstadt [:ahal]]

I wouldn't over-architect this dependency thing, I don't think it needs to be perfect (and fwiw, mochitest does depend on marionette). I'm a fan of keeping it simple and when/if things get more complicated, dealing with it then. What I really want is to be able to define a list of requirements files in configuration on a per-suite basis. This is what I filed bug 1256975 for. So just be aware that whatever approach you take will likely be refactored anyway when that gets tackled.

Comment 77

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/9-10/

Comment 78

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/2-3/

Comment 79

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/4-5/

Comment 80

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/5-6/

Comment 81

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/1-2/

Comment 82

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/2-3/

Comment 83

[Andrew Halberstadt [:ahal]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://reviewboard.mozilla.org/r/40229/#review37705

::: testing/mozharness/scripts/android_emulator_unittest.py:156
(Diff revision 6)
> +        requirements = os.path.join(dirs['abs_test_install_dir'],
> +                                    'config',
> +                                    'mochitest_requirements.txt')

I'm pretty sure this will break most test suites. The desktop_unittest.py script is used by all kinds of suites not restricted to mochitest. Besides, mochitest actually does depend on marionette.

I'd do something like:

    reqs = [
        'marionette_requirements.txt',
        'websocketprocessbridge_requirements.txt',
    ]
    register_virtualenv_module(requirements=reqs)

::: testing/mozharness/scripts/androidx86_emulator_unittest.py:163
(Diff revision 6)
> +        requirements = os.path.join(dirs['abs_test_install_dir'],
> +                                    'config',
> +                                    'mochitest_requirements.txt')

For android, mochitest does not depend on marionette, but this will likely change soon as we'll need to use it to get around addon signing.

Comment 84

[Andrew Halberstadt [:ahal]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

https://reviewboard.mozilla.org/r/40453/#review37707

Thanks!

::: testing/mochitest/runtests.py:832
(Diff revisions 2 - 3)
>                  sock.close()
>                  break
>              except:
>                  time.sleep(0.1)
>          else:
> -            self.log.info("TEST-UNEXPECTED-FAIL | runtests.py | Timed out while"
> +            self.log.error("TEST-UNEXPECTED-FAIL | runtests.py | Timed out while"

The TEST-UNEXPECTED-FAIL is redundant here. log.error will dump ERROR instead, and both mozharness and treeherder log parsers are set up to detect it.

Comment 85

[Nils Ohlmeier [:drno]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

https://reviewboard.mozilla.org/r/39525/#review38207

::: dom/media/tests/mochitest/pc.js:1821
(Diff revision 5)
> +  });
> +
> +  var spawnIceServer = () => new Promise( (resolve, reject) => {
> +    iceServerWebsocket = new WebSocket("ws://localhost:8191/");
> +    iceServerWebsocket.onopen = (event) => {
> +      info("launcher websocket open, running ICE Server...");

running = starting ?

::: dom/media/tests/mochitest/pc.js:1829
(Diff revision 5)
> +
> +    iceServerWebsocket.onmessage = event => {
> +      // The first message will contain the iceServers configuration, subsequent
> +      // messages are just logging.
> +      info("ICE Server: " + event.data);
> +      resolve(event.data);

I think it does not break anything right now, but I guess that if you keep resolving the Promise with new/different data if something later calls the same promise he would not get the configuration but the logging data instead?!

::: dom/media/tests/mochitest/pc.js:1842
(Diff revision 5)
> +  if (!useIceServer) {
> +    info("Skipping ICE Server for this test");
> +    iceServersArray = [];
> +    return enableHttpProxy(true);
> +  }

I think |iceServersArray| should simply be default inited to []. Then you can remove that assignment.

And the enabledHttpProxy() call is just extra over head, as it being enabled is still the default, or?

Once you remove these two line I think we should move the remaining check to the top of setupIceServerConfig() to exit as early as possible.

::: dom/media/tests/mochitest/pc.js:1850
(Diff revision 5)
> +    return enableHttpProxy(true);
> +  }
> +
> +  return enableHttpProxy(false)
> +    .then(spawnIceServer)
> +    .then(iceServersStr => { iceServersArray = JSON.parse(iceServersStr); });

You should guard this with a catch(), especially the JSON parsing.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:502
(Diff revision 5)
>      if (!cb_arg)
>        return;
>      ctx = cand->ctx;
>  
>      ctx->uninitialized_candidates--;
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE(%s)/CAND(%s): initialized",ctx->label,cand->codeword);

How about including the |unitialized_candidates| in the log message?

Comment 86

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/39525/#review38207

> I think it does not break anything right now, but I guess that if you keep resolving the Promise with new/different data if something later calls the same promise he would not get the configuration but the logging data instead?!

My understanding is that promises are resolved at most once, and any subsequent calls to resolve are ignored. A subsequent waiter will get the original argument passed to resolve.

> I think |iceServersArray| should simply be default inited to []. Then you can remove that assignment.
> 
> And the enabledHttpProxy() call is just extra over head, as it being enabled is still the default, or?
> 
> Once you remove these two line I think we should move the remaining check to the top of setupIceServerConfig() to exit as early as possible.

We need to re-enable http proxy support in case the previous test disabled it.

> You should guard this with a catch(), especially the JSON parsing.

I can look into this, although errors here do percolate up.

> How about including the |unitialized_candidates| in the log message?

Sure, why not.

Comment 87

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/3-4/

Comment 88

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/5-6/

Comment 89

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/6-7/

Comment 90

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/3-4/

Comment 91

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/39525/#review38207

> We need to re-enable http proxy support in case the previous test disabled it.

I've changed how iceServersArray is initted, the rest I can't do.

Comment 92

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/39525/#review38207

> I can look into this, although errors here do percolate up.

Errors in there end up getting caught here:

https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/head.js#325

This does not call SimpleTest.finish() though, so the test hangs. That's probably easily fixable.

Comment 93

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/6-7/

Comment 94

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/7-8/

Comment 95

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/4-5/

Comment 96

[Andrew Halberstadt [:ahal]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://reviewboard.mozilla.org/r/40229/#review38731

Thanks, looks good! (I assume that failing try run was from an earlier version of this patch?)

Comment 97

[Byron Campen [:bwc]]

(In reply to Andrew Halberstadt [:ahal] from comment #96)
> Comment on attachment 8730920 [details]
> MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python
> dependencies in CI. r?ahal
> 
> https://reviewboard.mozilla.org/r/40229/#review38731
> 
> Thanks, looks good! (I assume that failing try run was from an earlier
> version of this patch?)

Try is failing, but it failing in the way I expect given that the deps aren't there yet.

Comment 98

[Andrew Halberstadt [:ahal]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://reviewboard.mozilla.org/r/40615/#review38741

::: python/mozbuild/mozbuild/virtualenv.py:449
(Diff revision 5)
> +    def install_pip_requirements_file(self, requirements_filename):
> +        """Installs packages in |requirements_filename| via pip.
> + 
> +           Any requirements that are already present are ignored.
> +        """
> +
> +        import pkg_resources
> +        with open(requirements_filename, 'r') as f:
> +            requirements = f.readlines()
> +
> +        # This will eat comments and such
> +        for req in pkg_resources.parse_requirements(requirements):
> +            self.install_pip_package(str(req))

I think this function needs a build peer review which I am not. Unlike for /testing, they are a bit more strict on the module system. I'd run it by gps.

::: python/mozbuild/mozbuild/virtualenv.py:460
(Diff revision 5)
> +        import pkg_resources
> +        with open(requirements_filename, 'r') as f:
> +            requirements = f.readlines()
> +
> +        # This will eat comments and such
> +        for req in pkg_resources.parse_requirements(requirements):

I'm not sure if it makes a difference, but you might want to use pip.req.parse_requirements() here instead.

::: testing/mochitest/runtests.py:817
(Diff revision 5)
>          """
>  
>          command = [sys.executable,
>                     os.path.join("websocketprocessbridge",
>                                  "websocketprocessbridge.py")]
> +        os.environ['PYTHONPATH'] = ":".join(p for p in sys.path)

os.pathsep.join(...)

This seems reasonable to me. Consider this an f+.

Comment 99

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40615/#review38741

> I'm not sure if it makes a difference, but you might want to use pip.req.parse_requirements() here instead.

I tried that, I could find no programmatic way to get back to the string form with the stuff yielded by pip.req.parse_requirements() (__str__ has a bunch of stuff in it that the pip command line doesn't like)

Comment 100

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/10-11/

Comment 101

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/4-5/

Comment 102

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/7-8/

Comment 103

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/8-9/

Comment 104

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/5-6/

Comment 105

[Gregory Szorc [:gps]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://reviewboard.mozilla.org/r/40615/#review38949

::: python/mozbuild/mozbuild/virtualenv.py:449
(Diff revision 6)
> +    def install_pip_requirements_file(self, requirements_filename):
> +        """Installs packages in |requirements_filename| via pip.
> +
> +           Any requirements that are already present are ignored.
> +        """

This should be invoking `pip install -r <path>` because processing a requirements file is so much more than extracting packages from individual lines. For example, requirements files can specify additional servers advertising custom packages.

Comment 106

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/40229/#review38951

::: testing/tools/websocketprocessbridge/websocketprocessbridge_requirements.txt:1
(Diff revision 9)
> +twisted

Please pin versions and add `--hash` to packages in requirements files. This ensures:

a) Versions don't change out from under us (this helps with deterministic operations and prevents breakage when 3rd party packages are upgraded)

b) Content is verified against tampering (malicious or not). This helps keep the Firefox release process secure.

https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/ansible/roles/hg-web/files/requirements-replication.txt is an example.

Comment 107

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40229/#review38951

> Please pin versions and add `--hash` to packages in requirements files. This ensures:
> 
> a) Versions don't change out from under us (this helps with deterministic operations and prevents breakage when 3rd party packages are upgraded)
> 
> b) Content is verified against tampering (malicious or not). This helps keep the Firefox release process secure.
> 
> https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/ansible/roles/hg-web/files/requirements-replication.txt is an example.

I'll do this once bug 1256676 is done.

Comment 108

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40615/#review38949

> This should be invoking `pip install -r <path>` because processing a requirements file is so much more than extracting packages from individual lines. For example, requirements files can specify additional servers advertising custom packages.

Easy enough.

Comment 109

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/11-12/

Comment 110

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/5-6/

Comment 111

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/8-9/

Comment 112

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/9-10/

Comment 113

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/6-7/

Comment 114

[Gregory Szorc [:gps]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://reviewboard.mozilla.org/r/40615/#review38971

::: python/mozbuild/mozbuild/virtualenv.py:454
(Diff revision 7)
> +    def install_pip_requirements_file(self, requirements_filename):
> +        """Installs packages in |requirements_filename| via pip."""
> +
> +        args = [
> +            'install',
> +            '--use-wheel',

I'm 99% sure that --use-wheel is the default in modern versions of pip. And as of yesterday, we have the latest version of pip vendored in mozilla-central. So this can be dropped.

Comment 115

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/7-8/

Comment 116

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/12-13/

Comment 117

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/6-7/

Comment 118

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/9-10/

Comment 119

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/10-11/

Comment 120

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/8-9/

Comment 121

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/11-12/

Comment 122

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/9-10/

Comment 123

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/12-13/

Comment 124

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/10-11/

Comment 125

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/13-14/

Comment 126

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/11-12/

Comment 127

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/14-15/

Comment 128

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/12-13/

Comment 129

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/13-14/

Comment 130

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/7-8/

Comment 131

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/10-11/

Comment 132

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/15-16/

Comment 133

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/13-14/

Comment 134

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/8-9/

Comment 135

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/11-12/

Comment 136

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/16-17/

Comment 137

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/14-15/

Comment 138

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/9-10/

Comment 139

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/12-13/

Comment 140

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/17-18/

Comment 141

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/15-16/

Comment 142

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/18-19/

Comment 143

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/16-17/

Comment 144

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40229/#review38951

> I'll do this once bug 1256676 is done.

I've set the version numbers, but this does not seem to work with the version of pip the testers use.

Comment 145

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40229/#review38951

> I've set the version numbers, but this does not seem to work with the version of pip the testers use.

(I should clarify; the version numbers work fine, but the version of pip in use doesn't grok --hash)

Comment 146

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/19-20/

Comment 147

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/17-18/

Comment 148

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/20-21/

Comment 149

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/18-19/

Comment 150

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/21-22/

Comment 151

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/19-20/

Comment 152

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review commit: https://reviewboard.mozilla.org/r/46415/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46415/

Comment 153

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/14-15/

Comment 154

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/10-11/

Comment 155

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/13-14/

Comment 156

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/22-23/

Comment 157

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/20-21/

Comment 158

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/15-16/

Comment 159

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/11-12/

Comment 160

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/14-15/

Comment 161

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/23-24/

Comment 162

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/21-22/

Comment 163

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/1-2/

Comment 164

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/40615/#review43473

::: testing/mochitest/runtests.py:818
(Diff revision 22)
>          """
>  
>          command = [sys.executable,
>                     os.path.join("websocketprocessbridge",
>                                  "websocketprocessbridge.py")]
> +        os.environ['PYTHONPATH'] = os.pathsep.join(p for p in sys.path)

This feels like a hack. Either the virtualenv has its paths configured correctly or we supplement PYTHONPATH with just the paths not in the virtualenv.

Comment 165

[Gregory Szorc [:gps]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

https://reviewboard.mozilla.org/r/46415/#review43475

Comment 166

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40615/#review43473

> This feels like a hack. Either the virtualenv has its paths configured correctly or we supplement PYTHONPATH with just the paths not in the virtualenv.

It is a hack. Right now the virtualenv doesn't have the system python path, while the functions that install packages into the virtualenv do see the python path. This means that if you have twisted in your system python, it will not be installed in the virtualenv, and then the virtualenv python won't find it.

Comment 167

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/16-17/

Comment 168

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/12-13/

Comment 169

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/15-16/

Comment 170

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/24-25/

Comment 171

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/22-23/

Comment 172

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/2-3/

Comment 173

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/13-14/

Comment 174

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/16-17/

Comment 175

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/25-26/

Comment 176

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/23-24/

Comment 177

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/3-4/

Comment 178

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/17-18/

Comment 179

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/14-15/

Comment 180

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/17-18/

Comment 181

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/26-27/

Comment 182

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/24-25/

Comment 183

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/4-5/

Comment 184

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a244bcd9911b7fe2da9387e07acb3ca5f7e1d0e3
Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://hg.mozilla.org/integration/mozilla-inbound/rev/de3f062c9a999718143e4567c475a260f75a6f2e
Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fcd0e98cc34ee7e24d22c08e4469f2cbc7d548
Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

https://hg.mozilla.org/integration/mozilla-inbound/rev/507145be7f820701cc3980050200141f7afc9457
Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0d061f75488cd6a44e59d9858742ae61b48f04
Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://hg.mozilla.org/integration/mozilla-inbound/rev/15472b261c9b0b7dc15716d4d5f5c02a606afef9
Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Comment 185

[Carsten Book [:Tomcat]]

backed out for mass mochitest bustage like : https://treeherder.mozilla.org/logviewer.html#?job_id=26551894&repo=mozilla-inbound

 14:21:56     INFO -  MochitestServer : launching [u'/home/worker/workspace/build/tests/bin/xpcshell', '-g', '/home/worker/workspace/build/application/firefox', '-v', '170', '-f', '/home/worker/workspace/build/tests/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpQNmhY_.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/worker/workspace/build/tests/mochitest/server.js']
 14:21:56     INFO -  runtests.py | Server pid: 1198
 14:21:56     INFO -  runtests.py | Websocket server pid: 1201
 14:21:56     INFO -  runtests.py | websocket/process bridge pid: 1206
 14:22:06     INFO -  0 ERROR runtests.py | Timed out while waiting for websocket/process bridge startup.
 14:22:06     INFO -  Stopping web server
 14:22:06     INFO -  Stopping web socket server
 14:22:06     INFO -  Stopping websocket/process bridge
 14:22:06     INFO -  websocket/process bridge listening on port 8191
 14:22:06     INFO -  Stopping web server
 14:22:06     INFO -  Stopping web socket server
14:22:06 INFO - Stopping websocket/process bridge

Comment 186

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17a5796a6140
https://hg.mozilla.org/integration/mozilla-inbound/rev/966b2dc8d9e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8652ef9f94b
https://hg.mozilla.org/integration/mozilla-inbound/rev/202dd76b1103
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee63f8f80696
https://hg.mozilla.org/integration/mozilla-inbound/rev/67fde9672750

Comment 187

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/15-16/

Comment 188

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/18-19/

Comment 189

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/27-28/

Comment 190

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/25-26/

Comment 191

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/5-6/

Comment 192

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/18-19/

Comment 193

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/16-17/

Comment 194

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/19-20/

Comment 195

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/28-29/

Comment 196

[Byron Campen [:bwc]]

Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/26-27/

Comment 197

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/6-7/

Comment 198

[Byron Campen [:bwc]]

Should be fine now.

Comment 199

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8db6695b991
https://hg.mozilla.org/integration/mozilla-inbound/rev/c68772362633
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2ad6863f53
https://hg.mozilla.org/integration/mozilla-inbound/rev/2942bb31152c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8583070c9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa6c68ec413

Comment 200

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8db6695b991
https://hg.mozilla.org/mozilla-central/rev/c68772362633
https://hg.mozilla.org/mozilla-central/rev/dd2ad6863f53
https://hg.mozilla.org/mozilla-central/rev/2942bb31152c
https://hg.mozilla.org/mozilla-central/rev/5b8583070c9e
https://hg.mozilla.org/mozilla-central/rev/baa6c68ec413

Comment 201

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/86730d0a8209 for breaking local mochitests.

Comment 202

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r=ahal

Review commit: https://reviewboard.mozilla.org/r/49311/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49311/

Comment 203

[Byron Campen [:bwc]]

Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/19-20/

Comment 204

[Byron Campen [:bwc]]

Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/17-18/

Comment 205

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/20-21/

Comment 206

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/29-30/

Comment 207

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/7-8/

Comment 208

[Andrew Halberstadt [:ahal]]

Comment on attachment 8746218 [details]
MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r=ahal

https://reviewboard.mozilla.org/r/49311/#review46535

Looks good, though the variable name is off. r+ with that fixed.

::: testing/mochitest/runtests.py:826
(Diff revision 1)
>          self.log.info("runtests.py | websocket/process bridge pid: %d"
>                        % self.websocketProcessBridge.pid)
>  
>          # ensure the server is up, wait for at most ten seconds
>          for i in range(1,100):
> +            if self.websocketprocessbridge.proc.poll() is not None:

This needs to be camel cased.

Comment 209

[Byron Campen [:bwc]]

Comment on attachment 8746218 [details]
MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49311/diff/1-2/

Comment 210

[Byron Campen [:bwc]]

Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/21-22/

Comment 211

[Byron Campen [:bwc]]

Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/30-31/

Comment 212

[Byron Campen [:bwc]]

Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/8-9/

Comment 213

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74ad7376f37f
https://hg.mozilla.org/integration/mozilla-inbound/rev/aea71aa8552a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e15d5f65aa01
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c84dc97efb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6686a6d555ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa1c5a6dae55

Comment 214

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74ad7376f37f
https://hg.mozilla.org/mozilla-central/rev/aea71aa8552a
https://hg.mozilla.org/mozilla-central/rev/e15d5f65aa01
https://hg.mozilla.org/mozilla-central/rev/18c84dc97efb
https://hg.mozilla.org/mozilla-central/rev/6686a6d555ee
https://hg.mozilla.org/mozilla-central/rev/aa1c5a6dae55