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.

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.

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

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).

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

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

What can we do to drive this forward?

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

Yep.

Just want to draw attention to this bug. Lucas, do you want me to take a look?

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

(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

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

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 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?

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 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.

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

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.

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 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?

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.

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

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

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 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.

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?

(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)

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

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.

> and maybe about:finkle (doesn't exist) That looks like a bug. Someone should file.

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.

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

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

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.

(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?

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

...so did you file or not?

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

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

(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 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?

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

filter on [mass-p5]

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

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.

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()

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

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 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.