910893 - Allow users to "try again" more than once on new error pages.

Status: VERIFIED FIXED

(Firefox for Android :: General, defect)

Description

[Paul [pwd]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130821101641

Steps to reproduce:

Not being able to try again more than once is especially annoying on mobile whereby you may try once as you're entering a tunnel and then have to enter the awesomescreen to try again. Given the nature of mobile, spotty connections et al, it makes sense to allow users to try a few times.

Comment 1

[Aaron Train [:aaronmt]]

This isn't new, originally implemented in bug 582048. There's no comment as to why the button disables after one attempt. 

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/netError.xhtml#92

Comment 2

[Wesley Johnston (:wesj)]

Hmm.. This is copied from desktop:

http://mxr.mozilla.org/mozilla-central/source/docshell/resources/content/netError.xhtml#91

but seems to imply location.reload() is failing for some reason.

Comment 3

[Paul [pwd]]

The button doesn't disable on desktop after a single try, so perhaps location.reload() is coming up empty on fennec?

Comment 4

[Wesley Johnston (:wesj)]

Looked into this a bit, but haven't found the problem yet. location.reload() fires and doesn't fail or throw. Just nothing happens....

Comment 5

[Wesley Johnston (:wesj)]

GDB says that we're hitting [1] which implies the DocShell thinks this page is "still loading" i.e. its holding a reference to the "last session history entry". This should be cleared in nsDocShell::Stop() [2], and is the first time Stop() is called when loading the error page. For some reason, I see nsDocShell::Stop() hit again though and that time mLoadType != LOAD_ERROR_PAGE.

bz, you have any insight here before I dig further?

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4645
[2] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4693

Comment 6

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Not offhand, no.  This stuff is black invariant-violating voodoo.  :(

Comment 7

[Ben Bucksch (:BenB)]

Comment 5 might explain why many page loads on Firefox on mobile phones are stalling and never finish unless I *manually* abort and reload/resubmit, several times. This is obviously highly annoying on spotty cell phone networks.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

I hate doing this, but we should probably have done it long ago. This just doesn't disable the button when you click it. Fennec changes its viewport in this case anywhere, so there is some janky feedback that something happened (but even without that, not-disabling seems better than making users feel stuck).

Comment 9

[Ben Bucksch (:BenB)]

Comment on attachment 8466519 [details] [diff] [review]
Patch

I'm not authorized to give review, but FWIW: I think both the general change and the concrete code change (trivial and correct) are good.

Comment 10

[:Margaret Leibovic]

Would it work to manually stop the page load before calling reload? Keeping the button enabled would prevent users from feeling stuck, but would it really be helpful if the button doesn't actually work?

Comment 11

[Wesley Johnston (:wesj)]

(In reply to :Margaret Leibovic from comment #10)
> Would it work to manually stop the page load before calling reload? Keeping
> the button enabled would prevent users from feeling stuck, but would it
> really be helpful if the button doesn't actually work?

Sorry, that comment is pretty confusing. The page does "stop" in the real sense. The throbber goes away. Its all done. There are just some code paths that I expected to be hit that aren't (because we actually load the error from the fast-back cache).

Comment 12

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/5e3c1c70e2ed

If this works ok, we should uplift it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e3c1c70e2ed

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8466519 [details] [diff] [review]
Patch



Approval Request Comment
[Feature/regressing bug #]: Been on error pages for a long time
[User impact if declined]: Can't try again twice on error pages. Frustration galore.
[Describe test coverage new/current, TBPL]: We don't have good test coverage on error pages.
[Risks and why]: The risk is pretty minimal here. This is already broken for users and has been for a few years. This patch just causes us to never disable the button so that the user can go on working. We are painting over a problem, but this makes the user experience not suck while we figure out what it is.
[String/UUID change made/needed]: None.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22da5e9d92e9

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8466519 [details] [diff] [review]
Patch

beta+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7bb962c117df

Comment 18

[u421692]

Verified as fixed on latest Aurora/Nightly and Firefox 32 Beta 8 on LG Nexus 4(Android 4.4.4).

Comment 19

[Stefan Deiac]

Remove the qe-verify flag based on comment 18.