nsIDOMWindowUtils.goOnline() is going away (see bug 775868), so we need to remove it's usage. netError.xhtml uses it when it detect a page load error was due to working offline, and as a convenience will go online when you hit "retry" (at least, this is true on desktop). I talked to wesj on IRC about this, and we don't think this will impact mobile users - so it's safe to just remove the call, and not need to handle it elsewhere like bug 779680 is doing.

It is possible for Necko to "think" it is offline on a mobile device. We can get into this situation if we have no Wifi or Cell Data active. In fact, it's not uncommon to see this error page on mobile devices. In several places in the code we use: Services.ios.offline = false; We do this to "kick" Necko into assuming we are not stuck offline when attempting to load a URL. We should test the situation, because I'm not sure pressing the "Try Again" will be enough to "kick" Necko without the goOnline() call. If it is not, we could listen for the "click" in browser.js, like we do with the Cert Error page (see: getMeOutOfHere()) and use the | Services.ios.offline = false | trick.

Comment on attachment 650771 [details] [diff] [review] Patch v1 mfinkle, you can take the review here if you want. My original take was that we shouldn't need the kick, and if we do we should figure out the platform problem and fix it. i.e. we're currently wallpapering over a problem. But tracking down the problems and fixing them is likely going to take awhile running this with a larger audience. Maybe this is they type of patch we send to Aurora but back out before it hits beta? Blair, what was the desktop impetus for this? Do they feel like this offline-while-online error state will never happen?

(In reply to Wesley Johnston (:wesj) from comment #3) > Blair, what was the desktop impetus for this? Do they feel like this > offline-while-online error state will never happen? The situation is a bit different on desktop, as you can explicitly choose to go into offline mode (Firefox menu -> Web Developer -> Work Offline, or main menu -> File -> Work Offline). (In reply to Mark Finkle (:mfinkle) from comment #2) > If it is not, we could listen for the "click" in browser.js, like we do with > the Cert Error page (see: getMeOutOfHere()) and use the | > Services.ios.offline = false | trick. This is pretty much how desktop handles it now. I can whip up a mobile patch that does that easy enough (seems easy enough to do for XUL Fennec too), that would keep the existing behaviour (I'm away from home for a few more weeks though, so testing mobile stuff is difficult for me at the moment). (In reply to Wesley Johnston (:wesj) from comment #3) > Maybe this is they type of patch > we send to Aurora but back out before it hits beta? nsIDOMWindowUtils.goOnline needs to go away as soon as possible, and that removal may get uplifted to Aurora.

(In reply to Blair McBride (:Unfocused) from comment #4) > (In reply to Mark Finkle (:mfinkle) from comment #2) > > If it is not, we could listen for the "click" in browser.js, like we do with > > the Cert Error page (see: getMeOutOfHere()) and use the | > > Services.ios.offline = false | trick. > > This is pretty much how desktop handles it now. I can whip up a mobile patch > that does that easy enough (seems easy enough to do for XUL Fennec too), > that would keep the existing behaviour (I'm away from home for a few more > weeks though, so testing mobile stuff is difficult for me at the moment). Yeah. I think we should make such a patch. I'll see if someone from mobile can pick it up. We can use desktop as a guide. Also, If you get a chance to make a first pass of the patch, we can do the testing.

Comment on attachment 650771 [details] [diff] [review] Patch v1 I'd like a more complete patch so we don't regress our "get back online" support.

Submitting patches without even trying them, LIKE A BOSS. (Don't do this at home.)

Comment on attachment 651970 [details] [diff] [review] Patch v2 Flagging this just so it doesn't get lost. Note that capturing event listeners are needed so we go online before the page attempts to reload (ditto with the need for a sync message).

Is someone able to pick this up? Just needs verification the patch works, and a test. (I'm home now, but utterly swamped - so given that I'd need to catch up on mobile testing, I have no idea when I'd be able to get to it.)

Comment on attachment 651970 [details] [diff] [review] Patch v2 This looks good to me. Assigning to Brian for the final testing. Also CC'ing Matt for the Metro port.

> nsIDOMWindowUtils.goOnline needs to go away as soon as possible, and that removal may get uplifted to Aurora. I think the removal of goOnline(), whenever it lands, probably won't be backported, so you don't need to worry about that aspect of it. Thanks everybody for working on this!

I tested the first patch, and the "try again" button works fine without the "Browser:GoOnline" message. This is what I'm doing: * Start Fennec * Put phone in airplane mode * Go to some page, see offline error message * Turn off airplane mode * Hit "try again" several times until it works (i.e., once the radio is back up) Likewise, after going from offline->online, entering another URL or search automatically works again without having to first hit the "try again" button. So do we need this "Browser:GoOnline" change?

(In reply to Brian Nicholson (:bnicholson) from comment #12) > Likewise, after going from offline->online, entering another URL or search > automatically works again without having to first hit the "try again" > button. So do we need this "Browser:GoOnline" change? Note that the "Browser:GoOnline" message is for XUL Fennec. Native Fennec works fine without needing such a message. I should have said that only the Native Fennec changes need to be tested. The XUL Fennec changes look fine, but should only be used by Firefox Metro as a basis for porting.

(In reply to Mark Finkle (:mfinkle) from comment #13) > (In reply to Brian Nicholson (:bnicholson) from comment #12) > Note that the "Browser:GoOnline" message is for XUL Fennec. Native Fennec > works fine without needing such a message. > Oops, thanks - didn't notice that. Both patches (the obsolete one and the newer one) work fine for me using using the steps in comment 12, so I think setting 'Services.io.offline = false;' is unnecessary.

Comment on attachment 651970 [details] [diff] [review] Patch v2 I take that back - removing it from the posted patch doesn't work.

> Assignee: bnicholson@mozilla.com → nobody@mozilla.org What's the status here? Does this mean this is unowned?

(In reply to Bobby Holley (:bholley) from comment #16) > > Assignee: bnicholson@mozilla.com → nobody@mozilla.org > > What's the status here? Does this mean this is unowned? Sorry, I should've given this back to Blair. This is ready to be checked in.

Comment on attachment 651970 [details] [diff] [review] Patch v2 Well ok then! https://hg.mozilla.org/integration/fx-team/rev/353a5837b555

Hooray! Thanks for fixing these, Unfocused!

Comment on attachment 651970 [details] [diff] [review] Patch v2 Review of attachment 651970 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +3922,5 @@ > errorDoc.location.reload(); > } else if (target == errorDoc.getElementById("getMeOutOfHereButton")) { > errorDoc.location = "about:home"; > } > + } else if (/^about:neterror\?e=netOffline/.test(ownerDoc.documentURI)) { ownerDoc is not defined here (you probably intended errorDoc, which is defined as errorDoc = target.ownerDocument; above). Because of this bug, this code both doesn't ever do anything, and will prevent other error handlers in this function from working correctly. Please back out and re-consider if you really need this code both here and in XUL.