911828 - Crash in java.lang.IllegalStateException: Couldn''t move cursor to position 0 at org.mozilla.gecko.home.MultiTypeCursorAdapter.getCursor(MultiTypeCursorAdapter.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-effcc6c4-f2b2-4cca-a6c8-725932130902.
=============================================================

Steps

In a new profile

i) Access the address-bar, and accept Google Search Suggestions
ii) Type 'cat', and long tap on the first result to highlight it orange
iii) Hit back and then access the address-bar again and long-tap on the row


This is 100% reproducible for me on a variety of devices/

--
LG Nexus 4 (Android 4.3), Samsung Galaxy SIV (Android 4.3), Galaxy Note II (Android 4.2)
Nightly (09/02)

Comment 1

[Aaron Train [:aaronmt]]

E/GeckoAppShell(15745): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoAppShell(15745): java.lang.IllegalStateException: Couldn't move cursor to position 2
E/GeckoAppShell(15745): 	at org.mozilla.gecko.home.MultiTypeCursorAdapter.getCursor(MultiTypeCursorAdapter.java:49)
E/GeckoAppShell(15745): 	at org.mozilla.gecko.home.BrowserSearch$1.onItemClick(BrowserSearch.java:234)
E/GeckoAppShell(15745): 	at org.mozilla.gecko.home.HomeListView$1.onItemClick(HomeListView.java:111)
E/GeckoAppShell(15745): 	at android.widget.AdapterView.performItemClick(AdapterView.java:301)
E/GeckoAppShell(15745): 	at android.widget.AbsListView.performItemClick(AbsListView.java:1376)
E/GeckoAppShell(15745): 	at android.widget.AbsListView$PerformClick.run(AbsListView.java:3162)
E/GeckoAppShell(15745): 	at android.widget.AbsListView$1.run(AbsListView.java:4115)
E/GeckoAppShell(15745): 	at android.os.Handler.handleCallback(Handler.java:730)
E/GeckoAppShell(15745): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoAppShell(15745): 	at android.os.Looper.loop(Looper.java:137)
E/GeckoAppShell(15745): 	at android.app.ActivityThread.main(ActivityThread.java:5368)
E/GeckoAppShell(15745): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoAppShell(15745): 	at java.lang.reflect.Method.invoke(Method.java:525)
E/GeckoAppShell(15745): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1046)
E/GeckoAppShell(15745): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:862)
E/GeckoAppShell(15745): 	at dalvik.system.NativeStart.main(Native Method)

Comment 2

[Aaron Train [:aaronmt]]

Video: http://www.youtube.com/watch?v=v2Fr45I6Su4

Comment 3

[Aaron Train [:aaronmt]]

Working video: http://www.youtube.com/watch?v=1j3yY0TR8PA

Who can take a look at this? Sriram?

Comment 4

[Robert Kaiser]

This is the #3 topcrash on Nightly at this point, and is mostly within the first minute after launch.

Comment 5

[:Margaret Leibovic]

I can look at this, since it sounds similar to some other bugs I've worked on recently.

Comment 6

[:Margaret Leibovic]

Somehow the OnItemClickListener is firing on the long tap, but the OnItemLongClickListener isn't.

I can reproduce this easily if I try long tapping on any search row, regardless of whether or not suggestions are enabled. However, if I first open a valid context menu (on a history item), I can't reproduce it anymore until I restart the browser.

In the cases where we don't crash, the OnItemClickListener is never called, so it seems like there's some problem with the click/long click listeners here.

Comment 7

[:Margaret Leibovic]

I think this is a regression caused by bug 900744.

Comment 8

[:Margaret Leibovic]

I think it's also partially caused by bug 900148. Basically, it seems that having click listeners on SearchEngineRow itself sometimes causes problems with the item click listeners we're setting in BrowserSearch.

Comment 9

[:Margaret Leibovic]

Attachment: patch

This patch is similar to my original patch for bug 900148.

I'm a bit concerned that I don't actually know the root cause of this bad listener interaction, but this patch definitely fixes the crash. The only problem I see with it is the instanceof check (which lucasr dinged me for in bug 900148 comment 9), but I think that's more robust than relying on a getItemViewType check, since we want to actually call a method of SearchEngineRow (at least in one of the cases).

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

One way to defeat the "instanceof ConcreteClass" is to use an interface. Then you "instanceof SomeRowInterface" and call a method that is implemented by the SearchRow in a way that is different than other rows. Some rows might have empty impls.

Comment 11

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> One way to defeat the "instanceof ConcreteClass" is to use an interface.
> Then you "instanceof SomeRowInterface" and call a method that is implemented
> by the SearchRow in a way that is different than other rows. Some rows might
> have empty impls.

Yes, that's an appropriate Java way to do this, but I feel that might be overkill in this case, since the action we're doing here (performing a search) is actually really tightly coupled to the idea of a SearchEngineRow. It's also not just about that method, but about the fact that that row doesn't have the same cursor associated with it either.

Comment 12

[Brian Nicholson (:bnicholson)]

Using "instanceof" is usually code smell, and I like the more OO approach of an interface for each type. In fact, that's exactly what we had before fig: http://hg.mozilla.org/releases/mozilla-aurora/file/33d447b77903/mobile/android/base/awesomebar/AllPagesTab.java#l321. Then we can just do item.onClick(), with no type check at all: http://hg.mozilla.org/releases/mozilla-aurora/file/33d447b77903/mobile/android/base/awesomebar/AllPagesTab.java#l736. The same could be done for the long press click.

So can we just override getItem() like we did before? I can't tell if that's the same thing Mark was suggesting, but it seems cleaner than "instanceof" to me.

Comment 13

[:Margaret Leibovic]

Attachment: alternate patch WIP

Here's your damn interfaces!

I hurried through this because I need to stop working soon, but it seems to work. What do you think?

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 801781 [details] [diff] [review]
alternate patch WIP

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

Funny quote: "The great thing about Object Oriented code is that it can make small, simple problems look like large, complex ones."

There's more code, but each method is more concise, so that's a win. I agree that this seems unnecessarily complicated at certain parts, like having to add a setOnUrlOpenListener() and the akward way that onLongClick() works. To simplify this a bit, SearchPageRow could be an inner class of BrowserSearch so its onClick/onItemLongClick methods could be implemented directly in BrowserSearch, using its private methods. That should allow you to drop the listener and move the mList.onItemLongClick() directly into SearchPageRow's implementation.

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 799803 [details] [diff] [review]
patch

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

I've probably stretched this out too long -- a good exercise, but using instanceof here wouldn't be the end of the world. I suppose I'm OK with this approach if you like it better.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 801781 [details] [diff] [review]
alternate patch WIP

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

Drive-by:

::: mobile/android/base/home/BrowserSearch.java
@@ +227,5 @@
>  
>          mList.setOnItemClickListener(new AdapterView.OnItemClickListener() {
>              @Override
>              public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                ((HomeListItem) view).onClick();

I guess this would be done by default, if there's a click listener on the row. Or by calling "performClick()".

@@ +235,5 @@
>          mList.setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {
>              @Override
>              public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> +                // First let the item handle the long click.
> +                if (((HomeListItem) view).onLongClick()) {

I am not sure if this is a good approach. We should probably be using the long click listener.

if (view.performLongClick()) {
    return true;
}

Comment 17

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #16)
> Comment on attachment 801781 [details] [diff] [review]
> alternate patch WIP
> 
> Review of attachment 801781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by:
> 
> ::: mobile/android/base/home/BrowserSearch.java
> @@ +227,5 @@
> >  
> >          mList.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> >              @Override
> >              public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> > +                ((HomeListItem) view).onClick();
> 
> I guess this would be done by default, if there's a click listener on the
> row. Or by calling "performClick()".
> 
> @@ +235,5 @@
> >          mList.setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {
> >              @Override
> >              public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> > +                // First let the item handle the long click.
> > +                if (((HomeListItem) view).onLongClick()) {
> 
> I am not sure if this is a good approach. We should probably be using the
> long click listener.
> 
> if (view.performLongClick()) {
>     return true;
> }

We currently have those click listeners on the items themselves (see the parts of the code that the patch is removing), and that's what appears to be causing the crash here, so I think we want to avoid that approach.

Comment 18

[:Margaret Leibovic]

I just landed the first patch:
https://hg.mozilla.org/integration/fx-team/rev/dedfb60795aa

I filed bug 914449 as a follow-up to clean up the logic to use interfaces. I agree that it would be nice to improve this logic, but that's definitely less pertinent than fixing the topcrash here :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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