720320 - Starting Firefox with Wifi offline causes stays in offline, and no visual indication of offline mode (regression from turning off manage network offline state)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mark Banner (:standard8)]

In bug 620472, network.manage-offline-status was turned to false.

I don't know why I haven't see this before (I probably haven't done it recently), but I just started Firefox in offline mode, on starting up, I then refreshed some of the tabs that were open. However there was zero indication of Firefox being in offline mode. It took me quite a while (and turning on the web console) to realise that Firefox was offline.

Build id: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0

Quick STR:

1) Have your WiFi connection not connected
2) Start Firefox
3) Connect your WiFi
3) Try to use your existing tabs in Firefox, or load sites you visit frequently.

Actual Results:

Panic because nothing is re-loading, but non-existing tabs load sites from the cache, causing more confusion.

Expected Results:

Firefox should have detected the presence of WiFi.


Comments in Bug 620472 indicate that more bugs should have been filed to turn manage offline state back on after fixing issues. This doesn't appear to have happened - should it?

Maybe this is just bad for me because I didn't realise that Firefox was no longer managing my offline state properly and I didn't expect it to start in offline mode. Thunderbird which still has that pref set to true and manages my online/offline state just fine. So now my Firefox pref is set to true, but this is really bad UX for those just starting up and looking at pages they do regularly.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Having "manage offline state" disabled means that Firefox should never start up in offline mode, AIUI (at least since bug 663253 was fixed). Are you sure that's what's happening? If so, is there any chance you can debug to figure out why? I can't reproduce this...

Comment 2

[Mark Banner (:standard8)]

I reconfirmed today on the Mac, and it is definitely happening, taking a look at the code I think I can see an issue, need to debug a bit.

Comment 3

[Mark Banner (:standard8)]

Attachment: Possible fix

The basic issue is that the initialization routines are causing the network status to be set before we've set the manage offline status. So if we're not managing offline (which the member variable defaults to true), then we initialize the network link service, which sees we're offline and then by the time we set the manage offline status, we're then not in a position where we can put ourselves back out of offline mode.

This patch would fix that, but the only case I'm not really happy with is where we are managed, and we're starting in offline mode. At the moment, that does temporarily put us online until the profile fully kicks in, which seems sub-optimal.

This also seems like it is all kicking in on app-startup or earlier, which is possibly good, but kinda strange that we would need the ioservice that early.

I think this could also gain a couple of unit tests, just by faking the network link services.

I won't have time to work on this for a while though.

Comment 4

[RNicoletto]

Isn't this a DUPE of bug 578239?

Comment 5

[Mark Banner (:standard8)]

(In reply to RNicoletto from comment #4)
> Isn't this a DUPE of bug 578239?

Yes, but I didn't find that when I filed this, so I'll dupe forward in this instance as this one has more information (and a partial patch).

Comment 7

[Honza Bambas (:mayhemer)]

This could be a regression from bug 627672.

I can try to finish this.

Comment 8

[Honza Bambas (:mayhemer)]

I broke this via a review suggestion:

(In reply to Honza Bambas (:mayhemer) from bug 627672 comment #59)
> Comment on bug 627672 attachment 566814 [details] [diff] [review]
> v7

> @@ +1116,2 @@
> >      if (mManageOfflineStatus && !wasManaged)
> >          return TrackNetworkLinkStatusForOffline();
> 
> This code is wrong IMO.  I still think the attribute should throw when we
> are not able to turn manage on (i.e. we don't have the link service) and are
> before profile-do-change.  
> 
> To fix the code as I think is correct would be to do mManageOfflineStatus =
> aManage; *after* call to InitializeNetworkLinkService().  This way we will
> throw when there is no link service.


The logic really is complicated.

Mark, the patch is mostly correct, thanks for the investigation!  

However, it needs some updates:

* the default value of mMangeOfflineStatus needs to be changed from true to false.  When we call SetManageOfflineStatus(true) the first time, we must call to TrackNetworkLinkStatusForOffline() - it won't happen when mMangeOfflineStatus is initially true - to determine we are actually able to track the state.  If not, reset mMangeOfflineStatus to false and throw.  (It's for case users switch the pref to true).

* have an explanation comment to stop anyone else to break this again:
  "SetManageOfflineStatus must throw when we fail to go from non-managed to managed.  Usually because there is no link monitoring service available.  Failure to do this switch is detected by a failure of TrackNetworkLinkStatusForOffline().  When there is no network link available during call to InitializeNetworkLinkService(), application is put to offline mode.  And when we change mMangeOfflineStatus to false on the next line we get stuck on being offline even though the link becomes later available."

Also see http://hg.mozilla.org/mozilla-central/annotate/343ec916dfd5/netwerk/base/src/nsIOService.cpp#l317

Mark, I will update your patch with it and let someone from the net team review, OK?

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1

Christian, please check comment 8 for more explanation on this fix.

Comment 10

[Honza Bambas (:mayhemer)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/08aafcd6eb9c

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/08aafcd6eb9c

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

This patch breaks the online/offline events on Firefox Mobile. I don't know offhand if Firefox desktop is broken too. Backing out this patch makes the events work again.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Instead of just backing this patch out, can we start trying to figure out why the offline/online events are broken by this?

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Sure. Btw. Which version of Firefox Mobile is affected, can you reproduce it with latest nightly? And which is the best version to test?

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

Firefox 13+, though trunk is best to work against.

Comment 16

[Fabrice Bellet]

With Firefox 15 and Fedora 17, I still see a problem with the offline status of Firefox not being updated according to the status reported by NetworkManager.

I suggest the following patch

The idea is to change the mManageOfflineStatus variable only when the user toggles the network.manage-offline-status setting in about:config.

Specifically I think we should not set it to off in nsIOService::InitializeNetworkLinkService() when the mNetworkLinkService is not initialized yet, because 1/ there's no way for the user to know that the value has changed (network.manage-offline-status in about:config doesn't reflext the modification in that case), and 2/ there's no possibility for the variable to come back to the value true, except if the user manually toggles twice the network.manage-offline-status setting.

Similarly in the firefox interface (browser.js). If mManageOfflineStatus is on, and network is offline, firefox status will be offline too. If the user want to access some URL locally (http://localhost/...), he will set firefox online. Later, when the network on his box is back online, it seems natural that firefox status still follows the box connectivity status, which will be possible only if mManageOfflineStatus stays on.

Comment 17

[Fabrice Bellet]

Attachment: limit places where mManageOfflineStatus is modified

Comment 18

[Mark Banner (:standard8)]

Fabrice, thanks for the possible patch, I suggest though that as this bug is fixed you file a fresh bug and attach the patch there - it has generally been found that when comments have been left on closed/resolved bugs, then they are less likely to get followed up and driven through to completion. Thanks.

Comment 19

[Fabrice Bellet]

done, thanks (bz#791626)