732901 - java.lang.IllegalStateException: at android.widget.ListView.layoutChildren(ListView.java) at android.widget.ListView.commonKey(ListView.java) with Adapter(class org.mozilla.gecko.TabsTray$TabsAdapter)

Status: VERIFIED FIXED

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

Description

[Scoobidiver (away)]

There are 3 crashes so far:
bp-03db8cfb-e0b9-4b1a-8fc2-cf8052120301 (20120225)
bp-42213852-66f0-44cb-b7be-02adf2120228 (20120228)
bp-79d9cab9-c29d-408b-a93f-185f02120303 (20120303)

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(2131492964, class android.widget.ListView) with Adapter(class org.mozilla.gecko.TabsTray$TabsAdapter)]
	at android.widget.ListView.layoutChildren(ListView.java:1541)
	at android.widget.ListView.commonKey(ListView.java:2088)
	at android.widget.ListView.onKeyDown(ListView.java:2069)
	at android.view.KeyEvent.dispatch(KeyEvent.java:1326)
	at android.view.View.dispatchKeyEvent(View.java:3985)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:787)
	at android.widget.ListView.dispatchKeyEvent(ListView.java:2054)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:789)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:789)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:789)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:789)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchKeyEvent(PhoneWindow.java:1873)
	at com.android.internal.policy.impl.PhoneWindow.superDispatchKeyEvent(PhoneWindow.java:1166)
	at android.app.Activity.dispatchKeyEvent(Activity.java:2095)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.dispatchKeyEvent(PhoneWindow.java:1845)
	at android.view.ViewRoot.deliverKeyEventToViewHierarchy(ViewRoot.java:2813)
	at android.view.ViewRoot.handleFinishedEvent(ViewRoot.java:2774)
	at android.view.ViewRoot.handleMessage(ViewRoot.java:1992)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:130)
	at org.mozilla.gecko.GeckoApp$35.run(GeckoApp.java:1819)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:130)
	at android.app.ActivityThread.main(ActivityThread.java:3691)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:507)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:912)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:670)
	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

[Scoobidiver (away)]

I see crashes in:
* 14.0a1/20120424: bp-fac2812c-8707-48ec-bcd8-93f7c2120425
* 14.0a2/20120425: bp-721ede61-16c3-4615-b213-407eb2120426.

Comment 3

[Scoobidiver (away)]

It's #3 unfixed top crasher in 14.0b1.

Comment 4

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

I would have assigned to Lucas, but he is still out.

Comment 5

[:Margaret Leibovic]

I'm looking into what might cause this, but could someone help me find some STR so that I can test a fix? Aaron, you've done work with the tabs tray, maybe you can help?

It sounds like the error was caused by modifying the adapter from the background thread, so maybe our onTabChanged method is getting called from the background thread, but we want to be sure to only modify mTabsAdapter on the main thread:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#142

I'm also suspicious of these empty notify methods, since the error also talked about the ListView not being notified properly:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#349

(Also, I'm trying to stay focused here, but I think TabsAdapter should be the one implementing Tabs.OnTabsChangedListener, not TabsTray. I want to fix that in a follow-up.)

Comment 6

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #5)

> It sounds like the error was caused by modifying the adapter from the
> background thread, so maybe our onTabChanged method is getting called from
> the background thread, but we want to be sure to only modify mTabsAdapter on
> the main thread:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.
> java#142

Actually, I audited all the places where we call Tabs.notifyListeners, and they all happen on the main thread except for one:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1299

If this is indeed the problem, we should hopefully be able to reproduce the issue by loading a page in the background while the tabs tray is open.

Also, if this is the issue, perhaps we should put an assertion in notifyListeners to make sure we're on the main thread.

Comment 7

[:Margaret Leibovic]

Attachment: Call Tabs.notifyListeners() on the main thread

I looked through crashstats, and the crashes on older builds aren't TabsTray crashes. The TabsTray crashes start May 4, which makes me blame:
http://hg.mozilla.org/mozilla-central/rev/c798e1ea15b4

This would make my thread theory correct. This patch fixes that.

We should really do something to make it harder to make mistakes like this :/

Comment 8

[:Margaret Leibovic]

Attachment: (Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter

Er, wait, some of those older crashes (those mentioned in comment 0) are older... I also had a theory that some of these were caused by this changeset, since that modifies the tabs adapter data set without actually notifying the list view:
http://hg.mozilla.org/mozilla-central/rev/1c5c5e78f370

I tested this patch and it works well, and it seems safer, so maybe we should take this as well to try to fix those older crashes? I can double check that none are older than this changeset.

Comment 9

[:Margaret Leibovic]

Attachment: (Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter (v2)

Actually, this is an even more elegant solution. We should just let the adapter handle calling assignValues, since that's what it already does in getView. I'm not sure why we weren't taking advantage of this built-in adapter functionality before.

Comment 10

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

Comment on attachment 624574 [details] [diff] [review]
Call Tabs.notifyListeners() on the main thread

Damn. I should have caught this in the review. I wasn't thinking that the listeners should only be called on the UI thread.

Comment 11

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

Comment on attachment 624586 [details] [diff] [review]
(Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter (v2)

This is elegant. If we did have a reason for not using the adapter mechanism, I don't remember it.

Pinging Sriram for a feedback? review in case he remembers.

Comment 12

[:Margaret Leibovic]

Comment on attachment 624586 [details] [diff] [review]
(Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter (v2)

(I think you forgot to flip the feedback flag)

Comment 13

[Scoobidiver (away)]

Do these patches fix also bug 732902 and other crashes with the same signature and different stacks?

Comment 14

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 624586 [details] [diff] [review]
(Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter (v2)

https://bugzilla.mozilla.org/attachment.cgi?id=624586&action=diff#a/mobile/android/base/TabsTray.java_sec1 
I am not sure on why this change. The idea here was to scroll to the selected tab when we open tabs tray. You are removing that functionality here.

-        @Override
-        public void notifyDataSetChanged() {
-        }

If you want to make assignValues to be private, this should be overriden and assignValues be handled here. If we leave to default implementation, then I believe, we are recreating views for every change (like thumbnail, title) in the view.

Comment 15

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> Comment on attachment 624586 [details] [diff] [review]
> (Part 2) Use notifyDatasetChanged() to update view after modifying
> mTabsAdapter (v2)
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=624586&action=diff#a/mobile/
> android/base/TabsTray.java_sec1 
> I am not sure on why this change. The idea here was to scroll to the
> selected tab when we open tabs tray. You are removing that functionality
> here.

This does not change the functionality. Calling notifyDataSetChanged on an adapter makes it update the list for us - ListView's setAdapter sets a DataSetObserver on the adapter. That calls getView, which we have overridden to call assignValues exactly like we're doing here.

> -        @Override
> -        public void notifyDataSetChanged() {
> -        }
> 
> If you want to make assignValues to be private, this should be overriden and
> assignValues be handled here. If we leave to default implementation, then I
> believe, we are recreating views for every change (like thumbnail, title) in
> the view.

I just made assignValues private since it didn't need to be public anymore. We shouldn't be recreating views for every change, since this will be using our custom getView() method.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Aaah.. I reviewed poorly. The code was to get the view that changed values -- not for scrolling.

Here is something to consider: Where a title changes for a single tab, we call notifyDataSetChanged(), which tries to create / assign values to all views that are visible. Do we need to call getView() and assignValues() multiple times for single value change in a single tab? That doesn't seem efficient to me.

Comment 17

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #16)
> Aaah.. I reviewed poorly. The code was to get the view that changed values
> -- not for scrolling.
> 
> Here is something to consider: Where a title changes for a single tab, we
> call notifyDataSetChanged(), which tries to create / assign values to all
> views that are visible. Do we need to call getView() and assignValues()
> multiple times for single value change in a single tab? That doesn't seem
> efficient to me.

Hm, good point. In that case, my previous patch would be better here.

Comment 18

[:Margaret Leibovic]

Comment on attachment 624579 [details] [diff] [review]
(Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter

This patch only calls notifyDataSetChanged if a tab has been closed.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 624579 [details] [diff] [review]
(Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter

This feels better to me. One more thing to consider, do we really need to call notifyDataSetChanged() ? The reason for overriding is not to allow Android to use its "usual default un-optimized" way of removing and adding views. That's the reason I suppressed it, and called invalidateViews() only when needed. Please make sure this doesn't crash anywhere. Last time I removed notifyDataSetChanged, it crashed few times.

Comment 20

[:Margaret Leibovic]

(In reply to Scoobidiver from comment #13)
> Do these patches fix also bug 732902 and other crashes with the same
> signature and different stacks?

Sorry this comment got lost in the mix. Unfortunately, these patches won't fix those crashes; it only fixes crashes in TabsTray. However, they're likely caused by a similar problem with those adapters.

Comment 21

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #19)
 
> This feels better to me. One more thing to consider, do we really need to
> call notifyDataSetChanged() ? The reason for overriding is not to allow
> Android to use its "usual default un-optimized" way of removing and adding
> views. That's the reason I suppressed it, and called invalidateViews() only
> when needed. Please make sure this doesn't crash anywhere. Last time I
> removed notifyDataSetChanged, it crashed few times.

I didn't experience any crashes. Perhaps our code has changed since this was first needed. I think we should try to avoid custom behavior unless there's a pressing need and we're really confident we know exactly what's going on, since that might be what causes tough to solve crashes like this one.

We should definitely keep an eye out for any new crashes in TabTray to make sure this didn't regress anything, and hopefully we'll stop seeing these crashes, which will mean these patches worked :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/331833b840e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70b34f9477d

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I found that I was able to get this crash via Samsung Galaxy S Captivate; installing 
https://addons.mozilla.org/en-US/mobile/addon/adblock-plus/?src=search

and then going to the chrome page: chrome://adblockplus/content/ui/firstRun.xhtml

https://crash-stats.mozilla.com/report/index/bp-ba3e7015-7b06-4646-a929-e8bb52120517

Comment 23

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I had spawned multiple of these windows, and then closed all but one.  The app became unresponsive.

Comment 24

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat

Margaret pointed me to the inbound build : http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/1337290575/

Initially I ran into some weirdness; it may have been remnants from updating to this build rather than starting fresh. ie I believe it's due to OOM from the crash.  After rebooting, things seem to operate fine and I did not seem to crash with the inbound build.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/331833b840e0
https://hg.mozilla.org/mozilla-central/rev/c70b34f9477d

Should this have a test?

Comment 26

[:Margaret Leibovic]

(In reply to Ryan VanderMeulen from comment #25)
> https://hg.mozilla.org/mozilla-central/rev/331833b840e0
> https://hg.mozilla.org/mozilla-central/rev/c70b34f9477d
> 
> Should this have a test?

It's hard to test for this actual issue, since we couldn't figure out its exact cause. However, it would be nice to write some Robocop tests for the tabs tray, since that shouldn't be too hard.

Comment 27

[:Margaret Leibovic]

Comment on attachment 624574 [details] [diff] [review]
Call Tabs.notifyListeners() on the main thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 749305
User impact if declined: potential crashes, tabs tray not updating properly after pages load
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): low risk, reverts accidental change from bug 749305
String or UUID changes made by this patch: n/a

I'm requesting approval only for this patch for right now, since it's lower risk. I think we should let the other patch hang out on mozilla-central for a few days to see if it helps fix this crash and make sure it doesn't cause any new crashes.

(Also, note that in bug 755786, I commented that I think this particular issue may not have been the one causing the crash, since the exception was caught in the log I posted in that bug, but this is definitely still a fix we want.)

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 624574 [details] [diff] [review]
Call Tabs.notifyListeners() on the main thread

approved, low risk regression fix

Comment 29

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c064693c724c

Comment 30

[:Margaret Leibovic]

Comment on attachment 624579 [details] [diff] [review]
(Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: possible crashes in the tabs tray activity
Testing completed (on m-c, etc.): baked on m-c for 4 days
Risk to taking this patch (and alternatives if risky): we were worried about this causing new crashes, but I don't see any new TabsTray crashes on Nightly in crash-stats, so we should be safe
String or UUID changes made by this patch: n/a

Comment 31

[Alex Keybl [:akeybl]]

Comment on attachment 624579 [details] [diff] [review]
(Part 2) Use notifyDatasetChanged() to update view after modifying mTabsAdapter

[Triage Comment]
Mobile only blocker, approved for Aurora 14.

Comment 32

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af9be77989b6

Comment 35

[Kevin Brosnan [Ex-Mozilla]]

Verified fixed in Firefox 14b3. Only one crash in the last 7 days versus over 300 on beta 2.  That single crash is https://crash-stats.mozilla.com/report/index/bd73216f-98a1-48f6-a94d-d35892120527

Comment 36

[:Margaret Leibovic]

(In reply to Kevin Brosnan [:kbrosnan] from comment #35)
> Verified fixed in Firefox 14b3. Only one crash in the last 7 days versus
> over 300 on beta 2.  That single crash is
> https://crash-stats.mozilla.com/report/index/bd73216f-98a1-48f6-a94d-
> d35892120527

That is a different crash. It's in AwesomeBarCursorAdapter. I thought there was a bug filed for that but I can't find it at the moment.