948937 - crash in java.lang.ClassCastException: org.mozilla.gecko.home.TwoLinePageRow cannot be cast to org.mozilla.gecko.home.SearchEngineRow at org.mozilla.gecko.home.BrowserSearch$SearchAdapter.bindView(BrowserSearch.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Isac Lagerblad (:icaaq)]

This bug was filed from the Socorro interface and is 
report bp-42fc19a2-ea96-4fac-95da-137f12131211.
=============================================================

Comment 1

[Marco Zehe (:MarcoZ)]

icaaq: Would be good if you could write down the steps you took to get this bug.

Comment 2

[Isac Lagerblad (:icaaq)]

I think it only happens when I use firefox mobile 26 and talkback android version 4.4.2
 
1. Open Firefox
2. click on "Enter Search or Address"
3. Start typing a already visited URL
4. Click on some of the suggested sites in the list below the awesomebar
5. It crashes.

When I don't use talkback the screen looks all messed up when i click on some of the suggestion but it don't crash.

Comment 3

[Aaron Train [:aaronmt]]

Reports show three crashes on the Nexus 7 on the 26 branch.

Comment 4

[Robert Kaiser]

Looking at https://crash-stats.mozilla.com/report/list?product=FennecAndroid&signature=java.lang.ClassCastException%3A+org.mozilla.gecko.home.TwoLinePageRow+cannot+be+cast+to+org.mozilla.gecko.home.SearchEngineRow+at+org.mozilla.gecko.home.BrowserSearch%24SearchAdapter.bindView%28BrowserSearch.java%29 I see 26 final and 26 beta 10, and Nexus 4, 5, and 7 devices with KitKat (API level 19).

Comment 5

[Marco Zehe (:MarcoZ)]

I just got this crash, too, happened while I was either searching in my recent history, or typing in the awesome bar, and new stuff was being synchronized to the Nexus 7:
https://crash-stats.mozilla.com/report/index/d28cc717-c41d-48b8-bca0-e4a982140426

Comment 6

[Marco Zehe (:MarcoZ)]

I got another crash like this:
https://crash-stats.mozilla.com/report/index/7948f308-13ed-4f15-880c-0221e2140730

Here's what I did:
1. I had just installed Firefox beta.
2. Opened the awesome bar for the first time, and started to enter nightly.
3. Wanted to check if nightly.mozilla.org was offered to me through my sync'ed browser history. Found the prompt that said "Do you want to include Search results", and answered Yes.
4. Continued typing, typed the . behind nightly.

Result: Crash.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 8

[Marco Zehe (:MarcoZ)]

I have one user from the Eyes-Free mailing list, a blind TalkBack user, who is encountering this every single time he's using the address bar on his device. One sample report we could get from him is this one:
https://crash-stats.mozilla.com/report/index/3994545c-326f-4061-a34f-254052150915

Can we somehow get moving on this bug and finally finally fix it?

Comment 9

[Marco Zehe (:MarcoZ)]

Additional info: He's using a Samsung captivate glide.

Comment 10

[:Margaret Leibovic]

Ally, you've been working in this area of the code recently. Could you take a look at this?

Comment 11

[Allison Naaktgeboren :ally]

Sure. 

MarcoZ, thank you for the recent crash. Could you ask the reporter about what he is doing in the url bar? Does it crash when he taps into the urlbar? after he types the first character?

Other thoughts:
Although the original filing is relatively old, there does remain a direct cast in the SearchAdapter's bindView() [0][1] function that has been there the whole time. namely

final SearchEngineRow row = (SearchEngineRow) view;
and
final TwoLinePageRow row = (TwoLinePageRow) view;

[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/MultiTypeCursorAdapter.java
[1] MultiTypeCursorAdapter extends http://developer.android.com/reference/android/widget/CursorAdapter.html#bindView%28android.view.View,%20android.content.Context,%20android.database.Cursor%29

Just preventing a crash should be straightforward.What concerns me more is why a searchenginerow ended up in the wrong case. It suggests that there is state that is out of sync or corrupted in some manner and that the crash itself is hiding a larger issue. But I'll need to be able to re-pro it to figure that part out.

Comment 12

[Allison Naaktgeboren :ally]

While I have run into talkack causing the urlbar to ignore text entries or keyboard input on a samsung galaxy s, I have not been able to reproduce this crash.

The nexus family of devices seems unaffected.

I also noticed that samsung, in their infinite wisdom, has added/modified the voice software to go with talkback. 

I'll see if anyone on the team has one of these devices about.

Comment 13

[Allison Naaktgeboren :ally]

my best guess, and it is a guess until I can reproduce and verify, is that getItemViewType(), which looks at the position of the view to determine what type of object it is, is a position < getSearchEngineCount() when it shouldnt.

Comment 14

[Marco Zehe (:MarcoZ)]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #11)
> MarcoZ, thank you for the recent crash. Could you ask the reporter about
> what he is doing in the url bar? Does it crash when he taps into the urlbar?
> after he types the first character?

He just starts typing. He focuses the URL bar by touching or swiping to it, then double-tapping (the TalkBack gesture for actually activating something). Then, he uses a physical keyboard to enter an address or search term. In the middle of that process, Firefox crashes.

I remember that, when I encountered this type of crash a long while ago, it was roughly the same. I was typing away an address, but on the software keyboard, and suddenly Fennec was gone, and crash reporter came up. I remember that I didn't even interact with the search suggestions at the time.

Comment 15

[Allison Naaktgeboren :ally]

(In reply to Marco Zehe (:MarcoZ) from comment #14)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #11)
> > MarcoZ, thank you for the recent crash. Could you ask the reporter about
> > what he is doing in the url bar? Does it crash when he taps into the urlbar?
> > after he types the first character?
> 
> He just starts typing. He focuses the URL bar by touching or swiping to it,
> then double-tapping (the TalkBack gesture for actually activating
> something). Then, he uses a physical keyboard to enter an address or search
> term. In the middle of that process, Firefox crashes.
> 
> I remember that, when I encountered this type of crash a long while ago, it
> was roughly the same. I was typing away an address, but on the software
> keyboard, and suddenly Fennec was gone, and crash reporter came up. I
> remember that I didn't even interact with the search suggestions at the time.

Ok, thank you. 

:liuche in sf is going to see if she can repro. Theres a couple of old devices running 2.3ish, (talkback makes running them remotely not such a good idea).

If it doesn't, I'll have to table this until we can get a glide off ebay.

Comment 16

[Allison Naaktgeboren :ally]

Attachment: AccessibilityCrash

This patch will prevent this crash, its cousin (where a SearchSuggestionRow is incorrectly cast to a TwoLinePageRow), and improve the hygiene as unchecked casts are bad.

If getPosition returns a mismatch (the case of this specific crash), the search suggestions will not be drawn on screen. until the next bindview().

This patch does not answer the deeper questions around why the getTypeFromPosition(), controlled by getSearchEngineCount, which is based on the search engine list arriving from gecko first, is not behaving correctly. 

However, it will stop this crash from hurting users.

I will open a new bug for the getTypeFromPosition investigation.

Comment 17

[Allison Naaktgeboren :ally]

https://support.mozilla.org/en-US/kb/how-use-talkback-support-firefox#w_awesome-bar 

<-- handy for figuring out what the expected behavior between the awesomebar and talkback should be

Comment 18

[Chenxia Liu [:liuche]]

A few more devices that I want to try, but I haven't been able to repro any crashes.

Work fine with Talkback: N9, N7, S4, S2, Nexus S, some Moto Droid device
Can't type into urlbar (but doesn't crash): N4 running 4.4

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

I've tried with a Galaxy SII from ATT SGH-I727 which is a similar generation device and a Motorola Atrix which uses the Tegra 2 chipset. The device in question has a hardware keyboard, I wonder if that is needed. Might be best to just ship a try build to the affected user since we have a patch.

Comment 20

[Chenxia Liu [:liuche]]

Also couldn't repro with an Asus Transformer TF300T, or an Asus Transformer 201 (with hardware keyboard). Talkback enabled worked just fine.

Comment 21

[:Margaret Leibovic]

Comment on attachment 8663163 [details] [diff] [review]
AccessibilityCrash

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

::: mobile/android/base/home/BrowserSearch.java
@@ +902,5 @@
>          public void bindView(View view, Context context, int position) {
>              final int type = getItemViewType(position);
>  
>              if (type == ROW_SEARCH || type == ROW_SUGGEST) {
> +                if (!(view instanceof SearchEngineRow)) {

This will prevent the crash, but won't it result in the search suggestions looking wrong? If we just return here, we won't update the view, so it will be showing old data. Maybe the if/else should actually just check for the type of the view, rather than calling this `getItemViewType` method. 

Ideally, it would be nice to figure out how `getItemViewType` is returning ROW_SEARCH or ROW_SUGGEST for a view that isn't a SearchEngineRow? Could something be wrong with this position logic?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#856

I understand this is hard to debug without being able to reproduce. Perhaps we could make a build with some debug logging and give it to this user to test.

Comment 22

[Allison Naaktgeboren :ally]

(In reply to :Margaret Leibovic from comment #21)
> Comment on attachment 8663163 [details] [diff] [review]
> AccessibilityCrash
> 
> Review of attachment 8663163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BrowserSearch.java
> @@ +902,5 @@
> >          public void bindView(View view, Context context, int position) {
> >              final int type = getItemViewType(position);
> >  
> >              if (type == ROW_SEARCH || type == ROW_SUGGEST) {
> > +                if (!(view instanceof SearchEngineRow)) {
> 
> This will prevent the crash, but won't it result in the search suggestions
> looking wrong? If we just return here, we won't update the view, so it will
> be showing old data. Maybe the if/else should actually just check for the
> type of the view, rather than calling this `getItemViewType` method. 

As noted in comment 16, no pills are drawn as we exit early every time until we get a valid object/type match.  No matter what, we'll look wrong until those match up again.

If the problem is, as currently suspected, that getEngineCount() is 0 because the engines have not arrived from Gecko, http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#581 to be put into mEngines, then we'll probably still have incorrect/missing UI even if the type is correct.

I have no problem switching the if checks for instanceof, but it's unlikely to be magic.  

> 
> Ideally, it would be nice to figure out how `getItemViewType` is returning
> ROW_SEARCH or ROW_SUGGEST for a view that isn't a SearchEngineRow? Could
> something be wrong with this position logic?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> BrowserSearch.java#856
> 
> I understand this is hard to debug without being able to reproduce. Perhaps
> we could make a build with some debug logging and give it to this user to
> test.

We can certainly offer the user the option.
It's not clear to me that the affected sightless user _can_ extract the logs from his/her phone using talkback alone.

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

An Android 2.3 through 4.0 user can still get logs reasonably simply by using the aLogcat app. https://play.google.com/store/apps/details?id=org.jtb.alogcat

Comment 24

[Marco Zehe (:MarcoZ)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #23)
> An Android 2.3 through 4.0 user can still get logs reasonably simply by
> using the aLogcat app.
> https://play.google.com/store/apps/details?id=org.jtb.alogcat

This user is, however, using KitKat on that Samsung thing. So the process is quite different.

Comment 25

[:Margaret Leibovic]

Can they use jchen's add-on?
https://addons.mozilla.org/en-US/android/addon/logview/

Comment 26

[Kevin Brosnan [Ex-Mozilla]]

That device never had an OTA update to Android 4.4 (kitkat) http://www.gsmarena.com/samsung_i927_captivate_glide-4071.php . What ROM are they using? cyanogenmod?

Comment 27

[Marco Zehe (:MarcoZ)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #26)
> That device never had an OTA update to Android 4.4 (kitkat)
> http://www.gsmarena.com/samsung_i927_captivate_glide-4071.php . What ROM are
> they using? cyanogenmod?

Possibly.

Comment 28

[Allison Naaktgeboren :ally]

Could we get the user on this bug directly? That would help immensely.

Comment 29

[Allison Naaktgeboren :ally]

Attachment: talkbackCrasherBandAidAndLogging

bandaid with logging as discussed offline.

There is some duplication of logging in getItemType() and the error case in bindView(), as there is a hypothesis floating around that these values might change underfoot.

Comment 30

[:Margaret Leibovic]

Comment on attachment 8670065 [details] [diff] [review]
talkbackCrasherBandAidAndLogging

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

::: mobile/android/base/home/BrowserSearch.java
@@ +928,2 @@
>              if (position < getPrimaryEngineCount()) {
> +                Log.w(LOGTAG, "getItemType() mSuggestionsEnabled = " + mSuggestionsEnabled + "hasSuggestions is " + mSearchEngines.get(position).hasSuggestions());

Remove this logging before landing.

@@ +976,5 @@
>          }
>  
>          @Override
>          public void bindView(View view, Context context, int position) {
> +            if (view instanceof SearchEngineRow && view !=null) {

Why did you add this null check? We weren't seeing any problems with a null view, so I don't think you need this.

@@ +1001,5 @@
>                  row.updateFromCursor(c);
> +            } else {
> +                Log.w(LOGTAG, "bindview() VIEW in bad state. Should never happen");
> +                final int type = getItemViewType(position);
> +                Log.w(LOGTAG, "bindview() thinks row should be: "+ type);

Let's just use one log statement, and use Log.e.

But thinking about this more, would we ever actually have a view that's not SearchEngineRow or a TwoLinePageRow? I don't think that we would ever hit this else statement...

From the crash report, it seems like the problem is that getItemViewType isn't returning the right type for a given position, so that's probably where we want to be adding error logging for unexpected cases.

@@ +1004,5 @@
> +                final int type = getItemViewType(position);
> +                Log.w(LOGTAG, "bindview() thinks row should be: "+ type);
> +
> +                if (view == null) {
> +                    Log.w(LOGTAG, "bindview() view is null. Should never happen");

I don't think we should add logging for this case, if this is something we haven't seen before.

Comment 31

[Allison Naaktgeboren :ally]

(In reply to :Margaret Leibovic from comment #30)
> Comment on attachment 8670065 [details] [diff] [review]
> talkbackCrasherBandAidAndLogging
> 
> Review of attachment 8670065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BrowserSearch.java
> @@ +928,2 @@
> >              if (position < getPrimaryEngineCount()) {
> > +                Log.w(LOGTAG, "getItemType() mSuggestionsEnabled = " + mSuggestionsEnabled + "hasSuggestions is " + mSearchEngines.get(position).hasSuggestions());
> 
> Remove this logging before landing.

Even though there's a possibility that these values might change?
> 
> @@ +976,5 @@
> >          }
> >  
> >          @Override
> >          public void bindView(View view, Context context, int position) {
> > +            if (view instanceof SearchEngineRow && view !=null) {
> 
> Why did you add this null check? We weren't seeing any problems with a null
> view, so I don't think you need this.

Because that is the other implicit assumption that this code makes, and while I'm here preventing a crash from an implicit assumption about type, cover the other implicit assumption. If you would still like me to remove it, I can. 

> But thinking about this more, would we ever actually have a view that's not
> SearchEngineRow or a TwoLinePageRow? I don't think that we would ever hit
> this else statement...

As the logging message states, it shouldn't happen. It could happen, and then I _really_ want to know about it.  

> 
> From the crash report, it seems like the problem is that getItemViewType
> isn't returning the right type for a given position, so that's probably
> where we want to be adding error logging for unexpected cases.
> 

getItemViewType() has no context that anything "unexpected" has gone on. We only know that something is not right by matching against the other objects in bindview(), so for debugging I'll still need logging in bindview() even if I move (more) logging into getItemViewType(), which you asked me to remove in your first comment...
  
As you requested, the if() blocks no longer use the value from getItemViewType(), so it no longer needs to be called at all functionally by bindview(). If bindview() gets into a bad state, I still want to know because bindview() is pivotal to the UI updating properly. 

Please let me know if you still want all these changes.

Comment 32

[:Margaret Leibovic]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #31)
> (In reply to :Margaret Leibovic from comment #30)
> > Comment on attachment 8670065 [details] [diff] [review]
> > talkbackCrasherBandAidAndLogging
> > 
> > Review of attachment 8670065 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/home/BrowserSearch.java
> > @@ +928,2 @@
> > >              if (position < getPrimaryEngineCount()) {
> > > +                Log.w(LOGTAG, "getItemType() mSuggestionsEnabled = " + mSuggestionsEnabled + "hasSuggestions is " + mSearchEngines.get(position).hasSuggestions());
> > 
> > Remove this logging before landing.
> 
> Even though there's a possibility that these values might change?

Will this help with debugging the crash? I'm worried this will just be extra noise for the majority of the time.

> > 
> > @@ +976,5 @@
> > >          }
> > >  
> > >          @Override
> > >          public void bindView(View view, Context context, int position) {
> > > +            if (view instanceof SearchEngineRow && view !=null) {
> > 
> > Why did you add this null check? We weren't seeing any problems with a null
> > view, so I don't think you need this.
> 
> Because that is the other implicit assumption that this code makes, and
> while I'm here preventing a crash from an implicit assumption about type,
> cover the other implicit assumption. If you would still like me to remove
> it, I can. 

I think we should remove this. We should only add null checks for cases where we expect something to be null, otherwise they will just conceal problems in the future.

> > But thinking about this more, would we ever actually have a view that's not
> > SearchEngineRow or a TwoLinePageRow? I don't think that we would ever hit
> > this else statement...
> 
> As the logging message states, it shouldn't happen. It could happen, and
> then I _really_ want to know about it.  

But in this case, the "shouldn't happen" really is quite impossible. What would the view type be in this case? I feel like there's a difference between something that "shouldn't happen" but is happening due to some bug in our logic (i.e. the crash we're seeing here due to a mismatch of view type and the constant we decided to return from getItemViewType), vs. something that would have to be a more fundamental Android bug (given the logic in our constructor, we will only ever create either SearchEngineRow or TwoLinePageRow [1]).

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#921

> > 
> > From the crash report, it seems like the problem is that getItemViewType
> > isn't returning the right type for a given position, so that's probably
> > where we want to be adding error logging for unexpected cases.
> > 
> 
> getItemViewType() has no context that anything "unexpected" has gone on. We
> only know that something is not right by matching against the other objects
> in bindview(), so for debugging I'll still need logging in bindview() even
> if I move (more) logging into getItemViewType(), which you asked me to
> remove in your first comment...
>   
> As you requested, the if() blocks no longer use the value from
> getItemViewType(), so it no longer needs to be called at all functionally by
> bindview(). If bindview() gets into a bad state, I still want to know
> because bindview() is pivotal to the UI updating properly. 

Looking into the history of this code a bit more to try to understand what's going on, I wonder why we even need ROW_SEARCH anymore at all now that we have a quick search bar instead of listing additional search engines in rows below... That change happened in this changeset:
http://hg.mozilla.org/mozilla-central/rev/8e5abcb55c35

Sebastian, do you have any context about this? Why not just have a single search row type that we always return for the first item in the list?

> Please let me know if you still want all these changes.

I'm fine to leave in the logging if you think it will be useful to help you figure out what problems are going on, without creating noise for people who aren't experiencing any problems.

I would prefer for you to remove the null checks and the unexpected else case before landing this. You could even remove the instanceof TwoLinePageRow check, and just keep an else statement there.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

I dug into our code and the one of AbsListView and it almost looks like an Android bug. Especially those two changes in Android 5.0.0 look interesting:

https://github.com/android/platform_frameworks_base/commit/e6be9c78b55c46e221ebc3cd6b08f1be3588b1d7
https://github.com/android/platform_frameworks_base/commit/ff699570f62113b4df5b0efd74b9df8b9dbcd1a9

This here looks like the first bug: The view was incorrectly recycled as view type 0 (ROW_SEARCH). All similar crashes I looked at seem to be with Android 4.4 which supports this theory too.

We could handle this in our MultiTypeCursorAdapter class, where we only create a new view if the convertView is null. Additionally we may want to check the type and create a new view if it is wrong:

>    public final View getView(int position, View convertView, ViewGroup parent) {
>        final Context context = parent.getContext();
>        if (convertView == null) {
>            convertView = newView(context, position, parent);
>        }
> 
>        bindView(convertView, context, position);
>        return convertView;
>    }

Other than that I could not find any way a non-UI thread may change the search engines or the cursor causing race conditions.

(In reply to :Margaret Leibovic from comment #32)
> Sebastian, do you have any context about this? Why not just have a single
> search row type that we always return for the first item in the list?

The code points to bug 815937. Back then it seemed like we had problems when the same view was used for suggestions and just search. Now with this just being one view only and we are only switching types if the user opts into having suggestions we may be able to get rid of it and just use ROW_STANDARD and ROW_SEARCH. This will simplify the code but is not necessarily related to this issue (because we will still have those two view types).

However regarding "always return for the first item in the list": That's what we are doing now as soon as we have search engines. There's a short period, before we load the search engines for the first time, where we do not have any and therefore show something from history/bookmarks as first item.

Comment 34

[Sergej Kravcenko]

Keep in mind that CursorAdapter has stable ids. When you add additional rows, this could lead to bugs when list view is in transient state. AbsListView uses mTransientStateViewsById that is based on ids, not on type.

I'm not sure that this is the case, but imagine following situation: cursor row 0 has rowid 0. All extra rows in the back of the list view also get rowid 0, because cursor position is out of range. Here we have overlapping ids, and transient scrap cache mixes row 0, which is ROW_SUGGEST with last row, which is ROW_STANDARD.

Anyway. I don't think that CursorAdapter can be used in a such hacky way, when you override getCount and return  count more than cursor.getCount(). Correct me if I'm wrong.

Comment 35

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sergej Kravcenko from comment #34)
> Keep in mind that CursorAdapter has stable ids. When you add additional
> rows, this could lead to bugs when list view is in transient state.
> AbsListView uses mTransientStateViewsById that is based on ids, not on type.
> 
> I'm not sure that this is the case, but imagine following situation: cursor
> row 0 has rowid 0. All extra rows in the back of the list view also get
> rowid 0, because cursor position is out of range. Here we have overlapping
> ids, and transient scrap cache mixes row 0, which is ROW_SUGGEST with last
> row, which is ROW_STANDARD.

Good find! But I don't see how we can run into this here. The id of the first row in the database should be 1 and not 0. However I agree that we do not have stable ids here (0 is used more than once). Shouldn't we be able to work around this by returning false from hasStableIds()?

> Anyway. I don't think that CursorAdapter can be used in a such hacky way,
> when you override getCount and return  count more than cursor.getCount().
> Correct me if I'm wrong.

For the underlying CursorAdapter everything should be normal. All public methods should adjust the position/count before calling the super methods. Do you see other problems apart from hasStableIds()?

Comment 36

[Sergej Kravcenko]

(In reply to Sebastian Kaspari (:sebastian) from comment #35)
> Good find! But I don't see how we can run into this here. The id of the
> first row in the database should be 1 and not 0. 

I just did some tests. I don't know why, but all bookmarks ids (_id) are 0. Table create sql looks correct, but cursor has 0 _id for all rows.

10-09 14:21:10.740 28198-28198/org.mozilla.fennec_Pumba I/ss: 0 {
10-09 14:21:10.740 28198-28198/org.mozilla.fennec_Pumba I/ss:    _id=0
10-09 14:21:10.740 28198-28198/org.mozilla.fennec_Pumba I/ss:    url=https://www.google.com/
10-09 14:21:10.740 28198-28198/org.mozilla.fennec_Pumba I/ss:    title=Google
10-09 14:21:10.740 28198-28198/org.mozilla.fennec_Pumba I/ss:    bookmark_id=null
10-09 14:21:10.740 28198-28198/org.mozilla.fennec_Pumba I/ss:    history_id=12
10-09 14:21:10.740 28198-28198/org.mozilla.fennec_Pumba I/ss: }
10-09 14:21:10.750 28198-28198/org.mozilla.fennec_Pumba I/ss: 1 {
10-09 14:21:10.750 28198-28198/org.mozilla.fennec_Pumba I/ss:    _id=0
10-09 14:21:10.750 28198-28198/org.mozilla.fennec_Pumba I/ss:    url=https://www.dropbox.com/
10-09 14:21:10.750 28198-28198/org.mozilla.fennec_Pumba I/ss:    title=Dropbox
10-09 14:21:10.750 28198-28198/org.mozilla.fennec_Pumba I/ss:    bookmark_id=null
10-09 14:21:10.750 28198-28198/org.mozilla.fennec_Pumba I/ss:    history_id=263
10-09 14:21:10.750 28198-28198/org.mozilla.fennec_Pumba I/ss: }
......

Comment 37

[Sergej Kravcenko]

Search history is selected from Combined.VIEW_NAME, in this view _id is set to 0.

Comment 38

[:Margaret Leibovic]

We query the "combined" view for the BrowserSearch bookmark/history results. It's a bit gnarly:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#232

But it looks like we did think about this issue:

250         We need to return an _id column because CursorAdapter requires it for its
251         default implementation for the getItemId() method. However, since
252         we're not using this feature in the parts of the UI using this view,
253         we can just use 0 for all rows.

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #38)
> But it looks like we did think about this issue:
> 
> 250         We need to return an _id column because CursorAdapter requires
> it for its
> 251         default implementation for the getItemId() method. However, since
> 252         we're not using this feature in the parts of the UI using this
> view,
> 253         we can just use 0 for all rows.

Oh! Then we should definitely return false from hasStableIds(). As Sergej points out in comment #34 this could lead to the crash mentioned here.

Comment 40

[Allison Naaktgeboren :ally]

Attachment: talkbackcrashHasStableIds

Sebastian, is this what you had in mind? I don't know enough about android internals to fully follow. It sounds like you are reasonably confident this will fix the crash? Since I have never been able to reproduce this crash, I can't prove this fix works.

try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=23b3af9c56fb

Comment 41

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8673197 [details] [diff] [review]
talkbackcrashHasStableIds

Yes, that's what I had in mind. With the debugging work from Sergej I'm confident that this fixes at least one potentially source of error. But I can't guarantee that there aren't more... :)
At the very least this will stop our adapter from lying to the framework about having stable IDs.

Comment 42

[Allison Naaktgeboren :ally]

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

Comment 43

[Daniel Holbert [:dholbert]]

Commit message was:
>  Bug 948937 - crash in java.lang.ClassCastException: org.mozilla.gecko.home.TwoLinePageRow...r=sebastian

Nit: This ^^ is a bit of an arcane commit message.  Commit messages should describe the change, not just quote the bug title, per MDN docs here: https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a77d988aa36