Closed
Bug 910893
Opened 8 years ago
Closed 7 years ago
Allow users to "try again" more than once on new error pages.
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox31 wontfix, firefox32 verified, firefox33 verified, firefox34 verified, fennec33+)
VERIFIED
FIXED
Firefox 34
People
(Reporter: pwd.mozilla, Assigned: wesj)
References
Details
Attachments
(1 file)
923 bytes,
patch
|
Margaret
:
review+
BenB
:
review+
Sylvestre
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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
Assignee | ||
Comment 2•8 years ago
|
||
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.
Reporter | ||
Comment 3•8 years ago
|
||
The button doesn't disable on desktop after a single try, so perhaps location.reload() is coming up empty on fennec?
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → wjohnston
Assignee | ||
Comment 4•8 years ago
|
||
Looked into this a bit, but haven't found the problem yet. location.reload() fires and doesn't fail or throw. Just nothing happens....
Assignee | ||
Comment 5•7 years ago
|
||
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)
![]() |
||
Comment 6•7 years ago
|
||
Not offhand, no. This stuff is black invariant-violating voodoo. :(
Flags: needinfo?(bzbarsky)
Updated•7 years ago
|
OS: Linux → Android
Hardware: x86 → All
Updated•7 years ago
|
tracking-fennec: --- → ?
Comment 7•7 years ago
|
||
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.
Updated•7 years ago
|
Assignee: wjohnston → gpascutto
tracking-fennec: ? → 33+
Assignee | ||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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+
Updated•7 years ago
|
Assignee: gpascutto → nobody
Updated•7 years ago
|
Assignee: nobody → wjohnston
Comment 10•7 years ago
|
||
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?
Assignee | ||
Comment 11•7 years ago
|
||
(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).
Updated•7 years ago
|
Attachment #8466519 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5e3c1c70e2ed If this works ok, we should uplift it.
Comment 13•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5e3c1c70e2ed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify+
Assignee | ||
Comment 14•7 years ago
|
||
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?
Updated•7 years ago
|
status-firefox31:
--- → wontfix
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → fixed
Updated•7 years ago
|
Attachment #8466519 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•7 years ago
|
||
Comment on attachment 8466519 [details] [diff] [review] Patch beta+
Attachment #8466519 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•7 years ago
|
||
Verified as fixed on latest Aurora/Nightly and Firefox 32 Beta 8 on LG Nexus 4(Android 4.4.4).
Status: RESOLVED → VERIFIED
Updated•2 months ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•