732902 - java.lang.IllegalStateException: at android.widget.ListView.layoutChildren(ListView.java) with Adapter(class org.mozilla.gecko.AllPagesTab$AwesomeBarCursorAdapter)

Status: RESOLVED FIXED

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

Description

[Scoobidiver (away)]

There is one crash so far in 13.0a1/20120304: bp-ade0e9d6-4a45-41fd-b88e-3f8fa2120304

java.lang.IllegalStateException: The content of the adapter has changed but ListView did not receive a notification. Make sure the content of your adapter is not modified from a background thread, but only from the UI thread. [in ListView(2131492896, class android.widget.ListView) with Adapter(class org.mozilla.gecko.AwesomeBarTabs$AwesomeBarCursorAdapter)]
	at android.widget.ListView.layoutChildren(ListView.java:1538)
	at android.widget.AbsListView$CheckForTap.run(AbsListView.java:2728)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at org.mozilla.gecko.GeckoApp$35.run(GeckoApp.java:1766)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4424)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:784)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.IllegalStateException%3A+at+android.widget.ListView.layoutChildren%28ListView.java%29

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Not blocking, still would be good to get a fix.

Comment 2

[Chris Peterson [:cpeterson]]

lucars, does this crash look related to bug 723499, another AwesomeBarTabs layoutChildren() exception you fixed a few months ago?

This is currently the #2 topcrash for 14.0b2.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Renom then.

Comment 4

[Scoobidiver (away)]

(In reply to Chris Peterson (:cpeterson) from comment #2)
> This is currently the #2 topcrash for 14.0b2.
Bug 732901 is #2 top crasher in 14.0b2, not this one. Stacks are different.

Comment 5

[:Margaret Leibovic]

I'm not seeing this crash at all in recent builds (I just looked at the most recent 20 on crash-stats, and they were all the TabsTray crash).

The crash in comment 0 was from 13.0a1. Are there any more of these crashes on more recent builds?

Comment 6

[Chris Peterson [:cpeterson]]

Thanks, Scoobidiver. I was confused because Socorro had aggregated these exceptions because they all originated from android.widget.ListView (but with different code paths).

Since we haven't seen this stack trace in recently, can we just close it WORKSFORME?

Comment 7

[Scoobidiver (away)]

Seen in 14.0b3: bp-bd73216f-98a1-48f6-a94d-d35892120527.

Comment 8

[:Margaret Leibovic]

The fix for this will likely be similar to the fix for bug 732901. We should look into AwesomeBarCursorAdapter and make sure we're calling notifyDataSetChanged if it changes.

Comment 9

[Scoobidiver (away)]

It's #115 top crasher in 14.0.1, #128 in 15.0b4, #30 in 16.0a2 and #19 in 17.0a1.

Comment 10

[Chris Peterson [:cpeterson]]

mfinkle, this bug looks like it might be related to bug 799877, which is assigned to you.

Comment 11

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

(In reply to Chris Peterson (:cpeterson) from comment #10)
> mfinkle, this bug looks like it might be related to bug 799877, which is
> assigned to you.

Assigning both to Lucas to see if he can find the cause.

Comment 12

[Scoobidiver (away)]

There are still crashes after the fix of bug 799877.

Comment 13

[Scoobidiver (away)]

It's #5 top crasher in 18.0.

Comment 14

[Alex Keybl [:akeybl]]

Lucas - what are the next steps in your investigation here?

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Set list of search engines in one step

This is unlikely the real cause of this bug but it's probably a good idea to set the list of search engines in one step instead of emptying it and adding items incrementally.

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Notify adapter updates when search term changes

And here's what I think is the real fix for this bug. This exception only happens when there's a mismatch between the number of items that the ListView think the adapter has and the actual number of items returned from getCount(). So, the problem here is definitely related to how we count items in AwesomeBarCursorAdapter.

The cursor is updated using the FilterQueryProvider, which can't really be the cause. As far as I could see, all updates to mSearchEngines are done in the UI thread so this is not the problem either.

The actual bug seems to be in how we update mSearchTerm. Although updates to mSearchTerm do affect what we return in getCount(), we were not calling notifyDataSetChanged() when we do so (opening a possible window of time where getCount() returns a new value that the ListView doesn't know about).

Comment 17

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9864e375b6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/27066b1bf86b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9864e375b6f
https://hg.mozilla.org/mozilla-central/rev/27066b1bf86b

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 704917 [details] [diff] [review]
Set list of search engines in one step

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: one of the top crashers we have right now.
Testing completed (on m-c, etc.): landed in m-c, no issues found so far.
Risk to taking this patch (and alternatives if risky): Fairly low, patch is very simple.
String or UUID changes made by this patch: none.

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 704921 [details] [diff] [review]
Notify adapter updates when search term changes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: one of the top crashers we have right now.
Testing completed (on m-c, etc.): landed in m-c, no issues found so far.
Risk to taking this patch (and alternatives if risky): Fairly low, patch is very simple.
String or UUID changes made by this patch: none.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 704917 [details] [diff] [review]
Set list of search engines in one step

Nice - hope this brings the crash volume down! If so, we'll take for beta 4 or beta 5.

Comment 22

[Lucas Rocha (:lucasr)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e460db9318c7
https://hg.mozilla.org/releases/mozilla-aurora/rev/caf86701e0fd

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 704917 [details] [diff] [review]
Set list of search engines in one step

Requesting approval for Beta to get wider testing (as discussed with akeybl)

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 704921 [details] [diff] [review]
Notify adapter updates when search term changes

Requesting approval for Beta to get wider testing (as discussed with akeybl)

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 704917 [details] [diff] [review]
Set list of search engines in one step

Approving on beta to get wider testing. Please land asap. Thanks !

Comment 26

[Lucas Rocha (:lucasr)]

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/c8738b336445
https://hg.mozilla.org/releases/mozilla-beta/rev/cb0efe495a4a