Last Comment Bug 696315 - Menu > Quit is not really closing the application
: Menu > Quit is not really closing the application
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86_64 Windows 7
: P1 normal (vote)
: ---
Assigned To: Doug Turner (:dougt)
: Sebastian Kaspari (:sebastian)
: 699160 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-10-20 21:30 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2016-07-29 14:20 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.16 KB, patch)
2011-11-01 23:20 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
v.1 (3.12 KB, patch)
2011-11-02 21:19 PDT, Doug Turner (:dougt)
doug.turner: review-
Details | Diff | Splinter Review
backout (5.87 KB, patch)
2011-11-02 22:03 PDT, Doug Turner (:dougt)
bnicholson: review+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-10-20 21:30:28 PDT
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?) 2011-10-24 19:24:30 PDT
(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?) 2011-10-24 19:29:51 PDT
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:
Comment 3 Doug Turner (:dougt) 2011-10-27 13:10:07 PDT
I think what we can do is use appStartup->Quit(nsIAppStartup::eForceQuit) and change finish() to System.exit().
Comment 4 Brian Nicholson (:bnicholson) 2011-11-01 15:50:33 PDT
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) 2011-11-01 18:51:20 PDT
brian, can you attach the patch that i sent you?
Comment 6 Brian Nicholson (:bnicholson) 2011-11-01 23:20:54 PDT
Created attachment 571250 [details] [diff] [review]
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2011-11-01 23:42:27 PDT
Comment on attachment 571250 [details] [diff] [review]

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

::: embedding/android/
@@ +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 8 Brian Nicholson (:bnicholson) 2011-11-02 11:40:02 PDT
*** Bug 699160 has been marked as a duplicate of this bug. ***
Comment 9 Doug Turner (:dougt) 2011-11-02 21:19:45 PDT
Created attachment 571553 [details] [diff] [review]
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2011-11-02 21:23:40 PDT
Comment on attachment 571553 [details] [diff] [review]

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) 2011-11-02 21:27:51 PDT
I can file a followup to document it.
Comment 12 Doug Turner (:dougt) 2011-11-02 21:36:45 PDT
Comment on attachment 571553 [details] [diff] [review]

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) 2011-11-02 22:03:35 PDT
Created attachment 571559 [details] [diff] [review]

this backs out:

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) 2011-11-02 22:32:18 PDT
r+ with rememberLastScreen put back in
Comment 15 Doug Turner (:dougt) 2011-11-02 23:50:53 PDT
Comment 16 Aaron Train [:aaronmt] 2011-11-03 06:25:22 PDT
Samsung Nexus S (2.3.6)
Comment 17 Matt Brubeck (:mbrubeck) 2011-11-09 16:33:12 PST
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:
Comment 18 Matt Brubeck (:mbrubeck) 2011-11-09 17:32:48 PST
Re-landed because the backouts failed to fix Talos:
Comment 19 Wesley Johnston (:wesj) 2011-11-10 10:21:41 PST
These patches were backed out for causing talos failures.
Comment 20 Brad Lassey [:blassey] (use needinfo?) 2011-11-11 08:43:23 PST
backout was backed out
Comment 21 Aaron Train [:aaronmt] 2011-11-14 06:44:22 PST
(In reply to Brad Lassey [:blassey] from comment #20)
> backout was backed out

Comment 22 Catalin Suciu [:csuciu] 2011-11-29 00:13:36 PST
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

Note You need to log in before you can comment on or make changes to this bug.