Menu > Quit is not really closing the application

VERIFIED FIXED

Status

()

defect
P1
normal
VERIFIED FIXED
8 years ago
3 years 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

8 years ago
Assignee: nobody → blassey.bugs
Assignee

Updated

8 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

8 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

8 years ago
brian, can you attach the patch that i sent you?
Posted patch patch (obsolete) — Splinter Review
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

8 years ago
Posted patch v.1Splinter Review
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

8 years ago
I can file a followup to document it.
Assignee

Updated

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

Comment 12

8 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

8 years ago
Posted patch backoutSplinter Review
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

8 years ago
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
Closed: 8 years ago8 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
Closed: 8 years ago8 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+
You need to log in before you can comment on or make changes to this bug.