976426 - Progress bar stuck on neterror page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Attachment: device-2014-02-24-213543.png

The progress bar shows up on 404 pages.

Comment 1

[Lucas Rocha (:lucasr)]

It seems we don't get a STOP event on neterror page. Brian, could you have a look?

Comment 2

[Aaron Train [:aaronmt]]

See also similar for about:pages, I filed yesterday bug 976144.

Comment 3

[Aaron Train [:aaronmt]]

Sylvestre, we decide on release tracking during our mobile triage.

Comment 4

[Brian Nicholson (:bnicholson)]

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.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Filter out-of-order progress updates

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

Comment 6

[Lucas Rocha (:lucasr)]

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.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: (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.

Comment 8

[Lucas Rocha (:lucasr)]

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.

Comment 9

[Brian Nicholson (:bnicholson)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2062136555d

Comment 11

[Brian Nicholson (:bnicholson)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/299cd6f19cde