895709 - [guest] Clean up guest profile as soon as reasonable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: patch

This adds a GuestModeStatus variable in BrowserApp that determines if we're in guest mode, or have just left it, set by passing a commandline argument in both places. We cleanup the guest profile after the selected tab has finished (and we're still in the LEAVING state). I'm reusing the state for restoring session as well.

There may be some state there where you're never on a tab as it finishes loading... or maybe you kill Fennec right after returning from guest mode, and so a page load never happens. I added a quick call to remove the profile in onDestroy as well (but that's not guaranteed to happen often).

Feedback? or if we're ok with this review?

Comment 2

[Wesley Johnston (:wesj)]

Oh, also our delete code didn't actually work. Now it does.

Comment 3

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

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

drive-by

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>+    private enum GuestModeStatus {
>+        ACTIVE,   // The user is currently in guest mode
>+        LEAVING,  // The user just left guest mode
>+        NONE,     // The user is not in guest mode
>+        UNKNOWN   // Unknown what state the user is in. Used to determine if mGuestStatus is valid yet
>+    }
>+    // Cache the current value of mGuestStatus, since it shouldn't change very often
>+    private GuestModeStatus mGuestStatus = GuestModeStatus.UNKNOWN;

Being so Guest oriented might be odd when we support multiple profiles

>+            case STOP:
>+                // If the selected tab has finished loading, and we're coming out of guest mode, clean up the guest profile
>+                if (Tabs.getInstance().isSelectedTab(tab) && getGuestModeStatus() == GuestModeStatus.LEAVING) {
>+                    ThreadUtils.postToBackgroundThread(new Runnable() {
>+                        @Override
>+                        public void run() {
>+                            GeckoProfile.removeGuestProfile(BrowserApp.this);
>+                            mGuestStatus = GuestModeStatus.NONE;
>+                        }
>+                    });
>+                }

This seems a bit overkill IMO. We could just kill it in onCreate and be done, right?

>     public void onCreate(Bundle savedInstanceState) {

>-        String args = getIntent().getStringExtra("args");
>-        if (args != null && args.contains("--guest-mode")) {
>+        if (getGuestModeStatus() == GuestModeStatus.ACTIVE) {
>             mProfile = GeckoProfile.createGuestProfile(this);
>         }

Add an else if (GuestModeStatus.LEAVING) and just delete the guest profile here and be done with it? It might have a small effect on startup perf that one time they switch back. But in a background thread it won't be too bad.

>+        if (getGuestModeStatus() != GuestModeStatus.ACTIVE) {
>+            GeckoProfile.removeGuestProfile(BrowserApp.this);
>+        }

If onCreate is 100% effective, let's remove this one too

Comment 4

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> This seems a bit overkill IMO. We could just kill it in onCreate and be
> done, right?
Yeah. I'm fine with that. Will move this.

> >     public void onCreate(Bundle savedInstanceState) {
> 
> >-        String args = getIntent().getStringExtra("args");
> >-        if (args != null && args.contains("--guest-mode")) {
> >+        if (getGuestModeStatus() == GuestModeStatus.ACTIVE) {
> >             mProfile = GeckoProfile.createGuestProfile(this);
> >         }
> 
> Add an else if (GuestModeStatus.LEAVING) and just delete the guest profile
> here and be done with it? It might have a small effect on startup perf that
> one time they switch back. But in a background thread it won't be too bad.
> 
> >+        if (getGuestModeStatus() != GuestModeStatus.ACTIVE) {
> >+            GeckoProfile.removeGuestProfile(BrowserApp.this);
> >+        }
> 
> If onCreate is 100% effective, let's remove this one too

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

Moved the cleanup to just happen in onCreate() (but still OMT).

Comment 6

[Brian Nicholson (:bnicholson)]

What if we're killed in the background, or the guest kills Fennec from the recent apps list? There's the question of how to handle the session (bug 896092), but in the context of this patch, I think we'll have an issue with the profile not getting deleted. The guest profile is removed only after restarting Fennec with the --leaving-guest-mode argument, and that won't happen if Fennec is killed another way.

Comment 7

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

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> What if we're killed in the background, or the guest kills Fennec from the
> recent apps list? There's the question of how to handle the session (bug
> 896092), but in the context of this patch, I think we'll have an issue with
> the profile not getting deleted. The guest profile is removed only after
> restarting Fennec with the --leaving-guest-mode argument, and that won't
> happen if Fennec is killed another way.

* Killed in the background should mean we stay in guest mode
* Killed via the recent apps list should probably stay in guest mode too, but I could be convinced otherwise.

Leaving guest mode should only happen by explicitly "Leaving guest mode" menu action.

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> * Killed in the background should mean we stay in guest mode
> * Killed via the recent apps list should probably stay in guest mode too,
> but I could be convinced otherwise.
> 
> Leaving guest mode should only happen by explicitly "Leaving guest mode"
> menu action.

Yeah, I was thinking the same thing. We'll need to read some kind of state at every startup to determine whether we're in guest mode already -- either a pref, or checking the profile on disk.

Comment 9

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

Comment on attachment 779606 [details] [diff] [review]
Patch

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

::: mobile/android/base/BrowserApp.java
@@ +155,5 @@
> +        NONE,     // The user is not in guest mode
> +        UNKNOWN   // Unknown what state the user is in. Used to determine if mGuestStatus is valid yet
> +    }
> +    // Cache the current value of mGuestStatus, since it shouldn't change very often
> +    private GuestModeStatus mGuestStatus = GuestModeStatus.UNKNOWN;

I'd rather guest mode status be tracked in GeckoProfile

@@ -219,2 @@
>              case LOAD_ERROR:
> -            case STOP:

why?

@@ +1828,5 @@
>              public void onPromptFinished(String result) {
>                  try {
>                      int itemId = new JSONObject(result).getInt("button");
>                      if (itemId == 0) {
> +                        String args = "--leaving-guest-mode";

I think this over complicates things. Why not just look to see if the guest profile exists and we're not in guest mode?

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

After talking to finkle a bit, we need to find a way to always restart in guest mode until the user explicitly leaves. We're going to make that a separate bug though, so here I'm just cleaning up the guest mode folder any time you start and aren't in guest mode.

That means we do a file.exists() on every startup, but we can do it on the background thread.

Comment 11

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

Comment on attachment 781772 [details] [diff] [review]
Patch

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

r=blassey with nits addressed.

::: mobile/android/base/BrowserApp.java
@@ +396,5 @@
>              mProfile = GeckoProfile.createGuestProfile(this);
> +        } else {
> +            ThreadUtils.postToBackgroundThread(new Runnable() {
> +                @Override
> +                public synchronized void run() {

why is this synchronized?

In effect, it won't have any effect because it is locking on this which is the "new Runnable()" and no one else has a reference to it.

::: mobile/android/base/GeckoProfile.java
@@ +149,5 @@
> +            // Empty directories before deleting them
> +            String files[] = file.list();
> +            for (String temp : files) {
> +                File fileDelete = new File(file, temp);
> +                delete(fileDelete);

we shouldn't have to recursively delete everything in the profile. This warrants some investigation. Ideally we just want to unlink the directory, which is about as fast of a disk io operation as you can do.

I'm fine to do this as a follow up, but please do file it. In the mean time, perhaps try doing file.delete() right away and return early if it returns true. Otherwise fall into this recursive deletion path.

Comment 12

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17773671a1f3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17773671a1f3