636148 - Upgrading to 4.0 from 3.6 with offline state auto detected to true leaves the user offline since 4.0 no longer manages offline state

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Srinivas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.98 Safari/534.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13

After Pave over to 4.0 the browser is in Offline mode ("Work offline" from the menu is checked) even if it was initially in Online mode (in 3.6)

Reproducible: Always

Steps to Reproduce:
1.Install Firefox 3.6.13 or 3.6.14
2.Download and use the profile from http://dl.dropbox.com/u/13932234/WinXP/FirefoxWinXP_Rich_Profile_3.6.14.zip
3.Open Fx 3.6.* and make sure it is in online mode.
4.Now install Firefox 4 Beta 11 and open Fx 4 beta 11

Actual Results:  
After doing this Pave over to 4.0 the browser is in Offline mode ("Work offline" from the menu is checked) even if it was initially in Online mode (in 3.6)

Expected Results:  
Online mode in Fx 4b11

Comment 1

[juan becerra [:juanb]]

Of the few bugs we've found while testing major updates with rich profiles, this is one that might fit the criteria. Nominating.

Comment 2

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

Was this due to the changes we made about offline mode?

Comment 3

[Asa Dotzler [:asa]]

that would be bug 620472

Comment 4

[Robert Strong (they/them - no direct email)]

Likely due to bug 620472... cc'ing fryn since he worked on this code and I'm looking to see if I can figure out what's going on.

Comment 5

[Robert Strong (they/them - no direct email)]

(In reply to comment #0)
The profile has the following prefs

This one states you are running 3.6.14
user_pref("extensions.lastAppVersion", "3.6.14");

This one states you are already in offline mode
user_pref("browser.offline", true);

Just a guesss... I suspect what might be going on here is that 3.6.14 auto-detected you are in offline mode and after the upgrade to 4.0 it doesn't switch to online since there is no auto-detection for offline mode with 4.0.

Comment 6

[Robert Strong (they/them - no direct email)]

I *think* the best / safest solution besides backing out bug 620472 is to change the name of the browser.offline preference. Perhaps browser.isOffline?

Comment 7

[Frank Yan (:fryn)]

(In reply to comment #5)
> Just a guesss... I suspect what might be going on here is that 3.6.14
> auto-detected you are in offline mode and after the upgrade to 4.0 it doesn't
> switch to online since there is no auto-detection for offline mode with 4.0.

I think that this is exactly what is happening.

(In reply to comment #6)
> I *think* the best / safest solution besides backing out bug 620472 is to
> change the name of the browser.offline preference. Perhaps browser.isOffline?

I agree. I don't think it makes sense to back out bug 620472, since there aren't technically any new bugs in the code itself.

Changing the pref name seems most reasonable. Writing more migration code unnecessarily adds cruft.

browser.isOffline sounds good to me. Rob, do you want to write the patch, or shall I?

Comment 8

[Robert Strong (they/them - no direct email)]

Just about done.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Frank, could you do a once over of these changes? I believe this covers everything (searched comm-central) but chatzilla which also reads the browser.offline preference (I'll file a followup for that if we go with this approach). From a quick once over of nsDBusService.h it appears the comment is incorrect... if it is, I'd prefer it was fixed in a followup bug since fixing the comment isn't a hardblocker.

Comment 10

[Frank Yan (:fryn)]

Comment on attachment 514639 [details] [diff] [review]
patch rev1

(In reply to comment #9)

Looks fine to me. Thanks for investigating and making the patch. :)

Roc would be the best person to ask about that comment.

Comment 11

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

I don't understand why the browser.offline pref exists at all. Why can't we just remove it entirely?

Comment 12

[Robert Strong (they/them - no direct email)]

So when a user manually sets their status to offline it is retained across sessions. I personally think it would be a good thing to remove it and just require manually changing it but I also don't think it is a good idea to do so this late in the game.

Comment 13

[Frank Yan (:fryn)]

(In reply to comment #12)
> So when a user manually sets their status to offline it is retained across
> sessions. I personally think it would be a good thing to remove it and just
> require manually changing it but I also don't think it is a good idea to do so
> this late in the game.

I don't think it adds more risk to remove the persistence though. We actually enter online mode before we check the pref to go into offline mode anyway. Now that Gavin mentions it, I'd be happy with just removing the pref too.

Comment 14

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

Bug 312793's comments/patch helped explain the logic. I'll need to think about this a bit more tomorrow.

(What do you think is wrong with the nsDBusService.h comment?)

Comment 15

[Robert Strong (they/them - no direct email)]

From a quick scan and search of nsDBusService (and friends) it doesn't set the pref as the comment states.

Comment 16

[Robert Strong (they/them - no direct email)]

As far as risk is concerned, no one thought there was much risk with bug 620472 yet it resulted in two regressions one of which it was initially backed out for and this one which wasn't found until almost a month later. Without knowing this specific code changing the pref name seems safest to me.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 514639 [details] [diff] [review]
patch rev1

>--- a/js/src/tests/e4x/Regress/regress-308111.js
>+++ b/js/src/tests/e4x/Regress/regress-308111.js

There's no reason to keep this file in sync with actual prefs.

Comment 18

[Robert Strong (they/them - no direct email)]

Agreed but it doesn't hurt and mxr searches won't end up returning the old pref name in this file. Personally, I'd prefer that file didn't contain actual pref names at all so it wouldn't show up in mxr searches but barring that I prefer consistency.

Comment 19

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

The particular failure case here seems somewhat unlikely - someone would have had to explicitly "switch offline" at some point to get browser.offline to be false, right? I suppose that could have happened "a long time ago" before bug 312793 landed (which made us ignore the pref value when the link service was enabled), but it still doesn't seem particularly likely to occur much in practice?

Another option that would be simpler is just clearing any user values for browser.offline on upgrade, assuming that we think "user has just upgraded" is a valid reason to reset the user-set value for that pref (we seem to)...

Comment 20

[santoshpc]

Can't you just check for the user's current state then reset preferences and switch it back to the original user's preference for browser.offline?

Comment 21

[Robert Strong (they/them - no direct email)]

(In reply to comment #19)
> The particular failure case here seems somewhat unlikely - someone would have
> had to explicitly "switch offline" at some point to get browser.offline to be
> false, right? I suppose that could have happened "a long time ago" before bug
> 312793 landed (which made us ignore the pref value when the link service was
> enabled), but it still doesn't seem particularly likely to occur much in
> practice?
Possibly but the bug as reported states that Firefox was online which makes me worried that there is some edgecase where the auto-detection switched them to offline in 3.6 prior to restart.

> Another option that would be simpler is just clearing any user values for
> browser.offline on upgrade, assuming that we think "user has just upgraded" is
> a valid reason to reset the user-set value for that pref (we seem to)...
Not sure how that would be simpler since it would have to perform a check and iirc the only place that could be done is in nsBrowserContentHandler.js where it checks browser.startup.homepage_override.mstone which itself can be overridden with a value of ignore.

Comment 22

[Frank Yan (:fryn)]

(In reply to comment #19)
> Another option that would be simpler is just clearing any user values for
> browser.offline on upgrade, assuming that we think "user has just upgraded" is
> a valid reason to reset the user-set value for that pref (we seem to)...

I mentioned in comment 7 that this requires writing migration code, which in this case is unnecessary cruft, i.e. after Firefox 4, it will be completely unlikely for users to end up in offline mode without intending to be, so there will be no need for any migration code to run again upon a further upgrade.

Comment 23

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

(In reply to comment #21)
> Possibly but the bug as reported states that Firefox was online

Yeah, I'm not sure I believe that. Or rather, I'm not sure how this profile would have gotten into this state in any normal scenario.

> iirc the only place that could be done is in nsBrowserContentHandler.js where
> it checks browser.startup.homepage_override.mstone which itself can be
> overridden with a value of ignore.

Yeah, that's where I was thinking of doing it. I don't see that "ignore" thing as a problem.

Comment 24

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

(In reply to comment #22)
> I mentioned in comment 7 that this requires writing migration code

Not really - we already do the needHomePageOverride stuff.

> this case is unnecessary cruft, i.e. after Firefox 4, it will be completely
> unlikely for users to end up in offline mode without intending to be

Well theoretically that isn't possible right now either. The only indication of it happening is this one profile that for some reason had the pref set to true. It seems kind of over-reactive to start making widespread changes without any knowledge of how this can happen in practice, or how widespread it would be.

Comment 25

[Robert Strong (they/them - no direct email)]

Gavin, what do you want to do with this bug?

Comment 26

[Robert Strong (they/them - no direct email)]

btw: I have tried several times to reproduce and have been unable to. As I see it, changing the pref name will avoid releasing with a bug that might or might not exist with very little risk compared to any other approaches suggested so far.

Comment 27

[LegNeato]

Moving to .x. We'll approve it if it gets fixed in time for FF4.

Comment 28

[Robert Strong (they/them - no direct email)]

(In reply to comment #27)
> Moving to .x. We'll approve it if it gets fixed in time for FF4.
Did you intentionally leave the hardblocker keyword?

Comment 29

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

Changing the pref name can have costs; e.g. third parties may be trying to set/read this pref, and people with a user value for browser.offline will end up having both prefs show up in about:config, which could be confusing. Granted neither of those are major issues (the former may also be unlikely).

I think I'd rather do nothing about this bug, at least until we have a better idea of how this can happen in practice. Where does the profile from comment 0 come from? Does anyone else have a 3.6 profile that has this pref set to false under normal conditions?

Comment 30

[LegNeato]

(In reply to comment #28)
> (In reply to comment #27)
> > Moving to .x. We'll approve it if it gets fixed in time for FF4.
> Did you intentionally leave the hardblocker keyword?

Nope, apologies.

To expand on why we moved this to .x...

#1 - Can't reproduce
#2 - You have to be in offline mode in 3.6 and then pave over, which probably doesn't happen that often
#3 - It is temporary and easily fixed by the end user

Comment 31

[AndreiD[QA]]

Works For Me on:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11

*Note: The issue was reproduceable if updating from 3.6.14 to 4.0, but it seems that cannot be reproduced on a pave over. Please see attachments in bug 636221 (online vs offline without any user fixes in the menu)

Comment 33

[Steffen Wilberg]

Fixed by bug 663253: The offline state is no longer persisted across sessions.

Comment 34

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0a2) Gecko/20110707 Firefox/7.0a2