960746 - Back/forward button state can get out of sync on tablets

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Brian Nicholson (:bnicholson)]

This is a variant of the bug reported by Chenxia at https://bugzilla.mozilla.org/show_bug.cgi?id=917896#c34. Testing these steps on Nightly, we determined bug 917896 was not the cause.

STR:
1) Go to several pages in a row (2+), then click back repeatedly to go back to about:home
2) Repeatedly tap the forward button quickly
3) Go back to about:home again

On about:home, the back button is normally grayed out. After these steps, however, the back button is still active on about:home, and nothing happens when it's clicked. We probably have a race condition somewhere causing the history stack count to get out of sync.

Comment 1

[Aaron Train [:aaronmt]]

Similarly I notice that we lose forward sometimes when doing:

i) On about:home, visit mozilla.com
ii) Hit back and forward a few times

After a few back and forward navigations you'll be on about:home and the forward button will missing.

Comment 2

[Lucas Rocha (:lucasr)]

This is potentially a regression caused by some of the refactorings I landed in 29. But a regression range would be useful here.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

In 28 on my Galaxy Tab 3, after navigating to the first page, the back button grayed out so you cannot return to about:home. Hitting the back button closes the browser: related?

Otherwise, I cannot repro (but the device is too slow for me to be sure if it's possible or not).

Comment 4

[Flaviu Cos]

The bug is not a regression, it is reproducible even on 2012 builds.

Comment 5

[Lucas Rocha (:lucasr)]

Given that this is not a refression, I think we should reconsider the tracking state here. Renominating.

Comment 6

[Aaron Train [:aaronmt]]

What can we do to drive this forward?

Comment 7

[:Margaret Leibovic]

Looks like this probably isn't going to make 32. Should we re-nom?

Comment 8

[Lucas Rocha (:lucasr)]

Yep.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Just want to draw attention to this bug.

Lucas, do you want me to take a look?

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #9)
> Just want to draw attention to this bug.
> 
> Lucas, do you want me to take a look?

Sure, go ahead.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(Note that there may also be other issues in the code here, but this is just one I found)

It seems the issue is that calling browser.goBack(); twice in succession (e.g. the button is clicked quickly twice [1]) results in two OnHistoryGoBack [2] calls, but only one onLocationChange [3] call and with the first back event's location.

via IRC:

<NeilAway> mcomella: I don't know whether it's Gecko expected behaviour, but I expect all navigation methods to be async except for anchor scrolling, so that if you really want to go back 2 history items you need to use gotoIndex

---

This leaves me with two thoughts on how to fix this:
  1) Queue click events and act after a timeout, using `gotoIndex` to move the appropriate amount of steps back. This is bad because we have to add an artificial wait before responding to user input, and it will break if the user waits long enough and presses back some more.
  2) Wait to receive the onLocationChange call before firing another goBack event. This can be bad because I don't know if waiting for onLocationChange will actually prevent the call from behind hidden and there may be other asynchronous bugs we'd be masking. Additionally, we may also have an artificial delay given we don't know how long it will take to fire onLocationChange. Finally, we don't know that onLocationChange will be called for *all* pages (e.g. about: pages). There is some research to be done here.

It'd also be nice to figure out what desktop does.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=6797921c8f49#1509
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=6797921c8f49#4368
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=6797921c8f49#4192

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It seems desktop also uses `browser.goBack()` directy [1] - I wonder what the difference is.

[1]: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=a7819b6a9411#1669

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (WIP) Fix back button navigation

What do you think of this workaround, Lucas? I actually like the behavior of
this patch because instead of hitting back and being unsure if Android, under
heavy load from your taps, handled your multiple presses correctly (as I
usually feel), it just jumps directly to the page you requested, which seems
cleaner.

For next steps:
  * Fine-tune the delay value (likely a decrease)
  * Combine the back and forward button queues into one object
  * Make the members used by this method "private"
  * Only use the timeout if we know we can go back (or ensure it doesn't matter
if we queue the timeout anyway)
  * How does this interact with page back events
(e.g. `window.history.back()`). Does it matter?

Issues:
  * The browser will only navigate when back is not pressed within 250ms, thus
hitting back forever will cause the browser to never go back. This can be
fixed by immediately initiating navigation when an end index
(e.g. array.length - 1, 0) is reached (though the same race condition could
reappear if both directions are hit, but I expect this use case is unlikely)
  * There is a slight delay before navigating - not much we can do about that
  * The race condition that I believe is causing this issue is only being
covered up with this delay and can presumably be experienced again on slower
devices, or devices under load

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8483158 [details] [diff] [review]
(WIP) Fix back button navigation

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

This really looks like a gecko bug :-/ Consecutive goBack() calls should always lead to the expected behaviour, independently of the async nature of the call under the hood.

The delay approach seems a bit fragile. Maybe you should rely on the history state being committed instead of a delay? i.e. instead of using a delay, use the history listener to figure out whether you have pending events to handle? I'm thinking of something like this:

case "Session:Back":
   if (backCount == 0) {
       browser.goBack()
   }
   backCount++;
   break;

Then in OnHistoryGoBack() you'd do:

OnHistoryGoBack: function(aUri) {
  this._sendHistoryEvent("Back");
  backCount--;
  if (backCount > 0) {
      browser.goBack();
  }
  return true;
}

This way you'd be able to serialize consecutive goBack() calls. The drawback here is a possible delay when quickly tapping on the back button. But at least we wouldn't be adding a delay on the most common interaction (single tap on the back button).

Thoughts?

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I tried out the approach in comment 14, but it doesn't fix the bug - the back/forward buttons can still get out of sync.

Any other ideas, Lucas? If you try out the patch I attached, you'll find the behavior of the toolbar is smoothed out and polished. Despite the delay, I think I prefer the feeling. We can always reduce the delay too. (though I guess this would hurt us on the millisecond for millisecond page load tests)

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8483158 [details] [diff] [review]
(WIP) Fix back button navigation

I really don't want to create a queue for BACK or FORWARD buttons.

Comment 17

[Lucas Rocha (:lucasr)]

Clearing NI request based on our discussion on IRC. It seems we want to try throttling the UI instead.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As per IRC, I have disabled the back button until we can re-enable it. Finkle originally mentioned "page load", but I think until location change is enough to fix the asynchronous issue underlying this bug and where I reattached the listener. Note also that this patch only affects the back button's short click.

Disabled, but not appearing disabled: https://people.mozilla.org/~mcomella/apks/mcomella-960746_01.apk
Fully disabled: https://people.mozilla.org/~mcomella/apks/mcomella-960746_02.apk

Lucas, what do you think?

I like the first one, but I'm not sure how to mimic a slow-loading page to see how it affects the feeling.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (WIP) Back button disabled, but not visibly

The patch for the first build in comment 18, for any one curious. Notoe that I assume we have to disable both the forward and back buttons when one of them is pressed to fix this bug.

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 8485789 [details] [diff] [review]
(WIP) Back button disabled, but not visibly

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

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +131,5 @@
>      private ThemedImageButton tabsButton;
>      private ImageButton backButton;
>      private ImageButton forwardButton;
>  
> +    private final View.OnClickListener backButtonOnClickListener;

Maybe import View.OnClickListener directly to make this a little more concise?

@@ +1317,5 @@
>  
>          button.setEnabled(enabled);
>      }
>  
>      public void updateBackButton(Tab tab) {

Does is make sense to restore the back button listener any time updateBackButton is called? Would it be safer to only restore it on LOCATION_CHANGE so that our intent is more obvious here?

Comment 21

[Lucas Rocha (:lucasr)]

Yeah, I prefer the first one UX-wise too. Just wondering about cases where this approach (restoring click listener on locationchange) could backfire and leave the back button disabled forever.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #20)

> >      public void updateBackButton(Tab tab) {
> 
> Does is make sense to restore the back button listener any time
> updateBackButton is called? Would it be safer to only restore it on
> LOCATION_CHANGE so that our intent is more obvious here?

I assume we call updateBackButton from multiple code paths for a reason. Might have something to do with edge case states where LOCATION_CHANGE is not called correctly, for some reason. I think keeping the change in updateBackButton is probably safest. Let' make sure this change alone does not cause some regressions before trying to optimize it a bit more.

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #22)
> (In reply to Lucas Rocha (:lucasr) from comment #20)
> 
> > >      public void updateBackButton(Tab tab) {
> > 
> > Does is make sense to restore the back button listener any time
> > updateBackButton is called? Would it be safer to only restore it on
> > LOCATION_CHANGE so that our intent is more obvious here?
> 
> I assume we call updateBackButton from multiple code paths for a reason.
> Might have something to do with edge case states where LOCATION_CHANGE is
> not called correctly, for some reason. I think keeping the change in
> updateBackButton is probably safest. Let' make sure this change alone does
> not cause some regressions before trying to optimize it a bit more.

Careful. The reason updateBackButton() is called from multiple code paths now is not the same than the case we're trying to fix here. To be more specific, this bug might still happen if the START event is triggered before LOCATION_CHANGE (can't remember the expected order of the events now). Please double-check.

I'm fine with going ahead with this approach if there are no obvious regressions. Let's just make sure this new behaviour is well documented for future reference.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Disable navigation buttons when they are hit until page load

I moved resetting the listeners into the final phase of onTabChanged, which
"Always runs afterwards". [1] It handles the 

Not that while this is no longer relevant, START is called before
LOCATION_CHANGE, seemingly all the time under normal page load.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=b28df3d24a87#576

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8486870 [details] [diff] [review]
Disable navigation buttons when they are hit until page load

Sorry, just found a bug if the stop button is pressed between navigation button taps.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Disable navigation buttons when they are hit until page load

I tried enabling the buttons only on the STOP, SELECTED, and LOCATION_CHANGE
TabEvents but the issue remained (you have to press more sporadically to repro,
which is why I didn't realize this with ealier patches). Waiting for the first
two appears to work, however, which is what this patch does.

STOP indicates the page has finished loading (whether that's by hitting stop or
completed page load) but does not require the page to finish rendering, so
waiting for this event does not feel too terrible, while SELECTED is for when
the current tab changes (including tab addition and removal). However, I think
we should implement the history hint soon, as per bug 1065187.

Note that I'm unsure of what LOAD_ERROR indicates as it does not seem to be
called on 404 and a disabled network - do you know if STOP covers these cases?

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #26)

> Note that I'm unsure of what LOAD_ERROR indicates as it does not seem to be
> called on 404 and a disabled network - do you know if STOP covers these
> cases?

LOAD_ERROR is fired for things like trying to navigate to aboot:config (bad scheme) and maybe about:finkle (doesn't exist)

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Disable navigation buttons when they are hit until page load

Should have done more homework: LOAD_ERROR appears to be called when a
JavaScript exception is thrown while loading a URI [1]. Theoretically, this
means STOP does not have to get called, so I added it to the TabEvents that
will enable the navigation buttons.

Also added comments on why I chose to use those particular events.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=bf52063ed95f#969

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

For edification:

(In reply to Mark Finkle (:mfinkle) from comment #27)
> LOAD_ERROR is fired for things like trying to navigate to aboot:config (bad
> scheme)

LOAD_ERROR is not fired in this case.

> and maybe about:finkle (doesn't exist)

But LOAD_ERROR is fired here.

Comment 30

[Richard Newman [:rnewman]]

> and maybe about:finkle (doesn't exist)

That looks like a bug. Someone should file.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 8487608 [details] [diff] [review]
Disable navigation buttons when they are hit until page load

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

Looks good. You have rebasing to do now that larch has been merged to m-c :-)

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +617,5 @@
> +        //
> +        // See `setNavigationButtonListeners` javadoc for more information.
> +        if (msg == TabEvents.STOP ||
> +                msg == TabEvents.SELECTED ||
> +                msg == TabEvents.LOAD_ERROR) {

nit: this indentation looks a bit weird. Maybe align all expressions? Up to you.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #31)
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +617,5 @@
> > +        //
> > +        // See `setNavigationButtonListeners` javadoc for more information.
> > +        if (msg == TabEvents.STOP ||
> > +                msg == TabEvents.SELECTED ||
> > +                msg == TabEvents.LOAD_ERROR) {
> 
> nit: this indentation looks a bit weird. Maybe align all expressions? Up to
> you.

I think having the expressions at the same indentation level as the first line in the block is misleading so I'm going to leave it the way it is.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Disable navigation buttons when they are hit until page load

Rebased - note that I moved the OnClickListeners into classes to keep the constructor short but maintain the final keywords on the members.

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI: I think this would be good to keep this baking on Nightly because it changes essential UX functionality, but it may be worth uplifting too.

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #30)
> > and maybe about:finkle (doesn't exist)
> 
> That looks like a bug. Someone should file.

I'm not sure what about this is a bug - Richard, can you explain, or file yourself?

Comment 37

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #36)

> I'm not sure what about this is a bug - Richard, can you explain, or file
> yourself?

Maybe we should show <https://pbs.twimg.com/profile_images/498588007599837184/bpNnvyjB.jpeg>, maybe pulsing the colors between normal and posterized?

mfinkle would probably also enjoy it if we played <https://www.youtube.com/watch?v=xhrBDcQq2DM> in the background.

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

...so did you file or not?

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d663f978a22

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for turning bug 941746 into a perma-fail.
https://hg.mozilla.org/mozilla-central/rev/31a43e4f93d2

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Update tests' navigation to wait for back/forward buttons to be re-enabled

The problem with the tests was we need to wait for the back button to be re-enabled before we press it again.

try: https://tbpl.mozilla.org/?tree=Try&rev=a818a08cbd7f

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #35)
> NI: I think this would be good to keep this baking on Nightly because it
> changes essential UX functionality, but it may be worth uplifting too.

Actually, we've had this bug for a while and it's a substantial UX change. As long as it lands before/with the new tablet code, I think it's fine not to uplift.

Comment 43

[Lucas Rocha (:lucasr)]

Comment on attachment 8490410 [details] [diff] [review]
Part 2: Update tests' navigation to wait for back/forward buttons to be re-enabled

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

Good.

::: mobile/android/base/tests/BaseTest.java
@@ +830,5 @@
> +         * page load completion event before returning.
> +         *
> +         * @param runnable The Runnable to run.
> +         */
> +        private void runAndWaitForPageLoad(final Runnable runnable) {

Shouldn't we update the UITest framework to do the same? At least file a follow-up as reminder.

@@ +841,5 @@
> +            Tabs.registerOnTabsChangedListener(new OnTabsChangedListener() {
> +                @Override
> +                public void onTabChanged(final Tab tab, final TabEvents msg,
> +                        final Object data) {
> +                    if (msg == TabEvents.STOP) {

I assume it's safe to assume the STOP message is on the tab for which the back button was originally pressed?

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #43)
> Shouldn't we update the UITest framework to do the same? At least file a
> follow-up as reminder.

bug 1068172.

> @@ +841,5 @@
> I assume it's safe to assume the STOP message is on the tab for which the
> back button was originally pressed?

We should also look for LOAD_ERROR for when an error occurs (and STOP does not appear) - this mirrors the page load events we re-enable the buttons on in BrowserToolbar. 

However, this made me do my homework and I found some issues:
  * Navigating via back and forward over an error page does not show the STOP or LOAD_ERROR messages. The best message we can use is LOADED, but I don't know how that interacts with the other page load events 
  * Long-pressing a button and closing the context menu without selecting an option (i.e. staying on the same page) causes the navigation buttons to remain disabled
  * The clickability of the navigation buttons remains on when the listeners are removed so long-pressing the navigation buttons (without listeners) during page load results in the url bar long press context menu being opened (I'm suprised tapping the back button doesn't open about:home, however)

...I guess I have more work to do.

Comment 45

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 46

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

After playing around a bit, I find disabling that the back/forward buttons is infuriating on slow loading pages (I used kotaku.com). I've heard issues raised from users in locations where high-speed internet is not available criticize features for working poorly on slow page loads so if this feels bad to me, it will feel much worse to someone else (especially if they don't know what's going on under the hood).

Anthony, what do you think? [1] Would you agree that disabling the navigation buttons after being tapped, until ~page load, isn't really a viable solution?

Instead, I think we should try to fix the issues raised in comment 11 (which may include digging into the platform code), rather than disabling the navigation buttons. This also has the benefit of not being able to put the browser into a strange edge-case state where the navigation buttons get disabled.

[1]: https://people.mozilla.com/~mcomella/apks/mcomella-960746_03.apk

Comment 47

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Sounds like this is largely a technical issue. But, I talked to Michael and it makes sense to just fix the sync issue as suggested by him.

Comment 48

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

When gBrowser.goBack() is called twice in quick succession on desktop (e.g. in Scratchpad), the browser will only go back one page. On mobile, only one page is gone back but the back/forward buttons are out of sync - this is because we update the back/forward buttons on the OnHistoryGoBack listener [1][2], where the documentation states [3]:

The listener can prevent any action (except adding a new session history entry) from happening by returning false from the corresponding callback method.

Therefore, these listeners do not guarantee that a back/forward event has been correctly triggered as we see when the back button is hit twice in quick succession and the Android state gets out of sync with Gecko - we should move the messages to Android somewhere that is guaranteed to run.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=d63fe263014c#4489
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java?rev=53d7570e20f2#583
[3]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsISHistoryListener#OnHistoryGoBack()

Comment 49

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Seems desktop does their back/forward button enabling/disabling in onLocationChange [1] - I'll try that out.

[1]: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4183

Comment 50

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Sync tab state with gecko state in onLocationChange to update navigation buttons

If you *really* abuse the back/forward buttons, you can still get them out of
sync but I think this is a bug in our animations, as opposed to the underlying
platform - normal usage does not seem to cause issues.

Also, we should move the restore zoom functionality to a stable listener, but
in a different bug (see bug 1097317).

Comment 51

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://tbpl.mozilla.org/?tree=Try&rev=5c522df50776

Comment 52

[Lucas Rocha (:lucasr)]

Comment on attachment 8520980 [details] [diff] [review]
Sync tab state with gecko state in onLocationChange to update navigation buttons

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

I like how simple this looks. Let's see it goes.

Comment 53

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 54

[Ryan VanderMeulen [:RyanVM]]

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