Closed Bug 697528 Opened 11 years ago Closed 11 years ago

java.lang.NullPointerException on AwesomeBarTabs$HistoryQueryTask.onPostExecute(AwesomeBarTabs.java:249)

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: aaronmt, Assigned: kats)

References

()

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(2 files, 2 obsolete files)

E/GeckoApp(20612): top level exception
E/GeckoApp(20612): java.lang.NullPointerException
E/GeckoApp(20612): 	at org.mozilla.gecko.AwesomeBarTabs$HistoryQueryTask.onPostExecute(AwesomeBarTabs.java:249)
E/GeckoApp(20612): 	at org.mozilla.gecko.AwesomeBarTabs$HistoryQueryTask.onPostExecute(AwesomeBarTabs.java:121)
E/GeckoApp(20612): 	at android.os.AsyncTask.finish(AsyncTask.java:417)
E/GeckoApp(20612): 	at android.os.AsyncTask.access$300(AsyncTask.java:127)
E/GeckoApp(20612): 	at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:429)
E/GeckoApp(20612): 	at android.os.Handler.dispatchMessage(Handler.java:99)
E/GeckoApp(20612): 	at android.os.Looper.loop(Looper.java:130)
E/GeckoApp(20612): 	at org.mozilla.gecko.GeckoApp$20.run(GeckoApp.java:1103)
E/GeckoApp(20612): 	at android.os.Handler.handleCallback(Handler.java:587)
E/GeckoApp(20612): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoApp(20612): 	at android.os.Looper.loop(Looper.java:130)
E/GeckoApp(20612): 	at android.app.ActivityThread.main(ActivityThread.java:3691)
E/GeckoApp(20612): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoApp(20612): 	at java.lang.reflect.Method.invoke(Method.java:507)
E/GeckoApp(20612): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:907)
E/GeckoApp(20612): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:665)
E/GeckoApp(20612): 	at dalvik.system.NativeStart.main(Native Method)

STR:

1. Clear History
2. Back out of preferences, tap AwesomeBar

--
20111026113310
http://hg.mozilla.org/projects/birch/rev/5a87519d6173
Samsung Galaxy SII
bp-25426f5e-e7ff-4308-a6bd-ced032111026
Crash Signature: mozalloc_abort | __swrite
Assignee: nobody → kgupta
Attached patch Fix for the NPE (obsolete) — Splinter Review
This fixes the NPE, but exposes some other issues that are observable when history is cleared.
Attachment #569794 - Flags: review?(lucasr.at.mozilla)
Attached patch Fix for exposed issues (obsolete) — Splinter Review
Remove unvisited bookmarks from the awesomebar history tab, and also remove the dummy "Bookmarks" entry that shows up when history is empty.
Attachment #569796 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 569796 [details] [diff] [review]
Fix for exposed issues

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

Patch is mostly good, just want the nits fixed and questions answered before r+'ing it.

::: embedding/android/AwesomeBarTabs.java
@@ +89,5 @@
>          protected Cursor doInBackground(Void... arg0) {
>              ContentResolver resolver = mContext.getContentResolver();
>  
>              return resolver.query(Browser.BOOKMARKS_URI,
>                                    null,

Please add a comment here explaining why the LENGTH(URL) is necessary in this query.

@@ +129,5 @@
>          protected Cursor doInBackground(Void... arg0) {
>              ContentResolver resolver = mContext.getContentResolver();
>  
>              return resolver.query(Browser.BOOKMARKS_URI,
>                                    null,

Same here. Explain in which cases the date can be 0.

@@ +344,5 @@
>              public Cursor runQuery(CharSequence constraint) {
>                  ContentResolver resolver = mContext.getContentResolver();
>  
>                  mAllPagesCursor =
>                      resolver.query(Browser.BOOKMARKS_URI,

Don't you need to add paren around "TITLE LIKE ? OR URL LIKE ?" to ensure correctness?
Attachment #569796 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 569794 [details] [diff] [review]
Fix for the NPE

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

::: embedding/android/AwesomeBarTabs.java
@@ +211,2 @@
>              List<Map<String,?>> children = null;
> +            List<Map<String,?>> groups = new LinkedList<Map<String,?>>();

The whole point of not allocating these data structures upfront was to avoid allocating memory that won't be used in case the list is empty.

We'll eventually need to add UI to handle the empty-list case (i.e. showing a "No history records" message or something). For now, I think it would be better to simply not create the SimpleExpandableListAdapter in case the list is empty. Just check if groups == null after we close the cursor and simply call return if that's the case. In the future, we'll just hide the list view and show a message when that happens.
Attachment #569794 - Flags: review?(lucasr.at.mozilla) → review-
This way has more duplicated code because of the way you arranged the loop, but it's your call.
Attachment #569794 - Attachment is obsolete: true
Attachment #569957 - Flags: review?(lucasr.at.mozilla)
Added comments and fixed the operator precedence - good catch on that :)
Attachment #569796 - Attachment is obsolete: true
Attachment #569958 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 569957 [details] [diff] [review]
Fix for the NPE (v2)

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

::: embedding/android/AwesomeBarTabs.java
@@ +319,5 @@
> +                if (groups == null)
> +                    groups = new LinkedList<Map<String,?>>();
> +
> +                if (childrenLists == null)
> +                    childrenLists = new LinkedList<List<Map<String,?>>>();

This change is in the wrong place. If section is not null then groups and childrenLists should definitely not be null. What you have to do is move this very same code that is inside the loop outside the "section != itemSection" if so that groups and childrenLists will always be created if there is at least one item in history.

@@ +329,5 @@
>              // Close the query cursor as we won't use it anymore
>              cursor.close();
>  
> +            if (groups == null)
> +                return; // FIXME: display some sort of message when there's no history

Move this comment to the line before the 'if'. I tend to think that comments on same line are only good on variable/property declarations and should be very short.
Attachment #569957 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 569958 [details] [diff] [review]
Fix for exposed issues (v2)

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

Nice!
Attachment #569958 - Flags: review?(lucasr.at.mozilla) → review+
Pushed a fixed version of the first patch:
http://hg.mozilla.org/projects/birch/rev/e34d469c1991
http://hg.mozilla.org/projects/birch/rev/e87286aa52e0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 697270
20111027080410
http://hg.mozilla.org/projects/birch/rev/e87286aa52e0
--
Samsung Nexus S
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.