1118216 - crash in java.lang.IllegalStateException: This message cannot be recycled because it is still in use. at android.os.Message.recycle(Message.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

This bug was filed from the Socorro interface and is 
report bp-d072a8c1-e9f0-4a8c-8d02-5ca6b2150106.
=============================================================
Device: Nexus 5(Android 5.0.1); Firefox 35 Beta 10
I don't have STR for this issue, just browsing around when this happened.
From what I see in the crash reports, this is reproducing only on Android Lollipop, and most of the crashes are happening in Beta.

Comment 1

[Aaron Train [:aaronmt]]

java.lang.IllegalStateException: This message cannot be recycled because it is still in use.
	at android.os.Message.recycle(Message.java:279)
	at org.mozilla.gecko.GeckoAppShell.pumpMessageLoop(GeckoAppShell.java:2447)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:325)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:184)

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

I got this in Nightly today. I thought I had already fixed it, and it appears I tried in bug 1055166. I am not sure if this additional msg.recycle() snuck in or if I am just dumb.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Catch IllegalStateException when recycling Messages

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah I just flat out missed this other call in the initial patch. I blame the reviewer :)

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8558523 [details] [diff] [review]
Catch IllegalStateException when recycling Messages

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

This change is fine, but:

https://groups.google.com/forum/?fromgroups#!topic/android-developers/9pHuc7lGunY

"When in doubt, don't call Message.recycle().  It is just an optimization.  The platform does this for you when it is done dispatching a message.

And yes: the platform does this for you when done dispatching a message, so when you handler, you no longer own the message and should not touch that object any more."


So I think it might be worth reconsidering whether we need to do this at all, assuming that all of our messages end up being dispatched.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Richard Newman [:rnewman] from comment #5)
Now that you mention it, I think that's why I didn't change the other call here, because it never got dispatched. I wonder why it's throwing. I think maybe we do want to just stop calling recycle().

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Stop recycling Message instances, as it's unnecessary

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8558581 [details] [diff] [review]
Stop recycling Message instances, as it's unnecessary

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

Keep an eye on the log, wait until after a GC, make sure there aren't any scary messages about growing the message pool or finalizing messages.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05ecd4d258b2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05ecd4d258b2

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

This looks like a pretty simple crash fix. As 37 is affected, what do you think about uplifting the fix for Beta 4 (gtb Mon, Mar 9)?

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #11)
> This looks like a pretty simple crash fix. As 37 is affected, what do you
> think about uplifting the fix for Beta 4 (gtb Mon, Mar 9)?

Seems fine to me

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8558581 [details] [diff] [review]
Stop recycling Message instances, as it's unnecessary

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: nightly, aurora
[Risks and why]: very low
[String/UUID change made/needed]: none

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8558581 [details] [diff] [review]
Stop recycling Message instances, as it's unnecessary

Pretty simple crash fix that has been on m-c for a month. Let's get this into Beta 4. Beta+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a09088484ddf

Comment 16

[Catalin Suciu [:csuciu]]

Using the following STR's on Android 5.0.x, I was able to reproduce this crash on FF 36 Release and FF 37 Beta 2.
1. Go to intel.com/museumofme
2. Play the video in fullscreen 

Result: FF crashes

On FF 37 Beta 4, as well as on FF 38 an FF 39, when following this steps, FF becomes unresponsive
This issue was reported here: https://bugzilla.mozilla.org/show_bug.cgi?id=1140359

Comment 17

[u421692]

I am reproducing this issue on Nexus 7(Android 5.0.2) on Firefox 36.0.2 while playing a video on youtube.com (desktop site).
I see that this crash is WONTFIX on FF 36, should we consider uplifting this to release branch, since it affects Youtube desktop site?
PS: On big tablets, desktop site is by default on Youtube.com.

Comment 18

[u421692]

On HTC Nexus 9 (Android 5.0.1) the crash reproduces only by tapping "tap to play" on flash content

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Passing ni to Sylvestre, who is managing 36.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

This bug was present in 35 already, what changed (besides that we have a fix)? :) thanks

Comment 21

[u421692]

Nothing has changed, we've realized that Firefox crashes every time when playing videos using flash on Android 5.0, and this has a big user impact. I was thinking it's an important issue for a release build.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Ok, I agree the impact is important but as we are going to release 37 soon, we already built 36.0.2 and we have this bug for a while.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

James, we have to make a second build for 36.0.2 and we could take this patch. Do you think we should do it? If yes, could you fill the uplift request? Thanks

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8558581 [details] [diff] [review]
Stop recycling Message instances, as it's unnecessary

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: some crashes when using flash?
[Describe test coverage new/current, TreeHerder]: nightly, aurora
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/a541bb82052f

Comment 26

[u421692]

Verified as fixed on Firefox 36.0.2 Build2 on Nexus 5 (Android 5.0.2) and Nexus 7 (Android 5.0.2).