1233048 - crash in java.lang.NullPointerException: Attempt to invoke interface method ''int android.database.Cursor.getCount()'' on a null object reference at org.mozilla.gecko.preferences.AndroidImport.run(AndroidImport.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u549602]

This bug was filed from the Socorro interface and is 
report bp-b6f9a4e8-c727-47e6-b5d3-3195f2151216.
=============================================================
Device: Samsung Galaxy Note 5 (Android 5.1.1)
Build: All builds 

Steps to reproduce: 

1. Remove  default browser from android settings or disable it 
2. Go to settings- Advance- import options and select both or any option
3. Tap "Import"

Result:
Firefox crashes

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Does this happen in first run too? This would be a bad first impression.

Comment 2

[u549602]

Hi Sebastian,
Yes, this crash also occurs when trying to import at first run.

Comment 3

[Richard Newman [:rnewman]]

One clause has a check for a null cursor:

            if (Build.MANUFACTURER.equals(SAMSUNG_MANUFACTURER) && cursor != null && cursor.getCount() == 0) {
                cursor = mCr.query(SAMSUNG_BOOKMARKS_URI, null, null, null, null);
            }


and one doesn't:

            if (Build.MANUFACTURER.equals(SAMSUNG_MANUFACTURER) && cursor.getCount() == 0) {
                cursor = mCr.query(SAMSUNG_HISTORY_URI, null, null, null, null);
            }

This code could do with a little bit of cleanup, too.

Comment 4

[swaroop.rao]

Can I take this up?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to swaroop.rao from comment #4)
> Can I take this up?

Yeah, sure. Attach a bug and it's all yours! :) However as this is a crash.. time is a concern here and someone else might pick it up if it's not moving along.

Have a look at "Bugs ahoy!" or visit us on IRC (irc.mozilla.org #mobile) if you are looking for a bug that is less critical:
http://www.joshmatthews.net/bugsahoy/?mobileandroid=1

Comment 6

[swaroop.rao]

I've got a patch ready but I was unsure about the "little bit of cleanup" part mentioned by rnewman. Is there anything else I should be doing apart from adding the null check?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> Yeah, sure. Attach a bug and it's all yours! :)

Oops. This should say "Attach a patch".. :)

(In reply to swaroop.rao from comment #6)
> I've got a patch ready but I was unsure about the "little bit of cleanup"
> part mentioned by rnewman. Is there anything else I should be doing apart
> from adding the null check?

Avoiding the crash is good enough. While I do not know what rnewman had in mind, here are some things I noticed while reading the class:

* Good: To avoid errors like this we could move the query part from mergeBookmarks() and mergeHistory() to a query() method that first tries to query BOOKMARKS_URI and then SAMSUNG_BOOKMARKS_URI if needed - and then returns the cursor
* Maybe: I think we should query Samsung's bookmarks provider even if the previous query returned a null cursor. So I think it should be: SAMSUNG && (cursor == null || cursor.getCount() == 0)
* Minor: mContext is set but never used
* Minor: Instead of cursor.moveToFirst and while(!cursor.isAfterLast()) we could just use while(cursor.moveToNext())
* Minor: We could return early if the cursor is null after both queries: if (cursor == null) { return }

But again: The crash is important here. It would be nice to clean it up a bit while we edit the file but we can always file a follow-up bug if needed.

Comment 8

[swaroop.rao]

OK, I've made the null check change and also implemented the 'Good' and 'Maybe' changes suggested above. I have created a patch and currently testing it on try.

Comment 9

[swaroop.rao]

Attachment: 1233048.patch

Attaching the patch containing the changes described in earlier comment.

However, I don't have a Samsung phone and I could not reproduce this defect on my Nexus 6 (either before or after the fix).

Try log is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=42ef36717998.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8700873 [details] [diff] [review]
1233048.patch

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

Patch is looking good. But I'd like to get liuche's opinion on the changes (especially the logic change).

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8700873 [details] [diff] [review]
1233048.patch

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

This also looks good to me! And the logic change also makes sense, we should be trying if the cursor is null or empty.

Comment 12

[swaroop.rao]

So, is there anything else needed from me?

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to swaroop.rao from comment #12)
> So, is there anything else needed from me?

No. We can add "checkin-needed" to land this. :)

Comment 14

[:Margaret Leibovic]

Let's try to uplift this, since we're promoting this during first run.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

NI to remember requesting uplift.

Comment 16

[Pulsebot]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d43c0078a51d

Comment 18

[u549602]

Verified as fixed on the latest Nightly build (46.0a1 / 2015-12-27) on Nexus tab 7 with Android 5.1.1. Thank you

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8700873 [details] [diff] [review]
1233048.patch

Approval Request Comment

[Feature/regressing bug #]: Import bookmarks/history feature. Promoted from first-run experience and preferences.

[User impact if declined]: On Samsung devices using the import feature can crash the app in certain situations.

[Describe test coverage new/current, TreeHerder]: Only manual testing.

[Risks and why]: Low - Added missing null check.

[String/UUID change made/needed]: -

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700873 [details] [diff] [review]
1233048.patch

Crash fix that was verified, beta44+, aurora45+

Comment 21

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd19c7203ec3

Comment 22

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

This isn't applying cleanly to beta, mostly because Bug 1107811 wasn't uplifted to beta. Can we get a rebased patch to work around that?

Comment 23

[:Margaret Leibovic]

I changed the paths in the patch and it applied cleanly to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/28d49ead7a22

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/28d49ead7a22

Comment 25

[u549602]

Verified as fixed on latest Aurora build (45.0a2 07-01-2016), using a Nexus 4 with Android 5.1.1

Comment 26

[u549602]

Verified as fixed on the latest Beta build (44.0b7), using a Nexus 4 with Android 5.1.1