696315 - Menu > Quit is not really closing the application

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

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

Try to close the app using the Menu > Quit command. Then use:
 adb shell ps | grep fennec

Fennec is still running.

Comment 1

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

(In reply to Mark Finkle (:mfinkle) from comment #0)
> Try to close the app using the Menu > Quit command. Then use:
>  adb shell ps | grep fennec
> 
> Fennec is still running.

I can't reproduce. Any STR besides starting the app and choosing quit from the menu?

Comment 2

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

I opened some tabs. Doug backed out a patch you landed that might have been causing this. Make sure your code is active when testing:

http://hg.mozilla.org/projects/birch/rev/1b7748fbdf19

Comment 3

[Doug Turner (:dougt)]

I think what we can do is use appStartup->Quit(nsIAppStartup::eForceQuit) and change finish() to System.exit().

Comment 4

[Brian Nicholson (:bnicholson)]

System.exit() prevents onDestroy() from getting called. This is breaking things that need to be saved when we shut down (like preferences and the session store).

Comment 5

[Doug Turner (:dougt)]

brian, can you attach the patch that i sent you?

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: patch

Comment 7

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

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

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

::: embedding/android/GeckoApp.java
@@ +571,5 @@
>          finish();
> +
> +        // I think we want to delay this for a few seconds so that android can clean things up after the call to finish()
> +        //
> +        //        System.exit(0);

yes, I think that's the right thing to do. Essentially try to quit "right" with finish() and have a killer thread that forces us to die after a timeout.

Comment 9

[Doug Turner (:dougt)]

Attachment: v.1

Comment 10

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

Comment on attachment 571553 [details] [diff] [review]
v.1

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

::: toolkit/xre/nsAndroidStartup.cpp
@@ +187,5 @@
>  extern "C" NS_EXPORT void JNICALL
>  Java_org_mozilla_gecko_GeckoAppShell_nativeQuit(JNIEnv*, jclass)
>  {
> +    nsCOMPtr<nsIRunnable> death = new QuitCall();
> +    NS_DispatchToMainThread(death);

Is there any reason this needs to be on the main thread? If so, that should probably be documented in the idl and an assertion added to the nsAppStartup::Quit(). If not, why are you doing it?

Comment 11

[Doug Turner (:dougt)]

I can file a followup to document it.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 571553 [details] [diff] [review]
v.1

this has a minor problem that:

1) if the user does menu->Quit, then reopens immediately, bad things will happen.

Comment 13

[Doug Turner (:dougt)]

Attachment: backout

this backs out:

http://hg.mozilla.org/projects/birch/rev/653aa43e9643

and removes the Quit menu.  I am leaving the string resource and the image in case someone wants to write an addon.

Comment 14

[Brian Nicholson (:bnicholson)]

r+ with rememberLastScreen put back in

Comment 15

[Doug Turner (:dougt)]

http://hg.mozilla.org/projects/birch/rev/03c53ee42fc9

Comment 16

[Aaron Train [:aaronmt]]

20111103040334
http://hg.mozilla.org/projects/birch/rev/c09a52af4cd8
Samsung Nexus S (2.3.6)

Comment 17

[Matt Brubeck (:mbrubeck)]

These patches were backed while investigating Talos failures.  We will need to wait until Talos dhtml/svg/tp4m-nochrome/sunspider are green again on birch before trying to re-land them:

https://hg.mozilla.org/projects/birch/rev/3e1e9e88c794
https://hg.mozilla.org/projects/birch/rev/d34b5885abe8

Comment 18

[Matt Brubeck (:mbrubeck)]

Re-landed because the backouts failed to fix Talos:
https://hg.mozilla.org/projects/birch/rev/a2f776f67fa1

Comment 19

[Wesley Johnston (:wesj)]

These patches were backed out for causing talos failures.

Comment 20

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

backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Comment 21

[Aaron Train [:aaronmt]]

(In reply to Brad Lassey [:blassey] from comment #20)
> backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Empty?

Comment 22

[Catalin Suciu [:csuciu]]

Verified fixed on the latest fennec native build:
Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111128 Firefox/11.0a1 Fennec/11.0a1
Samsung GalaxyS, Android 2.2