Closed Bug 732901 Opened 8 years ago Closed 7 years ago

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)

Categories

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

13 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +

People

(Reporter: scoobidiver, Assigned: Margaret)

References

Details

(Keywords: crash, reproducible, topcrash, Whiteboard: [native-crash])

Crash Data

Attachments

(3 files, 1 obsolete file)

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
Not blocking, still would be good to get a fix.
blocking-fennec1.0: --- → -
Priority: -- → P1
I see crashes in:
* 14.0a1/20120424: bp-fac2812c-8707-48ec-bcd8-93f7c2120425
* 14.0a2/20120425: bp-721ede61-16c3-4615-b213-407eb2120426.
It's #3 unfixed top crasher in 14.0b1.
blocking-fennec1.0: - → ?
Keywords: topcrash
I would have assigned to Lucas, but he is still out.
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: ? → +
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.)
Keywords: qawanted
(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.
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 :/
Attachment #624574 - Flags: review?(mark.finkle)
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.
Attachment #624579 - Flags: review?(mark.finkle)
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.
Attachment #624579 - Attachment is obsolete: true
Attachment #624586 - Flags: review?(mark.finkle)
Attachment #624579 - Flags: review?(mark.finkle)
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.
Attachment #624574 - Flags: review?(mark.finkle) → review+
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.
Attachment #624586 - Flags: review?(mark.finkle) → review+
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)
Attachment #624586 - Flags: feedback?(sriram)
Do these patches fix also bug 732902 and other crashes with the same signature and different stacks?
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.
Attachment #624586 - Flags: feedback?(sriram) → feedback-
(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.
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.
(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 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.
Attachment #624579 - Attachment is obsolete: false
Attachment #624579 - Flags: review?(sriram)
Attachment #624579 - Flags: review?(mark.finkle)
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.
Attachment #624579 - Flags: review?(sriram) → review+
Attachment #624586 - Attachment is obsolete: true
Attachment #624579 - Flags: review?(mark.finkle)
(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.
(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
Target Milestone: --- → Firefox 15
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
I had spawned multiple of these windows, and then closed all but one.  The app became unresponsive.
Keywords: qawantedreproducible
Attached file 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.
https://hg.mozilla.org/mozilla-central/rev/331833b840e0
https://hg.mozilla.org/mozilla-central/rev/c70b34f9477d

Should this have a test?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Summary: java.lang.IllegalStateException: at android.widget.ListView.layoutChildren(ListView.java) at android.widget.ListView.commonKey(ListView.java) → 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)
(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 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.)
Attachment #624574 - Flags: approval-mozilla-aurora?
Comment on attachment 624574 [details] [diff] [review]
Call Tabs.notifyListeners() on the main thread

approved, low risk regression fix
Attachment #624574 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #624579 - Flags: approval-mozilla-aurora?
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.
Attachment #624579 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 750200
Duplicate of this bug: 757800
No longer blocks: 750200
Duplicate of this bug: 750200
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
Status: RESOLVED → VERIFIED
(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.
You need to log in before you can comment on or make changes to this bug.