Closed Bug 976426 Opened 6 years ago Closed 6 years ago

Progress bar stuck on neterror page

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox28 --- unaffected
firefox29 --- fixed
firefox30 --- verified
fennec 29+ ---

People

(Reporter: kbrosnan, Assigned: bnicholson)

References

()

Details

Attachments

(2 files, 1 obsolete file)

The progress bar shows up on 404 pages.
It seems we don't get a STOP event on neterror page. Brian, could you have a look?
Assignee: nobody → bnicholson
Flags: needinfo?(bnicholson)
See also similar for about:pages, I filed yesterday bug 976144.
Sylvestre, we decide on release tracking during our mobile triage.
tracking-fennec: ? → 29+
The bug here seems to be that the progress bar is stuck, which is not good. There's also the more minor issue that the progress bar appears in the first place on about: pages, which is bug 976144.
Flags: needinfo?(bnicholson)
Summary: Progress bar visible on neterror page → Progress bar stuck on neterror page
Usually, events are received in the following order for page loads: start, location change, document load, stop. For neterror pages, however, events happen in the following order: start, stop, location change, load.

I'm guessing this happens because of the following (unverified):
* We observe only network StateChange events [1]
* The request starts, but since the location is invalid, we get a stop after without location change/load events
* After the stop, we show about:neterror
* This results in a normal start, location change, document load, and stop, but since this isn't a network request, the start and stop events are ignored

We could try tackling this by tinkering around with StateChange events and changing which ones get forwarded to Java and/or which ones Java filters, but I went with a more generic fix: don't update the progress bar for out-of-order updates. That is, if we've moved past START, we won't go backwards unless we've gotten a new START.

Looking at this code, I also noticed we do setLoadProgress() on the Gecko thread but getLoadProgress() on the UI thread. Fixed this by making mLoadProgress volatile.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#447
Attachment #8385937 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8385937 [details] [diff] [review]
Filter out-of-order progress updates

Review of attachment 8385937 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok but maybe it would be more 'correct' to avoid updating the tab's load progress if LOCATION_CHANGE/LOAD happens while the tab is not in loading state? Just giving f+ for now to get have some discussion before going ahead with this patch.

::: mobile/android/base/Tab.java
@@ +772,5 @@
>       *
>       * @param progressPercentage Percentage to set progress to (0-100)
>       */
>      void setLoadProgress(int progressPercentage) {
> +        // Filter out-of-order events that would make the progress bar go backwards.

Mention that this happens on neterror pages and add bug number for future reference.
Attachment #8385937 - Flags: review?(lucasr.at.mozilla) → feedback+
Both approaches work. I think I prefer the first one since doing this check in LOCATION_CHANGE and LOADED feels kind of arbitrary, and if we were to add additional states, we'd need to add this check in those, too. I don't have a very strong opinion though.
Attachment #8386222 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8386222 [details] [diff] [review]
(Alt fix) Update progress on LOCATION_CHANGE and LOADED only if tab is loading

Review of attachment 8386222 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Created attachment 8386222 [details] [diff] [review]
> (Alt fix) Update progress on LOCATION_CHANGE and LOADED only if tab is
> loading
> 
> Both approaches work. I think I prefer the first one since doing this check
> in LOCATION_CHANGE and LOADED feels kind of arbitrary, and if we were to add
> additional states, we'd need to add this check in those, too. I don't have a
> very strong opinion though.

There are a couple of reasons I prefer this approach:
1. It's more specific to the issue we're trying to cover.
2. It fixes the 'model' instead of the view i.e. the load progress should always move forward in all cases.
3. It's a less 'abstract' fix and documents the actual behaviour of the tab states.

::: mobile/android/base/Tab.java
@@ +637,5 @@
>          setErrorType(ErrorType.NONE);
> +
> +        if (getState() == STATE_LOADING) {
> +            setLoadProgress(LOAD_PROGRESS_LOCATION_CHANGE);
> +        }

Factor out this code into a private method called setLoadProgressIfLoading(int) and document this method explaining why this is necessary for LOCATION_CHANGE and LOAD with a reference to this bug.
Attachment #8386222 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c2062136555d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attachment #8385937 - Attachment is obsolete: true
Comment on attachment 8386222 [details] [diff] [review]
(Alt fix) Update progress on LOCATION_CHANGE and LOADED only if tab is loading

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 917896
User impact if declined: progress bar can be stuck on screen
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none
Attachment #8386222 - Flags: approval-mozilla-aurora?
Attachment #8386222 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.