Last Comment Bug 784360 - java.util.ConcurrentModificationException: at java.util.HashMap$HashIterator.nextEntry(HashMap.java) at org.mozilla.gecko.DoorHangerPopup.onTabChanged(DoorHangerPopup.java)
: java.util.ConcurrentModificationException: at java.util.HashMap$HashIterator....
Status: VERIFIED FIXED
[native-crash]
: crash, regression, topcrash
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 17 Branch
: ARM Android
: -- critical (vote)
: Firefox 19
Assigned To: :Margaret Leibovic
:
:
Mentors:
Depends on:
Blocks: 732336
  Show dependency treegraph
 
Reported: 2012-08-21 08:28 PDT by Scoobidiver (away)
Modified: 2013-01-02 14:29 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
verified


Attachments
patch (1.81 KB, patch)
2012-08-29 16:51 PDT, :Margaret Leibovic
wjohnston2000: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-08-21 08:28:24 PDT
It first appeared in 17.0a1/20120806. The regression range is likely:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9453cf029b72&tochange=c8d94fe7506a
It's likely a regression from bug 732336.

Here is a crash report: bp-28152574-c339-4b59-8b99-62f822120821.

java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:796)
	at java.util.HashMap$KeyIterator.next(HashMap.java:823)
	at org.mozilla.gecko.DoorHangerPopup.onTabChanged(DoorHangerPopup.java:99)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:369)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:358)
	at org.mozilla.gecko.Tabs$3.run(Tabs.java:183)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:143)
	at android.app.ActivityThread.main(ActivityThread.java:4196)
	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:839)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:597)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.util.ConcurrentModificationException%3A+at+java.util.HashMap%24HashIterator.nextEntry%28HashMap.java%29
Comment 1 :Margaret Leibovic 2012-08-29 16:51:06 PDT
Created attachment 656673 [details] [diff] [review]
patch

Sigh, threads. I'm not sure if this is the smartest way to avoid a ConcurrentModificationException, but it's what we've done elsewhere. There are also things like CopyOnWriteArrayList, but that feels too heavyweight for this case.
Comment 2 Wesley Johnston (:wesj) 2012-09-11 17:03:17 PDT
Comment on attachment 656673 [details] [diff] [review]
patch

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

I think it'd be cleaner to just use synchronized here. We aren't going to be doing so much with this list that I expect that to happen often. It matches well with the advice from here:

http://developer.android.com/reference/java/util/concurrent/package-summary.html

"A CopyOnWriteArrayList is preferable to a synchronized ArrayList when the expected number of reads and traversals greatly outnumber the number of updates to a list."
Comment 3 :Margaret Leibovic 2012-09-12 02:37:18 PDT
(In reply to Wesley Johnston (:wesj) from comment #2)

> I think it'd be cleaner to just use synchronized here.

Where would we use synchronized? I see above that I mentioned threads, but thinking about it more, the problem isn't that this is being accessed from multiple threads, the problem is that we're modifying the array as we're iterating over it. I've found I can reliably reproduce this crash if I close any tab that has multiple doorhangers (a good testcase is http://people.mozilla.com/~mwargers/tests/dom/multiple_doorhangers.htm).
Comment 4 :Margaret Leibovic 2012-09-21 11:58:11 PDT
Wes, poke. See comment 3.
Comment 5 Scoobidiver (away) 2012-10-14 02:03:13 PDT
It's #8 top crasher in 17.0b1.
Comment 6 :Margaret Leibovic 2012-10-15 10:36:02 PDT
Comment on attachment 656673 [details] [diff] [review]
patch

Wes, can you re-review this? See comment 3.
Comment 7 Wesley Johnston (:wesj) 2012-10-15 10:39:24 PDT
Comment on attachment 656673 [details] [diff] [review]
patch

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

Ok. I can live with this.
Comment 9 :Margaret Leibovic 2012-10-15 16:27:14 PDT
Comment on attachment 656673 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): crash caused by bug 732336
User impact if declined: crash if you close a tab with multiple doorhanger notification messages
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk small self-contained change
String or UUID changes made by this patch: n/a
Comment 10 Ed Morley [:emorley] 2012-10-16 01:24:12 PDT
https://hg.mozilla.org/mozilla-central/rev/8595e313fc21
Comment 12 Kevin Brosnan [:kbrosnan] 2013-01-02 11:52:32 PST
This is still crashing on Firefox 17.
Comment 13 :Margaret Leibovic 2013-01-02 11:58:12 PST
(In reply to Kevin Brosnan [:kbrosnan] from comment #12)
> This is still crashing on Firefox 17.

The stacks for the recent crashes look different. I looked at the most recent 10, and they don't mention DoorHangerPopup at all. Please file a new bug.
Comment 14 Kevin Brosnan [:kbrosnan] 2013-01-02 14:29:10 PST
Verified then. I don't see any popup hanger crashes either.

Note You need to log in before you can comment on or make changes to this bug.