Closed Bug 910893 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 34
Tracking Status
firefox31 --- wontfix
firefox32 --- verified
firefox33 --- verified
firefox34 --- verified
fennec 33+ ---

People

(Reporter: pwd.mozilla, Assigned: wesj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
The button doesn't disable on desktop after a single try, so perhaps location.reload() is coming up empty on fennec?
Assignee: nobody → wjohnston
Looked into this a bit, but haven't found the problem yet. location.reload() fires and doesn't fail or throw. Just nothing happens....
See Also: → 932079
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
Flags: needinfo?(bzbarsky)
Not offhand, no.  This stuff is black invariant-violating voodoo.  :(
Flags: needinfo?(bzbarsky)
OS: Linux → Android
Hardware: x86 → All
tracking-fennec: --- → ?
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.
Assignee: wjohnston → gpascutto
tracking-fennec: ? → 33+
Attached patch PatchSplinter Review
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).
Attachment #8466519 - Flags: review?(margaret.leibovic)
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.
Attachment #8466519 - Flags: review+
Assignee: gpascutto → nobody
Assignee: nobody → wjohnston
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?
(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).
Attachment #8466519 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/integration/fx-team/rev/5e3c1c70e2ed

If this works ok, we should uplift it.
https://hg.mozilla.org/mozilla-central/rev/5e3c1c70e2ed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify+
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.
Attachment #8466519 - Flags: approval-mozilla-beta?
Attachment #8466519 - Flags: approval-mozilla-aurora?
Attachment #8466519 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8466519 [details] [diff] [review]
Patch

beta+
Attachment #8466519 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on latest Aurora/Nightly and Firefox 32 Beta 8 on LG Nexus 4(Android 4.4.4).
Remove the qe-verify flag based on comment 18.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.