Closed Bug 636877 Opened 9 years ago Closed 9 years ago

Improve android restarts

Categories

(Core :: Widget: Android, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(5 files, 2 obsolete files)

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.
Attachment #515221 - Flags: review?(doug.turner)
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?
Attachment #515221 - Flags: review?(doug.turner) → review+
Attachment #515221 - Flags: approval2.0?
Attachment #515221 - Flags: approval2.0? → approval2.0+
(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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 636439
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
Attached file restart fail log
Browser fails to restart when installing an add-on or changing the language from preferences.
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?
Attached file 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.
Attached file log3
Restart log (log3) after trying to restart after switching locales and having deleted the Beta Release 5 attached.
Attachment #516287 - Attachment mime type: application/octet-stream → text/plain
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #516352 - Flags: review?(doug.turner)
Attachment #516352 - Flags: approval2.0?
Comment on attachment 516352 [details] [diff] [review]
patch v2

r+ before a?!
Attachment #516352 - Flags: approval2.0?
Whiteboard: [approved-patches-landed]
marking as blocking after talking to dougt on irc
tracking-fennec: --- → 2.0+
+        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.
tracking-fennec: 2.0+ → ---
Attachment #516352 - Flags: review?(doug.turner) → review-
(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
Whiteboard: [approved-patches-landed]
tracking-fennec: --- → 2.0+
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #516352 - Attachment is obsolete: true
Status: REOPENED → NEW
Attachment #516687 - Flags: review?(doug.turner)
Attachment #515221 - Attachment description: patch → patch (landed, needs follow up)
Whiteboard: [needs-review (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.
Attachment #516687 - Flags: review?(doug.turner) → review-
Whiteboard: [needs-review (dougt)]
Attached patch patchSplinter Review
this addresses doug's review comments
Attachment #516687 - Attachment is obsolete: true
Attachment #516999 - Flags: review?(doug.turner)
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()?
(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
Attachment #516999 - Flags: review?(doug.turner) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/6b25a8d97e21
Status: NEW → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, Therefor, due to comment 21 and this one I will reopen the bug.
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.