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

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: aaronmt, Assigned: vivek, Mentored)

Tracking

(Blocks: 1 bug, {crash})

36 Branch
Firefox 37
All
Android
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 affected, fennec36+)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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)
(Reporter)

Comment 1

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

* hitting the back button
Vivek, want to take this?
Flags: needinfo?(vivekb.balakrishnan)
Mentor: bnicholson
tracking-fennec: ? → 36+
(Assignee)

Comment 3

4 years ago
Yes thanks, I will look into that.
Assignee: nobody → vivekb.balakrishnan
Flags: needinfo?(vivekb.balakrishnan)
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.
Keywords: steps-wanted
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?
Flags: needinfo?(vivekb.balakrishnan)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1103409
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1103605
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1104053
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1104736
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1105289
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1105372
(Reporter)

Updated

4 years ago
Status: NEW → ASSIGNED
Aaron, do you happen to have any STR?
Flags: needinfo?(aaron.train)
(Reporter)

Comment 13

4 years ago
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?
Flags: needinfo?(aaron.train)
(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.
(Assignee)

Comment 15

4 years ago
Created attachment 8531913 [details] [diff] [review]
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.
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8531913 - Flags: review?(bnicholson)
(Assignee)

Comment 16

4 years ago
Created attachment 8531918 [details] [diff] [review]
1097098.patch

Added bug comment and review info.
Attachment #8531913 - Attachment is obsolete: true
Attachment #8531913 - Flags: review?(bnicholson)
Attachment #8531918 - Flags: review?(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.
Attachment #8531918 - Flags: review?(bnicholson) → feedback+
(Assignee)

Comment 19

4 years ago
Created attachment 8532781 [details] [diff] [review]
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
Attachment #8531918 - Attachment is obsolete: true
Attachment #8532781 - Flags: review?(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.
Attachment #8532781 - Flags: review?(bnicholson) → review+
(Assignee)

Updated

4 years ago
Keywords: crash, steps-wanted → checkin-needed
(Assignee)

Updated

4 years ago
Keywords: crash
(Assignee)

Updated

4 years ago
Blocks: 1108720
(Assignee)

Updated

4 years ago
Blocks: 732752
https://hg.mozilla.org/integration/fx-team/rev/27cf288a8f5c
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/27cf288a8f5c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.