463046 - Network connectivity checking breaks running netwerk unit tests and mochitests with no connection

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jeff Walden [:Waldo]]

During time spent on flights recently I did most of the hacking to create the patch in bug 462864.  Naturally, I didn't have a net connection.  When I went to find the first obvious bugs in the patch by running just the httpd.js tests in netwerk/test/httpserver/test, I discovered after gdbing that netwerk was trying to be smart, determining at startup that no net connection was present, and shutting down the socket transport service.  With that shut down, the asyncListen method on server sockets used in the HTTP server would throw an exception, and the tests would fail.

This isn't exactly a common scenario, but is there some way to make netwerk not try to be too smart here?  This change means anyone hacking offline on a Mac and trying to run tests on patches is in for a rude surprise, even though all of it would work perfectly fine if it were allowed to do so.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This made it impossible for me to run mochitests today as well...

Comment 2

[(not currently active) Ted Mielczarek]

FWIW, bug 424626 added a pref "toolkit.networkmanager.disable" to override this behavior on Linux:
http://hg.mozilla.org/mozilla-central/annotate/c04e27e056ba/toolkit/system/dbus/nsNetworkManagerListener.cpp

Comment 3

[Dave Camp (:dcamp)]

Attachment: v1

I wonder why this hasn't bitten us on Windows before..

Attached patch adds a pref to turn off link detection, and uses it in mochitests.  xpcshell and the mochitest server just set up the ioservice manually.

Reftests were mentioned when we discussed this on irc, so I did something similar to the xpcshell change there.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 355662 [details] [diff] [review]
v1

>diff -r 91fd1000d75d layout/tools/reftest/reftest-cmdline.js

>+    /* Ignore the platform's online/offline status while running reftests. */
>+    var ios = Cc["@mozilla.org/network/io-service;1"]
>+              .getService(Ci.nsIIOService2)) {
>+    ios.manageOfflineStatus = false;
>+    ios.offline = false;

Copy-paste error above.


>diff -r 91fd1000d75d netwerk/base/src/nsIOService.cpp

>+    if (!pref || strcmp(pref, MANAGE_OFFLINE_STATUS_PREF) == 0) {
>+        PRBool manage;
>+        nsresult rv = prefs->GetBoolPref(MANAGE_OFFLINE_STATUS_PREF, &manage);
>+        if (NS_SUCCEEDED(rv)) {
>+            SetManageOfflineStatus(manage);
>         }
>     }

The temporary doesn't seem necessary, just wrap the call in the NS_SUCCEEDED, and the single-line bracing seems unstylish in this code.

Otherwise, this looks good.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Where does the IOService get this pref on startup?

Comment 6

[Dave Camp (:dcamp)]

PrefsChanged() is called at startup (right after the observer is hooked up)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 355662 [details] [diff] [review]
v1

Ah, ok.

Comment 8

[Dave Camp (:dcamp)]

Attachment: review comments fixed

Comment 9

[Dave Camp (:dcamp)]

Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5eeab4033c20

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 355692 [details] [diff] [review]
review comments fixed

Bug 446300 moved
/tools/test-harness/xpcshell-simple/
...

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 355692 [details] [diff] [review]
review comments fixed

This doesn't feel like it's really needed for the 191 branch, which will be going out of primary development in a few months.

Renominate if you feel strongly, and express why!