If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla1.9.2a1



9 years ago
3 years ago


(Reporter: Waldo, Assigned: dcamp)


Mac OS X

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

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.
This made it impossible for me to run mochitests today as well...
Summary: Network connectivity checking breaks running netwerk unit tests with no connection → Network connectivity checking breaks running netwerk unit tests and mochitests with no connection
FWIW, bug 424626 added a pref "toolkit.networkmanager.disable" to override this behavior on Linux:

Comment 3

9 years ago
Created attachment 355662 [details] [diff] [review]

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.
Assignee: nobody → dcamp
Attachment #355662 - Flags: review?


9 years ago
Attachment #355662 - Flags: review?(jwalden+bmo)
Attachment #355662 - Flags: review?(bzbarsky)
Attachment #355662 - Flags: review?
Attachment #355662 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 355662 [details] [diff] [review]

>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.
Where does the IOService get this pref on startup?

Comment 6

9 years ago
PrefsChanged() is called at startup (right after the observer is hooked up)
Attachment #355662 - Flags: review?(bzbarsky) → review+
Comment on attachment 355662 [details] [diff] [review]

Ah, ok.

Comment 8

9 years ago
Created attachment 355692 [details] [diff] [review]
review comments fixed
Attachment #355662 - Attachment is obsolete: true
Attachment #355692 - Flags: approval1.9.1?

Comment 9

9 years ago
Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5eeab4033c20
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Comment on attachment 355692 [details] [diff] [review]
review comments fixed

Bug 446300 moved
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!
Attachment #355692 - Flags: approval1.9.1? → approval1.9.1-


3 years ago
Duplicate of this bug: 437453
You need to log in before you can comment on or make changes to this bug.