Menu > Quit is not really closing the application

VERIFIED FIXED

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: mfinkle, Assigned: dougt)

Tracking

unspecified
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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

Fennec is still running.
(Assignee)

Updated

6 years ago
Assignee: nobody → blassey.bugs
(Assignee)

Updated

6 years ago
Priority: -- → P1
(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?
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
(Assignee)

Comment 3

6 years ago
I think what we can do is use appStartup->Quit(nsIAppStartup::eForceQuit) and change finish() to System.exit().
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).
(Assignee)

Comment 5

6 years ago
brian, can you attach the patch that i sent you?
Created attachment 571250 [details] [diff] [review]
patch
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.
Duplicate of this bug: 699160
(Assignee)

Comment 9

6 years ago
Created attachment 571553 [details] [diff] [review]
v.1
Assignee: blassey.bugs → doug.turner
Attachment #571250 - Attachment is obsolete: true
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?
(Assignee)

Comment 11

6 years ago
I can file a followup to document it.
(Assignee)

Updated

6 years ago
Attachment #571553 - Flags: review?(blassey.bugs)
(Assignee)

Comment 12

6 years ago
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.
Attachment #571553 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 13

6 years ago
Created attachment 571559 [details] [diff] [review]
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.
Attachment #571559 - Flags: review?
Attachment #571559 - Flags: review? → review+
r+ with rememberLastScreen put back in
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/projects/birch/rev/03c53ee42fc9
20111103040334
http://hg.mozilla.org/projects/birch/rev/c09a52af4cd8
Samsung Nexus S (2.3.6)
Status: RESOLVED → VERIFIED
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Re-landed because the backouts failed to fix Talos:
https://hg.mozilla.org/projects/birch/rev/a2f776f67fa1
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
These patches were backed out for causing talos failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(In reply to Brad Lassey [:blassey] from comment #20)
> backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Empty?
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
Status: RESOLVED → VERIFIED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.