627332 - add-ons compatibility check always presents "Minefield is working offline" dialog when network link state detection is disabled

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre ID:20110119030331

With the patch landed on bug 620472, we now always display the "Minefield is working offline" on the first start after switching from Firefox 3.6.

Steps:
1. Create a profile with Firefox 3.6 (make sure you are offline)
2. Use the latest Minefield build with this profile

In step 2 you will see the offline dialog. That's wrong because the online mode worked well in step 1.

Asking for blocking because it's a strange experience for the major upgrade.

Comment 1

[Frank Yan (:fryn)]

(In reply to comment #0)
> Steps:
> 1. Create a profile with Firefox 3.6 (make sure you are offline)
> 2. Use the latest Minefield build with this profile
> 
> In step 2 you will see the offline dialog. That's wrong because the online mode
> worked well in step 1.

What do you mean by "make sure you are offline"?
Does this mean to check the box "Work Offline" in the profile manager, turn off AirPort, or something else?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Wow, that's not what I wanted to write. Too much offline/online words here. Sorry.

So here the steps again:
1. Create a profile with Firefox 3.6 (don't go offline)
2. Use the latest Minefield build with this profile

Comment 3

[Frank Yan (:fryn)]

(In reply to comment #2)
> So here the steps again:
> 1. Create a profile with Firefox 3.6 (don't go offline)
> 2. Use the latest Minefield build with this profile

I can't reproduce this.
I've tried the following steps:

1. Open Firefox 3.6 Profile Manager (.../firefox-bin -p).
2. Create a new profile named 'test'.
3. Select the profile 'test'.
4. Click 'Start Firefox'.
5. After Firefox browser window finishes loading, quit Firefox.
6. Open Minefield (m-c 20100120) Profile Manager (.../firefox-bin -p).
7. Select the profile 'test'.
8. I don't even see the updater wizard, and Minefield opens immediately.

1. Open Firefox 3.6 Profile Manager (.../firefox-bin -p).
2. Create a new profile named 'test2'.
3. Click 'Quit'.
4. Open Minefield (m-c 20100120) Profile Manager (.../firefox-bin -p).
5. Select the profile 'test2'.
6. I don't even see the updater wizard, and Minefield opens immediately.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, you will have to install an extension first. It's definitely related to the add-ons compatibility check. Without it the offline dialog doesn't pop-up.

Comment 5

[Frank Yan (:fryn)]

(In reply to comment #4)
> Ok, you will have to install an extension first. It's definitely related to the
> add-ons compatibility check. Without it the offline dialog doesn't pop-up.

Alright, now I can reproduce this. Looks like a hard blocker to me. I have a possible fix and am waiting for it to compile, so I can test it.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I hit this just updating nightlies.

Comment 8

[Frank Yan (:fryn)]

Bug 627601 has blocking-betaN+ and was duped to this bug.
Could a driver set the flag here?

Comment 9

[Dave Townsend [:mossop]]

This needs to block beta 10 which code freezes tomorrow at 2. If there isn't a quick fix then we'll just back out bug 620472

Comment 10

[Frank Yan (:fryn)]

Attachment: patch

While fixing the bug, the patch retains the behavior that if you quit Firefox with "Work Offline" manually checked to true, it will persist that state upon the next startup.

Comment 11

[Frank Yan (:fryn)]

Henrik, I just pushed this to tryserver, so builds will be up in a few hours. I've tested the patch locally, and it works for me, but could you verify that the bug is fixed with the patch?

Comment 12

[Frank Yan (:fryn)]

Attachment: simpler workaround

Same patch as above, except without the part that disables auto-detection for all Gecko applications by default, since Mossop recommended that "Yeah if it is really unreliable then we probably should do that at some point, but not after api freeze ;)".

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I believe roc is away for a few days per his blog.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #11)
> Henrik, I just pushed this to tryserver, so builds will be up in a few hours.
> I've tested the patch locally, and it works for me, but could you verify that
> the bug is fixed with the patch?

Can you please specify which build you wanna have tested? You should add at least the bug number as description if possible. For now I assume it's http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-fc4980d25bb4/

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so the default path works. When I start this tryserver build while staying in online mode, the offline dialog doesn't appear, and we correctly checking for add-on compatibility.

But when I go offline in Firefox 3.6 and start the tryserver build, we are also searching for compatibility updates and dont' bring up the offline window. When the browser is started, it's correctly in offline mode.

Comment 16

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

This is no longer a blocker, given that bug 620472 was backed out.

Comment 17

[Frank Yan (:fryn)]

(In reply to comment #14)
> Can you please specify which build you wanna have tested? You should add at
> least the bug number as description if possible. For now I assume it's
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-fc4980d25bb4/

I added the bug number in the commit message; is there somewhere else I should add it? The tryserver build directory had not yet appeared, and I had forgotten that it's just named after the changeset. I will include the directory next time. Thanks.

(In reply to comment #15)
> Ok, so the default path works. When I start this tryserver build while staying
> in online mode, the offline dialog doesn't appear, and we correctly checking
> for add-on compatibility.
> 
> But when I go offline in Firefox 3.6 and start the tryserver build, we are also
> searching for compatibility updates and dont' bring up the offline window. When
> the browser is started, it's correctly in offline mode.

Hmm, I suppose it's best to wait until roc gets back. Two questions we should consider are: Is being in offline mode when upgrading to 4.0 to be likely caused by an auto-detection false negative? If so, should we set the user back to online mode by default on the first run after upgrading to 4.0?

Comment 18

[Frank Yan (:fryn)]

Comment on attachment 505680 [details] [diff] [review]
simpler workaround

This patch isn't doing what we want. What we need is for Services.io.offline (aka mOffline) to report the correct value by the time the Add-ons updater accesses it. Currently, when mManageOfflineStatus is false, mOffline just reports true everytime. My patch just made it report false everytime, with the side effect of initializing things that should be initialized if it will set to true again when actually working offline. I'm not sure how to do this; unassigning self.

Comment 19

[Dave Townsend [:mossop]]

The problem I think is that the IO Service isn't currently responsible for caching the user-set offline state across instances of the app. Instead right now Firefox restores that state late in startup well after it is possible for things like the add-ons manager to actually make network connections.

Seems like it would be better for the IO service to cache it instead so it can restore it the moment it is initialised.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What about just setting mOffline to false in nsIOService's constructor, and removing the SetOffline(PR_FALSE) in nsIOService::Init? It seems to me that that will fix everything here: we'll be online unless and until either the network link service or the browser.offline pref takes us offline.

Comment 21

[Frank Yan (:fryn)]

(In reply to comment #20)
> What about just setting mOffline to false in nsIOService's constructor, and
> removing the SetOffline(PR_FALSE) in nsIOService::Init? It seems to me that
> that will fix everything here: we'll be online unless and until either the
> network link service or the browser.offline pref takes us offline.

I tried setting mOffline to false in the constructor, and it seemed to cause the following security manager error prompt upon shutdown: https://mxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pipnss/pipnss.properties#386

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's weird. I think we should debug that to make progress here.

Comment 23

[Frank Yan (:fryn)]

(In reply to comment #21)
> (In reply to comment #20)
> > What about just setting mOffline to false in nsIOService's constructor, and
> > removing the SetOffline(PR_FALSE) in nsIOService::Init? It seems to me that
> > that will fix everything here: we'll be online unless and until either the
> > network link service or the browser.offline pref takes us offline.
> 
> I tried setting mOffline to false in the constructor, and it seemed to cause
> the following security manager error prompt upon shutdown:
> https://mxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pipnss/pipnss.properties#386

(In reply to comment #22)
> That's weird. I think we should debug that to make progress here.

It seems like SetOffline assumes that mOffline is true the first time that it's called and does something to make the state of everything else consistent. I'll run gdb on that once I have time, but I won't this week.

> > It seems to me that
> > that will fix everything here: we'll be online unless and until either the
> > network link service or the browser.offline pref takes us offline.

Leaving mOffline to true in the constructor and calling SetOffline(PR_FALSE) in Init (like attachment 505680 [details] [diff] [review] does) accomplishes this too. Since browser.offline doesn't take us offline until after the add-ons manager update wizard, this means that the add-ons manager will always try to make connections to update add-ons, which I think is okay, since we don't have the correct value until delayed startup anyway. If you concur that the workaround in attachment 505680 [details] [diff] [review] is sufficient, let's check that in and give the users suffering from bug 620472 a big UX win.

Comment 24

[Frank Yan (:fryn)]

Pushed.

https://hg.mozilla.org/mozilla-central/rev/b5b1e065bc0e

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre