If I use 'mach' to execute mochitests I can currently not overwrite MOZ_DISABLE_NONLOCAL_CONNECTIONS and prevent a crash when accessing external services. Yes I know that I'm not suppose to write test cases which access the Internet. I'm not planing on executing them on TBPL. The code from bug 995417 only looks for the existence of the MOZ_DISABLE_NONLOCAL_CONNECTIONS, so the only way to disable would be to not have that variable set at all. The actual value of the variable does not matter for the current implementation. Combined with simply overwriting the MOZ_DISABLE_NONLOCAL_CONNECTIONS in build/automationutils.py this results in no way of disabling the crash on Internet access on my local dev environment. The attached patch only sets the MOZ_DISABLE_NONLOCAL_CONNECTIONS if it is not set already. And the new code in nsSocketTransport2.cpp only enables the crash if MOZ_DISABLE_NONLOCAL_CONNECTIONS has a value different then 0. Essentially the attached patch allows me to disable the crash by setting MOZ_DISABLE_NONLOCAL_CONNECTIONS to 0

The current behaviour of mochitests renders the suite entirely useless if you happen to be unfortunate enough to have to operate in an environment with misconfigured DNS that routes all unresolvable names to something like this: http://navigationshilfe1.t-online.de/ +1 for landing this patch or something equivalent.

Comment on attachment 8468550 [details] [diff] [review] fix_nonlocal_connections.patch Would you mind looking at this for Nils? :-)

Comment on attachment 8468550 [details] [diff] [review] fix_nonlocal_connections.patch Review of attachment 8468550 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, we probably do need something like this. The idea is right, but the implementation needs a little polish. ::: build/automation.py.in @@ +510,5 @@ > else: > env['MOZ_CRASHREPORTER_DISABLE'] = '1' > > # Crash on non-local network connections. > + env.setdefault('MOZ_DISABLE_NONLOCAL_CONNECTIONS', '1') There should be a comment here describing why we're using setdefault. ::: build/automationutils.py @@ +510,5 @@ > else: > env['MOZ_CRASHREPORTER_DISABLE'] = '1' > > # Crash on non-local network connections. > + env.setdefault('MOZ_DISABLE_NONLOCAL_CONNECTIONS', '1') And here. And in the two other places we set this in the tree: http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#865 http://mxr.mozilla.org/mozilla-central/source/build/mobile/remoteautomation.py#76 ::: netwerk/base/src/nsSocketTransport2.cpp @@ +1175,5 @@ > nsSocketTransport::InitiateSocket() > { > SOCKET_LOG(("nsSocketTransport::InitiateSocket [this=%p]\n", this)); > > + const char *conEnvChar = getenv("MOZ_DISABLE_NONLOCAL_CONNECTIONS"); This is going to call getenv every time through this function, which is bad. @@ +1177,5 @@ > SOCKET_LOG(("nsSocketTransport::InitiateSocket [this=%p]\n", this)); > > + const char *conEnvChar = getenv("MOZ_DISABLE_NONLOCAL_CONNECTIONS"); > + static int conEnvVal = 0; > + if (conEnvChar) { In the same vein, if MOZ_DISABLE_NONLOCAL_CONNECTIONS is set, we're going to parse it every time through this function, which seems undesirable. @@ +1178,5 @@ > > + const char *conEnvChar = getenv("MOZ_DISABLE_NONLOCAL_CONNECTIONS"); > + static int conEnvVal = 0; > + if (conEnvChar) { > + conEnvVal = atoi(conEnvChar); This is admittedly nitpicky, but using atoi here means that things like MOZ_DISABLE_NONLOCAL_CONNECTIONS=blah will turn off the crash code, which is at odds with your stated intent. It's probably better to test explicitly for the string "0".

Updated according to last review.

Comment on attachment 8473897 [details] [diff] [review] bug_1049688_fix_MOZ_DISABLE_NONLOCAL_CONNECTIONS.patch Review of attachment 8473897 [details] [diff] [review]: ----------------------------------------------------------------- Definitely better! Just a few more things to fix up. ::: build/automation.py.in @@ +510,5 @@ > else: > env['MOZ_CRASHREPORTER_DISABLE'] = '1' > > # Crash on non-local network connections. > + # Setdefault allows to turn this of if set to 0. (This comment applies to all the setdefault calls in this patch.) Nit: "turn this off" But this isn't really explaining why we'd want to do this; I think it's worth being more explicit here than the WebRTC logging comment below. Maybe a comment like: "MOZ_DISABLE_NONLOCAL_CONNECTIONS can be set to "0" to temporarily enable non-local connections for the purposes of local testing. Don't override the user's choice here. See bug 1049688." ::: netwerk/base/src/nsSocketTransport2.cpp @@ +1176,5 @@ > { > SOCKET_LOG(("nsSocketTransport::InitiateSocket [this=%p]\n", this)); > > + static int conEnvVal = -1; > + if (conEnvVal == -1) { I think this would be easier to follow if you had: static int crashOnNonLocalConnections = -1; if (crashOnNonLocalConnections == -1) { const char* s = getenv(...); ... } @@ +1179,5 @@ > + static int conEnvVal = -1; > + if (conEnvVal == -1) { > + const char *conEnvChar = getenv("MOZ_DISABLE_NONLOCAL_CONNECTIONS"); > + if (conEnvChar) { > + conEnvVal = strncmp(conEnvChar, "0", 1); Please use !!strncmp(...) or an explicit comparison, so it's obvious that we don't care about all three possible return values. (Ensuring that conEnvVal is non-negative here also means that we don't have to worry about re-entering this bit of code via the condition above...)

Pretty please can this be moved along? It's inconvenient to have to patch the tree every time I want to run mochitests.

Updated according to last feedback. Try run: https://tbpl.mozilla.org/?tree=Try&rev=37c2a4729e5e

Comment on attachment 8485711 [details] [diff] [review] bug_1049688_fix_MOZ_DISABLE_NONLOCAL_CONNECTIONS.patch Review of attachment 8485711 [details] [diff] [review]: ----------------------------------------------------------------- We should probably update the documentation string that gets printed at the crash to alert people to being able to set MOZ_DISABLE_NONLOCAL_CONNECTIONS=0 for themselves. But I'm fine with doing that in a followup bug.

Looks like we don't have an env available in the xpcshell tests. I'll have to look into how to work around that....

Fixed problems with XPC shell tests. Carrying forward r+=nfroyd Try run: https://tbpl.mozilla.org/?tree=Try&rev=4249b4b1f5b5

Try run appears green.