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

VERIFIED FIXED in Firefox 26

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: Margaret)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
Firefox 26
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25 unaffected, firefox26+ verified, fennec26+)

Details

(Whiteboard: [native-crash][startupcrash], crash signature)

Attachments

(2 attachments)

Reporter

Description

6 years ago
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)
Reporter

Comment 1

6 years ago
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)

Updated

6 years ago
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…
Reporter

Comment 3

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

Who can take a look at this? Sriram?

Comment 4

6 years ago
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]
Assignee

Comment 5

6 years ago
I can look at this, since it sounds similar to some other bugs I've worked on recently.
Assignee: nobody → margaret.leibovic
Assignee

Comment 6

6 years ago
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.
Assignee

Comment 7

6 years ago
I think this is a regression caused by bug 900744.
Blocks: 900744
Assignee

Comment 8

6 years ago
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
Assignee

Comment 9

6 years ago
Posted 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.
Assignee

Comment 11

6 years ago
(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.
Assignee

Comment 13

6 years ago
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;
}
Assignee

Comment 17

6 years ago
(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.
Assignee

Updated

6 years ago
Blocks: 914449
Assignee

Comment 18

6 years ago
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 :)
Assignee

Updated

6 years ago
Blocks: 914452
https://hg.mozilla.org/mozilla-central/rev/dedfb60795aa
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Reporter

Updated

6 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.