Index out of range in RecentTabsCursorLoader

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: rnewman, Assigned: Manu Jain, Mentored)

Tracking

Trunk
Firefox 37
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Got this testing locally. This shouldn't happen. Likely easy to solve via code inspection.

E/GeckoSessionParser(12965): JSON error
E/GeckoSessionParser(12965): org.json.JSONException: Index -1 out of range [0..0)
E/GeckoSessionParser(12965): 	at org.json.JSONArray.get(JSONArray.java:282)
E/GeckoSessionParser(12965): 	at org.json.JSONArray.getJSONObject(JSONArray.java:510)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.SessionParser.parse(SessionParser.java:68)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.home.RecentTabsPanel$RecentTabsCursorLoader.loadCursor(RecentTabsPanel.java:325)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:44)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:26)
E/GeckoSessionParser(12965): 	at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:242)
E/GeckoSessionParser(12965): 	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:51)
E/GeckoSessionParser(12965): 	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:40)
E/GeckoSessionParser(12965): 	at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:123)
E/GeckoSessionParser(12965): 	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
E/GeckoSessionParser(12965): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
E/GeckoSessionParser(12965): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
E/GeckoSessionParser(12965): 	at java.lang.Thread.run(Thread.java:864)
E/GeckoSessionParser(12965): JSON error
E/GeckoSessionParser(12965): org.json.JSONException: Index -1 out of range [0..0)
E/GeckoSessionParser(12965): 	at org.json.JSONArray.get(JSONArray.java:282)
E/GeckoSessionParser(12965): 	at org.json.JSONArray.getJSONObject(JSONArray.java:510)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.SessionParser.parse(SessionParser.java:68)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.home.RecentTabsPanel$RecentTabsCursorLoader.loadCursor(RecentTabsPanel.java:325)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:44)
E/GeckoSessionParser(12965): 	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:26)
E/GeckoSessionParser(12965): 	at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:242)
E/GeckoSessionParser(12965): 	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:51)
E/GeckoSessionParser(12965): 	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:40)
E/GeckoSessionParser(12965): 	at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:123)
E/GeckoSessionParser(12965): 	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
E/GeckoSessionParser(12965): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
E/GeckoSessionParser(12965): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
E/GeckoSessionParser(12965): 	at java.lang.Thread.run(Thread.java:864)
(Reporter)

Updated

3 years ago
Mentor: rnewman@mozilla.com
Whiteboard: [good first bug][lang=java]

Comment 1

3 years ago
Hi. I am new here and I would be interested in fixing this bug. Where do I start?

Updated

3 years ago
Flags: needinfo?(rnewman)
(Reporter)

Comment 2

3 years ago
Get started by checking out the Fennec source and building:

https://wiki.mozilla.org/Mobile/Fennec/Android

Take a look at this page to figure out how to create patches:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ

Once you're at the stage of having a working build of your own, you'll want to take a look at SessionParser.java. There's a coding error somewhere around line 68.

Ideally the fix will be accompanied by a test, but let's get to the stage of a working build first.
Flags: needinfo?(rnewman)

Comment 3

3 years ago
Thank you for the information. I am still downloading and building firefox. I expect to have it all prepared for tomorrow. My internet conection is very slow...

Comment 4

3 years ago
I will not be able to download the mozilla build this week due to internet issues. So I think I will download all and then I will begin fixing bugs. So at the moment I am not interested in this bug. Sorry and thank you for your help.

Comment 5

3 years ago
Hi, I am new here. Can I proceed with the above steps?
I suggest trying out what Richard mentioned in comment #2. Let's get a build of Firefox working on your device first.

Comment 7

3 years ago
So far I have followed the instructions on this page:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build#Get_the_source

I have this build ruuning on my pc. How do I proceed from here?

Thanks!
(Reporter)

Comment 8

3 years ago
See Comment 2.

Comment 9

3 years ago
The sessionStrings variable which is parsed as a JSON object is read from the file sessionstore.js as below in my test run.
{
  "windows": [
    {
      "tabs": [
        {
          "entries": [
            {
              "url": "about:home",
              "title": "Nightly Start Page",
              "ID": 2,
              "docshellID": 5,
              "docIdentifier": 2
            }
          ],
          "lastAccessed": 1415084649457,
          "hidden": false,
          "attributes": {},
          "image": "chrome://branding/content/icon32.png",
          "index": 1
        }
      ],
      "selected": 1,
      "_closedTabs": [],
      "width": "1395",
      "height": "764",
      "screenX": "59",
      "screenY": "288",
      "sizemode": "maximized",
      "title": "Nightly Start Page",
      "_shouldRestore": true,
      "closedAt": 1415084649461
    }
  ],
  "selectedWindow": 0,
  "_closedWindows": [],
  "session": {
    "lastUpdate": 1415084649543,
    "startTime": 1415084641563,
    "recentCrashes": 0
  },
  "global": {}
}

The code looks fine, the index value of zero will give the exception. What do you suggest can be done here.

Thanks

Comment 10

3 years ago
I am sorry I think I have erred by using the Firefox build for desktop.

Comment 11

3 years ago
I am new here, please comment.
(Reporter)

Comment 12

3 years ago
Folks have meetings, and live in different time zones. Please don't expect immediate responses to questions.

You need to look at Comment 2 and get a build running for Android. Fennec also parses your session store in Java, hence the stack trace in Comment 0.

Comment 13

3 years ago
I have the fennec build running now. I need to look at the code where the sessionstore is created.

Thanks.
(Assignee)

Comment 14

3 years ago
Created attachment 8531648 [details] [diff] [review]
bug1088186.patch

patch created. please tell if it is correct or not. I have used try catch thing to remove that error.
Attachment #8531648 - Flags: review?(rnewman)
(Reporter)

Comment 15

3 years ago
Comment on attachment 8531648 [details] [diff] [review]
bug1088186.patch

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

I don't think this is the correct fix.

The issue arises when some part of the input state violates our success-case assumptions. For example, if .getInt("index") returns 0, this will throw. If .getJSONArray("entries") returns a shorter array than `index`, this will throw. And theoretically, if the JSON is malformed -- there's no "tabs" array, the value of "index" isn't an int -- anywhere in the expression can throw.

We want truly malformed to throw, and violated assumptions to not, IMO.

That means we want sane range checks here, rather than just a catch block.
Attachment #8531648 - Flags: review?(rnewman) → review-
(Assignee)

Comment 16

3 years ago
Ok, you mean we doesn't want the error to be thrown when .getInt("index") return 0 or .getJSONArray("entries") returns shorter array than index. Except these two conditions we want the error to be thrown. So, I think if-else are needed here for range checks. Am I right?
(Reporter)

Comment 17

3 years ago
It's probably adequate to simply check something like this:

  if (index < 1 || entries.length() < index) {
      Log.w(LOGTAG, "Session entries and index don't agree.");
      return;        // As appropriate.
  }
(Assignee)

Comment 18

3 years ago
Created attachment 8532056 [details] [diff] [review]
bug_1088186_v2.patch

added if condition.
Attachment #8531648 - Attachment is obsolete: true
Attachment #8532056 - Flags: review?(rnewman)
(Reporter)

Comment 19

3 years ago
Comment on attachment 8532056 [details] [diff] [review]
bug_1088186_v2.patch

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

::: mobile/android/base/SessionParser.java
@@ +67,5 @@
>                      final int index = tab.getInt("index");
> +                    final JSONArray entries = tab.getJSONArray("entries");
> +                    if (index < 1 || entries.length() < index) {
> +                        Log.w(LOGTAG, "Session entries and index don't agree.");
> +                        return;

continue; rather than return;. Don't abort processing tabs just because one was malformed.
Attachment #8532056 - Flags: review?(rnewman) → review-
(Assignee)

Comment 20

3 years ago
Created attachment 8532805 [details] [diff] [review]
bug_1088186_v3.patch

return replaced with continue.
Attachment #8532056 - Attachment is obsolete: true
Attachment #8532805 - Flags: review?(rnewman)
(Reporter)

Updated

3 years ago
Attachment #8532805 - Flags: review?(rnewman)
Attachment #8532805 - Flags: review+
Attachment #8532805 - Flags: checkin?
(Reporter)

Updated

3 years ago
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment on attachment 8532805 [details] [diff] [review]
bug_1088186_v3.patch

https://hg.mozilla.org/integration/fx-team/rev/6271f062572d
Attachment #8532805 - Flags: checkin? → checkin+
In the future, please just use the checkin-needed keyword. It plays more nicely with our automated bug marking tools.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6271f062572d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.