640068 - Browser does not restart when installing extensions

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Extension Compatibility, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Using the Ideos s7 tablet.

Install an extension that requires a restart or uninstall an extension requiring a restart.
Click the restart Firefox button after the extension installs

results:
Firefox does not restart

expected:
Firefox restarts and the extension is installed

Firefox Mobile build id 20110308121615

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Attachment: logcat of a restart request

Comment 2

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

there's only one line on your log?

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Attachment: logcat of a restart request

The correct log.

Comment 4

[Thomas Arend [:tarend]]

I have seen this on my Nexus One and on a Galaxy Tab, too. Installed TwitterBar, hit Restart. Browser shuts down but doesn't restart.

Comment 5

[Thomas Arend [:tarend]]

Attachment: Logcat of Restart issue on Galaxy Tab (tarend)

Comment 6

[Thomas Arend [:tarend]]

Attachment: Another logcat

This time: trying to install add-on "readability" - same issue - browser quits but fails to restart on my Galaxy Tab

Comment 7

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

Attachment: patch to sleep after the kill

Comment 8

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

Attachment: patch to wait after kill

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 518304 [details] [diff] [review]
patch v.1

spoke to brad.  needs to not hardcode processes, and probably should tokenize the ps output so that if anyone ever installs a different ps (or ps output changes), the right thing will continue to happen.

Comment 11

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

Comment on attachment 518304 [details] [diff] [review]
patch v.1

gave my review comments to dougt on irc

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 14

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

Comment on attachment 518553 [details] [diff] [review]
patch v.2

I'd like to avoid Runtime.exec() if we can. From what I've heard from you, the best way to list processes is to use Runtime.exec("ps"), but let's not open the flood gates because of that. You can use Process.killProcess(pid) rather than Runtime.exed("kill -9 " + pid) and you can use getUidForName(username) to compare the and Process.myUid() to check if the process is one of ours.

As to your comment that killProcess() doesn't work, see the patch I posted last night for how to make that work.

Also, when using Log.i for an exception, you should pass the exception as the third argument rather than appending it to the end of the message.

Comment 15

[Doug Turner (:dougt)]

Attachment: v.3

untested w/ blassey's nits.

please review, and i can test on my devices.

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 518594 [details] [diff] [review]
v.3

fixed the commit line.

Comment 17

[Matt Brubeck (:mbrubeck)]

The build at http://people.mozilla.org/~dougt/fennec.apk restarts fine on Verizon Galaxy Tab.

Comment 18

[Doug Turner (:dougt)]

same on the g2, my tab, and this huawei tablet.

Comment 19

[Doug Turner (:dougt)]

Attachment: v.4

Comment 20

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

Comment on attachment 518628 [details] [diff] [review]
v.4


>-                int pid = Integer.parseInt(p.getName());
>+
>+        class GeckoPidCallback implements GeckoProcessesVisitor {
nit, there's an extra blank line here, remove it

>+                    try {
>+                        Thread.currentThread().sleep(100);
>+                    } catch (InterruptedException ie) {}
>+                }
>+            }
>         } catch (Exception e) {
>             Log.i("Restarter", e.toString());
While you're here, use the 3 argument version of the log method

Comment 21

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/823105711a3b

missed the followups (sry).

Comment 22

[Cristian Nicolae (:xti)]

This issue is still reproducing on Sony Ericsson Xperia X10 - Android 2.1-update1
Please see the following video: http://www.youtube.com/user/qaioana#p/a/u/0/lckokurNxfg

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

Comment 23

[Cristian Nicolae (:xti)]

The video from above is a mistake. Sorry

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #23)
> The video from above is a mistake. Sorry

What kind of mistake? Does the problem still happen or not?

Comment 25

[Cristian Nicolae (:xti)]

This part of my comment shouldn't exits:
"Please see the following video:
http://www.youtube.com/user/qaioana#p/a/u/0/lckokurNxfg"

The issue is still reproducing on the:
Build id : Mozilla/5.0 (Maemo;Linux armv7l;rv:2.0b13pre)Gecko/201100310
Firefox/4.0b13pre Fennec /4.0b6pre

Comment 26

[Cristian Nicolae (:xti)]

It seems to be fixed on the latest nightly Build id : Mozilla/5.0 (Maemo;Linux armv7l;rv:2.0b13pre)Gecko/201100311
Firefox/4.0b13pre Fennec /4.0b6pre

Comment 27

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

(In reply to comment #26)
> It seems to be fixed on the latest nightly Build id : Mozilla/5.0 (Maemo;Linux
> armv7l;rv:2.0b13pre)Gecko/201100311
> Firefox/4.0b13pre Fennec /4.0b6pre

that's yesterday's build and doesn't have the patches from this bug

Comment 28

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

err.. the 3/10 build was yesterday's you already figured that out

Comment 29

[Aaron Train [:aaronmt]]

Verified fixed w/
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre) Gecko/20110317 Firefox/4.0b13pre Fennec/4.0b6pre