719795 - SMS related Android crashes. Device rotation in XUL builds and language change in native builds [@ TouchBadMemory][@ GeckoSmsManager.init]

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Bill Gianopoulos [:WG9s]]

bp-8ed31cc2-df21-43a5-a3b7-f70742120120

This crash is easily reproducable on my Samsung Galaxy Tab 8.9.

Navigate to http://www.wg9s.com/ in Landscape.

Rotate to portrait.

Comment 1

[Aaron Train [:aaronmt]]

Build? Log?

I can't reproduce on my Galaxy Tab 10.1 (Android 3.1) w/ Nightly (01/20) nor Aurora (01/20).

Comment 3

[Bill Gianopoulos [:WG9s]]

Not a dupe.  Bug 719373 is a Fennec Native bug.  This is Fennec XUL crash.

Comment 4

[Bill Gianopoulos [:WG9s]]

I just verified that this issue is in the XUL build ONLY.

Comment 5

[Bill Gianopoulos [:WG9s]]

(In reply to Aaron Train [:aaronmt] from comment #1)
> Build? Log?
> 
> I can't reproduce on my Galaxy Tab 10.1 (Android 3.1) w/ Nightly (01/20) nor
> Aurora (01/20).

This is with the Android XUL official nightly located here:

http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2012-01-20-03-11-25-mozilla-central-android-xul/fennec-12.0a1.multi.android-arm.apk

Comment 6

[Scoobidiver (away)]

(In reply to Bill Gianopoulos [:WG9s] from comment #3)
> Not a dupe.  Bug 719373 is a Fennec Native bug.  This is Fennec XUL crash.
In addition, bug 719373 is a meta bug so each bug with STR should be marked as dependent.

Comment 7

[Bill Gianopoulos [:WG9s]]

Regression window:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f4049f65efc6&tochange=58e933465c36

Comment 8

[Bill Gianopoulos [:WG9s]]

I am currently doing an hg bisect withing that window.

Comment 9

[Bill Gianopoulos [:WG9s]]

Bisect found this:

The first bad revision is:
changeset:   84713:2b834066b4ab
user:        Mounir Lamouri <[e-mail]>
date:        Mon Dec 19 11:16:39 2011 +0100
summary:     Bug 674725 - Part AB - Create a thread to handle SMS IO on Android. r=cjones

Comment 10

[Bill Gianopoulos [:WG9s]]

Building with "ac_add_options --disable-websms-backend" in .mozconfig is sufficient to avoid this crash.

Comment 11

[Bill Gianopoulos [:WG9s]]

This does not make a great deal of sense to me because:

1.  What does my website have to do with SMS?
2.  What does rotating the device have to do with SMS?
3.  Why does the same device not crash under native UI builds? (So, it can't just be blamed on some Honeycomb issue).

One thing to note is that SMS makes little sense on my device as it is WIFI only.  I am not sure if that fact has anything to do with why it crashes.

Comment 12

[Robert Kaiser]

I'm seeing this on every orientation change on my TF101. As AFAIK we are still planning to ship the XUL version of 12 to tablet users, we really should look into this.

Comment 13

[Matt Brubeck (:mbrubeck)]

I can reproduce this bug and am looking into it.  (It happens on all my Android devices, not just Honeycomb tablets.)

Comment 14

[Scoobidiver (away)]

It's #2 top crasher in Fennec 12.0a1.

Comment 15

[Matt Brubeck (:mbrubeck)]

Here's the stack trace from the logs.  The crash happens because rotating the device destroys and re-creates the activity.  When onCreate calls GeckoSmsManager.init a second time, it crashes trying to start a thread that is already running.

I/GeckoApp( 4538): pause
I/GeckoApp( 4538): stop
I/GeckoApp( 4538): destroy
I/GeckoSurfaceView( 4538): surface destroyed
I/GeckoApp( 4538): create
E/GeckoApp( 4538): top level exception
E/GeckoApp( 4538): java.lang.RuntimeException: Unable to start activity ComponentInfo{org.mozilla.fennec/org.mozilla.fennec.App}: java.lang.IllegalThreadStateException: Thread already started.
E/GeckoApp( 4538): 	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:1651)
E/GeckoApp( 4538): 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:1667)
E/GeckoApp( 4538): 	at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:2836)
E/GeckoApp( 4538): 	at android.app.ActivityThread.access$1600(ActivityThread.java:117)
E/GeckoApp( 4538): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:939)
E/GeckoApp( 4538): 	at android.os.Handler.dispatchMessage(Handler.java:99)
E/GeckoApp( 4538): 	at android.os.Looper.loop(Looper.java:130)
E/GeckoApp( 4538): 	at org.mozilla.gecko.GeckoApp$3.run(GeckoApp.java:356)
E/GeckoApp( 4538): 	at android.os.Handler.handleCallback(Handler.java:587)
E/GeckoApp( 4538): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoApp( 4538): 	at android.os.Looper.loop(Looper.java:130)
E/GeckoApp( 4538): 	at android.app.ActivityThread.main(ActivityThread.java:3687)
E/GeckoApp( 4538): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoApp( 4538): 	at java.lang.reflect.Method.invoke(Method.java:507)
E/GeckoApp( 4538): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:842)
E/GeckoApp( 4538): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:600)
E/GeckoApp( 4538): 	at dalvik.system.NativeStart.main(Native Method)
E/GeckoApp( 4538): Caused by: java.lang.IllegalThreadStateException: Thread already started.
E/GeckoApp( 4538): 	at java.lang.Thread.start(Thread.java:1227)
E/GeckoApp( 4538): 	at org.mozilla.gecko.GeckoSmsManager.init(GeckoSmsManager.java:367)
E/GeckoApp( 4538): 	at org.mozilla.gecko.GeckoApp.onCreate(GeckoApp.java:417)
E/GeckoApp( 4538): 	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1047)
E/GeckoApp( 4538): 	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:1615)
E/GeckoApp( 4538): 	... 16 more
D/Zygote  (   74): Process 4538 exited cleanly (1)
I/ActivityManager(  113): Process org.mozilla.fennec (pid 4538) has died.

Comment 16

[Matt Brubeck (:mbrubeck)]

This bug also affects Native fennec, though it is harder to reproduce there because native Fennec has configChanges="orientation" in the AndroidManifest for the main activity.  But native Fennec will crash in the same place if the OS locale, font size preference, number of screens, or other configurations change while it is running.

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: XUL Fennec patch

This patch ensures that the SMS thread is started only once, and interrupted only when the activity finishes.  The intent receiver still needs to be added/removed whenever the activity is created/destroyed, since it is attached to the activity.

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: Native Fennec patch

The same change applied to the native Fennec code.  Steps to reproduce the crash in native Fennec:

1. Open Fennec.
2. Open the Android "Settings" app.
3. In the "Language & keyboard" section, change the system language.
4. Return to Fennec.

Comment 19

[Bill Gianopoulos [:WG9s]]

(In reply to Matt Brubeck (:mbrubeck) from comment #17)
> Created attachment 590797 [details] [diff] [review]
> XUL Fennec patch
> 
> This patch ensures that the SMS thread is started only once, and interrupted
> only when the activity finishes.  The intent receiver still needs to be
> added/removed whenever the activity is created/destroyed, since it is
> attached to the activity.

This seems to fix the issue that I originally reported for me.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 590797 [details] [diff] [review]
XUL Fennec patch

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

::: embedding/android/GeckoSmsManager.java
@@ -928,2 @@
>    public void shutdown() {
> -    GeckoApp.mAppContext.unregisterReceiver(this);

nit, call stop() instead

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 590811 [details] [diff] [review]
Native Fennec patch

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

::: mobile/android/base/GeckoApp.java
@@ +1931,5 @@
>  
>          if (SmsManager.getInstance() != null) {
> +            SmsManager.getInstance().stop();
> +            if (isFinishing())
> +                SmsManager.getInstance().shutdown();

the implementation of shutdown() does what stop() does, so only call one or the other:

if (isFinishing())
    SmsManager.getInstance().shutdown();
else
    SmsManager.getInstance().stop();

::: mobile/android/base/GeckoSmsManager.java
@@ -928,2 @@
>    public void shutdown() {
> -    GeckoApp.mAppContext.unregisterReceiver(this);

nit, call stop() instead

Comment 22

[Matt Brubeck (:mbrubeck)]

(In reply to Brad Lassey [:blassey] from comment #20)
> > -    GeckoApp.mAppContext.unregisterReceiver(this);
> 
> nit, call stop() instead

This line was removed by the patch.  Ignoring this comment, with Brad's permission via IRC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cafa419af61

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7cafa419af61

Comment 24

[Robert Kaiser]

I can verify that rotation doesn't crash XUL trunk any more on my TF101.

The question is if bug 720734, bug 717663 and bug 719373 are the same thing or something else.

Comment 25

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #24)
> The question is if bug 720734, bug 717663 and bug 719373 are the same thing
> or something else.
No. In FennecAndroid, crashes with exceptions are characterized by their exception in App notes, not by their crash signatures. Different crash signatures can be the same crash if they have the same exception in App Notes.

Comment 26

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

fyi, Kairo, scoobidiver is correct.

Comment 27

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

Hit return too quickly.  It will be changing where the Java Crashes will be moving to it's own field rather than in the App Notes for Socorro.  [bug 701390 and Bug 718820/ 721078]

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Matt, this isn't a problem on Aurora, right? If so, please mark the whiteboard not-fennec-11