636877 - Improve android restarts

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Attachment: patch (landed, needs follow up)

In working on bug 635342, I've found that we fail to restart a good percentage of the time. It looks like there is a race condition between the main process shutting down and the restarter process starting up. If it shuts down too quickly, the activity manager won't start the restarter process.

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 515221 [details] [diff] [review]
patch (landed, needs follow up)

why 1s?  the last code waited for longer (4s).  do you anticipate problem such a smaller value?

Comment 2

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

(In reply to comment #1)
> Comment on attachment 515221 [details] [diff] [review]
> patch
> 
> why 1s?  the last code waited for longer (4s).  do you anticipate problem such
> a smaller value?

I don't, the 4s was over kill.

pushed http://hg.mozilla.org/mozilla-central/rev/ad545273888c

Comment 4

[Andreea Pod]

Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110302 Firefox/4.0b13pre Fennec /4.0b6pre
Device: Motorola Droid 2
OS: Android 2.2

This is happening constantly starting with yesterday's build. Reopening the bug.

Comment 5

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

(In reply to comment #4)
> Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110302
> Firefox/4.0b13pre Fennec /4.0b6pre
> Device: Motorola Droid 2
> OS: Android 2.2
> 
> This is happening constantly starting with yesterday's build. Reopening the
> bug.

what is happening constantly? how is it failing? what's on the log?

Comment 6

[Andreea Pod]

Attachment: restart fail log

Browser fails to restart when installing an add-on or changing the language from preferences.

Comment 7

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

I'm assuming that log starts with the browser running and ends after the restart. If that's the case it looks likes things are working. The restart is scheduled, xre exists, the restarter starts and then the application starts up. Then the application pauses and the home screen comes up, presumably because you switched to your log app?

Comment 8

[Andreea Pod]

Attachment: log2

That log starts when Fennec was running and after the language change it failed to restart so after one minute I started it by taping on the shortcut.

I attached now a log that starts when browser is running and stopped it after the required restart of the browser, which didn't happened.

Comment 9

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

Attachment: log3

Restart log (log3) after trying to restart after switching locales and having deleted the Beta Release 5 attached.

Comment 10

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

Attachment: patch v2

two things with this patch, first it fixes the manifest so that the restarter is run in a separate process as it is supposed to. Second rather than setting time outs, this patch waits until old processes die and new processes launch before continuing.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 516352 [details] [diff] [review]
patch v2

r+ before a?!

Comment 12

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

marking as blocking after talking to dougt on irc

Comment 13

[Doug Turner (:dougt)]

+        while (!checkForGeckoProcs()) {
+            // Give the restart process time to start before we die
+            try {
+                Thread.currentThread().sleep(100);
+            } catch (InterruptedException ie) {}


This cool loop forever if somehow we get a fennec process zombied.

Is this why the other look has a wait-for-n-seconds loop?



Can we share checkForGeckoProcs()?  Can it be put into some utility class that is imported by both users?

Also, the logging in those files isn't needed.

Comment 14

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

(In reply to comment #13)
> +        while (!checkForGeckoProcs()) {
> +            // Give the restart process time to start before we die
> +            try {
> +                Thread.currentThread().sleep(100);
> +            } catch (InterruptedException ie) {}
> 
> 
> This cool loop forever if somehow we get a fennec process zombied.
> 
> Is this why the other look has a wait-for-n-seconds loop?

yes, that's what the wait-for-n-seconds loop is for. The restarter takes care of killing this loop if it somehow fails.


> Can we share checkForGeckoProcs()?  Can it be put into some utility class that
> is imported by both users?

I'll look into it

> Also, the logging in those files isn't needed.

Since this code has been broken by unrelated changes at least twice that we know if, I'd like to have this logging in the patch. To the best of my knowledge this code is a lot more resilient than the previous code, but the logging will hopefully help us diagnose any problems that come up down the line. Its not particularly noisy, nor should it effect performance

Comment 15

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

Attachment: patch

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 516687 [details] [diff] [review]
patch

You really don't need killAnyZombiesOnce() in GeckoApp().  Please just inline it and save the function jump.

Can you move this block:

+        int countdown = 40;
+        while (!GeckoAppShell.checkForGeckoProcs() &&  --countdown > 0) {
+            // Give the restart process time to start before we die
+            try {
+                Thread.currentThread().sleep(100);
+            } catch (InterruptedException ie) {}
+        }

Also into the GeckoAppShell.  It is code dup that can be shared.

Comment 17

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

Attachment: patch

this addresses doug's review comments

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 516999 [details] [diff] [review]
patch

     public void onCreate(Bundle savedInstanceState) {
         Log.i("Restarter", "trying to restart @MOZ_APP_NAME@");
         try {
-            killAnyZombies();
+            int countdown = 40;
+            while (GeckoAppShell.checkForGeckoProcs() &&  --countdown > 0) {
+                // Wait for the old process to die before we continue
+                try {
+                    Thread.currentThread().sleep(100);
+                } catch (InterruptedException ie) {}
+            }
+            
+            if (countdown <= 0) // if the countdown expired, something is hung
+                GeckoAppShell.killAnyZombies();


call waitForAnotherGeckoProc()?

Comment 19

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

(In reply to comment #18)
> Comment on attachment 516999 [details] [diff] [review]
> patch
> 
>      public void onCreate(Bundle savedInstanceState) {
>          Log.i("Restarter", "trying to restart @MOZ_APP_NAME@");
>          try {
> -            killAnyZombies();
> +            int countdown = 40;
> +            while (GeckoAppShell.checkForGeckoProcs() &&  --countdown > 0) {
> +                // Wait for the old process to die before we continue
> +                try {
> +                    Thread.currentThread().sleep(100);
> +                } catch (InterruptedException ie) {}
> +            }
> +            
> +            if (countdown <= 0) // if the countdown expired, something is hung
> +                GeckoAppShell.killAnyZombies();
> 
> 
> call waitForAnotherGeckoProc()?

No, that's the opposite logic, waiting for there to be no other processes

Comment 20

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

pushed http://hg.mozilla.org/mozilla-central/rev/6b25a8d97e21

Comment 21

[Cristian Nicolae (:xti)]

The issue is still reproducing when I try to change the language and install/uninstall an addon.

Build id : Mozilla/5.0 (Maemo;Linux armv7l;rv:2.0b13pre)Gecko/20110308
Firefox/4.0b13pre Fennec /4.0b6pre
Device: Sony Ericsson Xperia X10
OS: Android 2.1 update 1

Comment 22

[Ioana Chiorean]

I also encounter problems at restarting after changing language. The application closes and doesn't restart automatically . Therefor, due to comment 20 and this one I will reopen the bug.

Comment 23

[Ioana Chiorean]

Sorry, Therefor, due to comment 21 and this one I will reopen the bug.

Comment 24

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

restarts are working fine for me on my i9000, nexus one and nexus s. If there is a problem specific to your X!0, please file another bug with information such as the log and traces.txt.

Comment 25

[Cristian Nicolae (:xti)]

I followed your suggestion and I file a bug with the related issue. Please have a look on https://bugzilla.mozilla.org/show_bug.cgi?id=640489