1097098 - crash in java.lang.RuntimeException: Unhandled error for GeckoRequest: Session:GetHistory at org.mozilla.gecko.util.GeckoRequest.onError(GeckoRequest.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-7abcb9ea-ff1e-4421-9259-378b22141110.
=============================================================

java.lang.RuntimeException: Unhandled error for GeckoRequest: Session:GetHistory
	at org.mozilla.gecko.util.GeckoRequest.onError(GeckoRequest.java:89)
	at org.mozilla.gecko.GeckoAppShell$3.handleMessage(GeckoAppShell.java:437)
	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:168)
	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2259)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:370)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:190)

Comment 1

[Aaron Train [:aaronmt]]

* Crash when opening long press back menu. Back button was unresponsive, pressed multiple times. Crash after previous app reappeared. Rnewman

* hitting the back button

Comment 2

[Brian Nicholson (:bnicholson)]

Vivek, want to take this?

Comment 3

[Vivek Balakrishnan[:vivek]]

Yes thanks, I will look into that.

Comment 4

[Brian Nicholson (:bnicholson)]

Vivek says he hasn't been able to reproduce, so marking steps-wanted for some QA help. Bug 1052158 should help us here once it lands since reports will also include the JS stack.

Comment 5

[Brian Nicholson (:bnicholson)]

Vivek, bug 1104053, bug 1103605, and bug 1103409 have stacks showing that we throw at nsISHistory.getEntryAtIndex(), so it looks like we're sending invalid indices somewhere. Might be worth enabling the "always restore tabs" pref, then killing/restoring Fennec while hitting the back button to see if things get out of sync somewhere. Can you see if that helps to reproduce?

Comment 12

[Brian Nicholson (:bnicholson)]

Aaron, do you happen to have any STR?

Comment 13

[Aaron Train [:aaronmt]]

I have not encountered this; the comments in comment #1 are what's detailed here. Is this code called only on invoking the long-press back menu button?

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Aaron Train [:aaronmt] from comment #13)
> I have not encountered this; the comments in comment #1 are what's detailed
> here. Is this code called only on invoking the long-press back menu button?

Yep.

Comment 15

[Vivek Balakrishnan[:vivek]]

Attachment: 1097098.patch

As discussed, a new patch which offloads the from and to Index calculation based on the browser history maintained in js. This will solve the issue with history not in sync between java and js.

Comment 16

[Vivek Balakrishnan[:vivek]]

Attachment: 1097098.patch

Added bug comment and review info.

Comment 17

[Vivek Balakrishnan[:vivek]]

Try run log:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=507a0001498c

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 8531918 [details] [diff] [review]
1097098.patch

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

::: mobile/android/base/BrowserApp.java
@@ +3071,5 @@
>              }
>  
>              Tab tab = Tabs.getInstance().getSelectedTab();
>              if (tab != null  && !tab.isEditing()) {
> +                return tabHistoryController.showTabHistory(TabHistoryController.HistoryAction.ALL);

Please keep tab, then pass the tabId to Gecko.

::: mobile/android/base/tabs/TabHistoryController.java
@@ +41,4 @@
>          JSONObject json = new JSONObject();
>          try {
> +            json.put("action", action.name());
> +            json.put("maxListSize", Tab.MAX_HISTORY_LIST_SIZE);

Since this isn't used anywhere else, you might as well drop this and define MAX_HISTORY_LIST_SIZE in JS instead of Java.

::: mobile/android/chrome/content/browser.js
@@ +1961,5 @@
>      this._prefObservers = newPrefObservers;
>    },
>  
> +  // This method will return a list of history items and toIndex based on the action provided from the fromIndex to toIndex,
> +  // optionally selecting selIndex(if fromIndex<=selIndex<=toIndex)

Nit: add some spaces between words/operators: "selIndex (if fromIndex <= selIndex <= toIndex)"

@@ +1966,4 @@
>    getHistory: function(data) {
> +    let action = data.action;
> +    let maxListSize = data.maxListSize;
> +    let webNav = BrowserApp.selectedTab.window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation);

We should get the tab using the given tabId instead.

@@ +2001,5 @@
> +    } else {
> +      // return empty list immediately.
> +      return { "historyItems" : listitems,
> +               "toIndex" : toIndex
> +             };

Nit: please format this block like so:

return {
  historyItems: listitems,
  toIndex: toIndex,
};

(Removed space before ":", reformatted indentation, and trailing commas on all lines).

@@ +2018,5 @@
>      }
>  
> +    return { "historyItems" : listitems,
> +    	     "toIndex" : toIndex
> +           };

Same here.

Comment 19

[Vivek Balakrishnan[:vivek]]

Attachment: 1097098.patch

Review comments correction.

As discussed added a sanity check in Session:Navigate to deal with index not in sync between JS and Java

Try server run :
https://tbpl.mozilla.org/?tree=Try&rev=dc3bdd37a3a8

Comment 20

[Vivek Balakrishnan[:vivek]]

A new try log:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0c604886211c

Comment 21

[Brian Nicholson (:bnicholson)]

Comment on attachment 8532781 [details] [diff] [review]
1097098.patch

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

Nice, looks good to me. Let's hope this fixes the crash.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27cf288a8f5c