1088186 - Index out of range in RecentTabsCursorLoader

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

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)

Comment 1

[oscarerades]

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

Comment 2

[Richard Newman [:rnewman]]

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.

Comment 3

[oscarerades]

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

[oscarerades]

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

[veeviek]

Hi, I am new here. Can I proceed with the above steps?

Comment 6

[Aaron Train [:aaronmt]]

I suggest trying out what Richard mentioned in comment #2. Let's get a build of Firefox working on your device first.

Comment 7

[veeviek]

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!

Comment 8

[Richard Newman [:rnewman]]

See Comment 2.

Comment 9

[veeviek]

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

[veeviek]

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

Comment 11

[veeviek]

I am new here, please comment.

Comment 12

[Richard Newman [:rnewman]]

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

[veeviek]

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

Thanks.

Comment 14

[Manu Jain]

Attachment: bug1088186.patch

patch created. please tell if it is correct or not. I have used try catch thing to remove that error.

Comment 15

[Richard Newman [:rnewman]]

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.

Comment 16

[Manu Jain]

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?

Comment 17

[Richard Newman [:rnewman]]

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

Comment 18

[Manu Jain]

Attachment: bug_1088186_v2.patch

added if condition.

Comment 19

[Richard Newman [:rnewman]]

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.

Comment 20

[Manu Jain]

Attachment: bug_1088186_v3.patch

return replaced with continue.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8532805 [details] [diff] [review]
bug_1088186_v3.patch

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

In the future, please just use the checkin-needed keyword. It plays more nicely with our automated bug marking tools.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6271f062572d