Closed Bug 911828 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox25 --- unaffected
firefox26 + verified
fennec 26+ ---

People

(Reporter: aaronmt, Assigned: Margaret)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [native-crash][startupcrash])

Crash Data

Attachments

(2 files)

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)
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)
Crash Signature: [@ java.lang.IllegalStateException: Couldn''t move cursor to position 0 at org.mozilla.gecko.home.MultiTypeCursorAdapter.getCursor(MultiTypeCursorAdapter.java)] → [@ java.lang.IllegalStateException: Couldn''t move cursor to position 0 at org.mozilla.gecko.home.MultiTypeCursorAdapter.getCursor(MultiTypeCursorAdapter.java)] [@ java.lang.IllegalStateException: Couldn''t move cursor to position -1 at org.mozilla.gecko…
Working video: http://www.youtube.com/watch?v=1j3yY0TR8PA

Who can take a look at this? Sriram?
This is the #3 topcrash on Nightly at this point, and is mostly within the first minute after launch.
Keywords: topcrash
Whiteboard: [native-crash] → [native-crash][startupcrash]
I can look at this, since it sounds similar to some other bugs I've worked on recently.
Assignee: nobody → 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.
I think this is a regression caused by bug 900744.
Blocks: 900744
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.
Blocks: 900148
Attached patch patchSplinter Review
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).
Attachment #799803 - Flags: review?(bnicholson)
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.
(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.
tracking-fennec: ? → 26+
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.
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?
Attachment #801781 - Flags: feedback?(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.
Attachment #801781 - Flags: feedback?(bnicholson) → feedback+
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.
Attachment #799803 - Flags: review?(bnicholson) → review+
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;
}
(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.
Blocks: 914449
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 :)
Blocks: 914452
https://hg.mozilla.org/mozilla-central/rev/dedfb60795aa
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.