1049688 - MOZ_DISABLE_NONLOCAL_CONNECTIONS can not be disabled

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Nils Ohlmeier [:drno]]

Attachment: fix_nonlocal_connections.patch

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

Comment 1

[Julian Seward [:jseward]]

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 2

[Ed Morley [:emorley]]

Comment on attachment 8468550 [details] [diff] [review]
fix_nonlocal_connections.patch

Would you mind looking at this for Nils? :-)

Comment 3

[Nathan Froyd [:froydnj]]

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".

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: bug_1049688_fix_MOZ_DISABLE_NONLOCAL_CONNECTIONS.patch

Updated according to last review.

Comment 5

[Nathan Froyd [:froydnj]]

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...)

Comment 6

[Julian Seward [:jseward]]

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

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: bug_1049688_fix_MOZ_DISABLE_NONLOCAL_CONNECTIONS.patch

Updated according to last feedback.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=37c2a4729e5e

Comment 8

[Nathan Froyd [:froydnj]]

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.

Comment 9

[Nils Ohlmeier [:drno]]

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

Comment 10

[Nils Ohlmeier [:drno]]

Attachment: bug_1049688_fix_MOZ_DISABLE_NONLOCAL_CONNECTIONS.patch

Fixed problems with XPC shell tests.

Carrying forward r+=nfroyd

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4249b4b1f5b5

Comment 11

[Nils Ohlmeier [:drno]]

Try run appears green.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1637aba02543

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/1637aba02543