1106075 - 'Try Again' option from Offline mode error page does not reload the page at 1st attempt

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Ada [:adalucinet]]

Reproducible on:
Firefox 34 beta 11 (Build ID: 20141120192249): 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0
Firefox 34.0.5 (Build ID: 20141126041045): 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0
latest Nightly 36.0a1 (Build ID: 20141127030208): 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:36.0) Gecko/20100101 Firefox/36.0
latest Aurora 35.0a2 (Build ID: 20141127004008): 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0

Steps to reproduce:
1. Launch Firefox.
2. Go to File -> Work Offline and perform a search.
3. Click on Try Again button.

Expected results: The online mode is active again (Work Offline is not checked) and the page reloads.

Actual results: The online mode is active, but the page does not reload.

Note:
1. Reproducible also on Ubuntu 14.04 x32, but *not* on Windows 7 64-bit (2 different machines) nor Windows 8.1 64-bit.
2. Screencast: http://goo.gl/lkepwa
3. Regression range:
(m-c):
Last good revision: 38ecfc3922b8 (2014-07-03)
First bad revision: e8df6826a571 (2014-07-04)
Pushlog:https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38ecfc3922b8&tochange=e8df6826a571

(m-i):
Last good revision: 94f150f5b21f
First bad revision: 1022c98a62d2
Pushlog:https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=94f150f5b21f&tochange=1022c98a62d2

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Requesting tracking for 35 as this is a serious regression, but it's probably too late for 34...


This looks like bug 989875... Jim/Felipe, can you take a look?

Comment 2

[Jim Mathies [:jimm]]

I wouldn't consider this a "serious regression", if you hit the try again button twice, you switch back to online mode. Just the first click fails.

Comment 3

[Jim Mathies [:jimm]]

We end up here in e10s mode, and we flip this offline pref - 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2609

but for some reason it doesn't take affect immediately.

Need to check non-e10s next.

Comment 4

[Jim Mathies [:jimm]]

same for non-e10s.

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=80c1259623c3

Comment 6

[Jim Mathies [:jimm]]

try push, this time with mochitests - 

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f438a414acf6

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8538076 [details] [diff] [review]
patch

>       case "Browser:NetworkError":
>-        // Reset network state, the error page will refresh on its own.
>-        Services.io.offline = false;
>+        if (Services.io.offline) {
>+          // Reset network state and refresh the page.
>+          Services.io.offline = false;
>+          msg.target.reload();
>+        }

>+  onAboutNetError: function (event, documentURI) {
>+    let elmId = event.originalTarget.getAttribute("id");
>+    if (elmId != "errorTryAgain" || !/e=netOffline/.test(documentURI)) {
>       return;
>     }
>+    // browser front end will handle clearing offline mode and refreshing
>+    // the page *if* we're in offline mode now. Otherwise let the error page
>+    // handle the click.
>+    if (Services.io.offline) {
>+      event.preventDefault();
>+    }
>     sendSyncMessage("Browser:NetworkError", {});

Why send this message at all when not offline? Also, can we rename that message to something clearer? It asks for a specific action and it's not about a generic "network error", as far as I can tell.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8538536 [details] [diff] [review]
patch

tested for e10s and non-e10s, appears to be working.

Comment 10

[Jim Mathies [:jimm]]

Note the reason why we needed this originally is that the set offline call over PContent is async.

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#448

So even though the set online call from front content.js is sent sync, the io services doesn't alert the content process until later.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Jim Mathies [:jimm] from comment #10)
> Note the reason why we needed this originally is that the set offline call
> over PContent is async.
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#448
> 
> So even though the set online call from front content.js is sent sync, the
> io services doesn't alert the content process until later.

So is there any point in Browser:EnableOnlineMode still being sent synchronously?

Comment 12

[Jim Mathies [:jimm]]

(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Jim Mathies [:jimm] from comment #10)
> > Note the reason why we needed this originally is that the set offline call
> > over PContent is async.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#448
> > 
> > So even though the set online call from front content.js is sent sync, the
> > io services doesn't alert the content process until later.
> 
> So is there any point in Browser:EnableOnlineMode still being sent
> synchronously?

No. I thought about changing this, but sync sends from content to chrome are completely acceptable, so, I didn't change it.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8538536 [details] [diff] [review]
patch

(In reply to Jim Mathies [:jimm] from comment #12)
> > So is there any point in Browser:EnableOnlineMode still being sent
> > synchronously?
> 
> No. I thought about changing this, but sync sends from content to chrome are
> completely acceptable, so, I didn't change it.

Still seems like a good idea to be async by default, though, and only sync when the client code actually needs that. So r=me with sendAsyncMessage instead of sendSyncMessage

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch r=Dão

updated!

new try push: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=37ee80f9d8de

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 8539271 [details] [diff] [review]
patch r=Dão

Approval Request Comment
[Feature/regressing bug #]:
bug 989875
[User impact if declined]:
buggy 'try again' button for netwerk error pages when in offline mode.
[Describe test coverage new/current, TBPL]:
try runs on mc
[Risks and why]: 
low
[String/UUID change made/needed]:
none

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8539271 [details] [diff] [review]
patch r=Dão

busted tests

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

This definitely sounds annoying and we should try to get it fixed as high up as risk/reward make possible but since it already exists in release builds and does work on 2nd attempt, it's less pressing as a tracking issue.  Please nominate for uplift if the fix is low risk.

Comment 18

[Jim Mathies [:jimm]]

Attachment: fix test

This fixes the non-e10s test, and it also goes a long way toward fixing the e10s test. Unfortunately working on this exposed a weird bug down in our socket transport code related to running tests which needs to be fixed before the e10s test can be enabled again.

Comment 19

[Jim Mathies [:jimm]]

Turns out the failures in bug 1113829 are common to all sorts of debug tests when e10s is enabled. I'm going to land this with the non-e10s test update and worry about bug 1113829 later.

Comment 20

[Jim Mathies [:jimm]]

Attachment: patch plus non-e10s test update

Fore changes to browser_bug435325.js.

Comment 21

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=367ea3cb778c

Comment 22

[Jim Mathies [:jimm]]

Attachment: patch r=Dão

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/46dbdedeb31c

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46dbdedeb31c

Comment 25

[[e-mail]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Firefox/38.0
Bug Patched verified , May 30,2015