906030 - Regression: Unable to start a Guest Session; menu item stuck on 'Exit Guest Session'

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Open the menu, see 'Exit Guest Session'. Selecting that will restart Fennec with the same profile.

--
Nightly (08/16)

Comment 1

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-08-14&enddate=2013-08-15

Regression from bug 901903

Comment 2

[Aaron Train [:aaronmt]]

Interestingly I don't see this issue on my 4.2.2


On my 4.3 devices (Nexus 4, Galaxy SIV)

Startup:

I/GeckoThread(31177): RunGecko - args =  -profile /data/data/org.mozilla.fennec/files/mozilla/tz4pyxxg.guest

On entry 'Exit Guest Session' shown as the item name

W/GeckoProfile(31421): requested profile directory missing: /data/data/org.mozilla.fennec/files/guest

It restarts with the default profile

I/GeckoThread(31421): RunGecko - args =  -profile /data/data/org.mozilla.fennec/files/mozilla/tz4pyxxg.guest

On my HTC One 4.2.2

On entry it works as expected and the name of the item is shown correctly

I/GeckoThread(23330): RunGecko - args = --guest-mode -profile /data/data/org.mozilla.fennec/files/guest

Comment 3

[Aaron Train [:aaronmt]]

Reproduced on the new Nexus 7 (2013) on fresh install (4.3) as well

Comment 4

[Aaron Train [:aaronmt]]

Unable to reproduce on the Galaxy Note II (4.2)

Comment 5

[Aaron Train [:aaronmt]]

Scratch that, I nuked an older install and re-installed cleanly and was able to reproduce. Having an older install prior to the mentioned bug seems to have things work out fine. New installations seem to be faulty.

Comment 6

[Wesley Johnston (:wesj)]

Grr. Yeah, this exposed a separate bug. i.e. before we were creating the directory so creating a profile with

GeckoProfile(context, "guest", file);

would use the File passed in. Now, since that file doesn't exist, GeckoProfile creates it and happily adds it to profiles.ini for us at the same time.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

This basically

1.) Assumes that getGuestProfile always returns something
2.) Is careful not to create a profile before locking it (I also added a similar check in unlock just to be consistent here)

I'm working on a larger refactor of this (and shilpan is deleting all of the profiles.ini code). In the mean time, I think this is a pretty simple fix.

Comment 8

[Wesley Johnston (:wesj)]

With this patch, users who've installed nightly since the switch will still be stuck in a "guest" profile forever. I can newsgroup post if we want, or we could ship some code to get them out if their profile name is "guest".

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Fix return values for maybeCleanupGuestProfile

I found several issues looking through the guest profile code, so I'll try to fix them all here.

I found that maybeCleanupGuestProfile() isn't returning what we'd expect. In BrowserApp, we set mRestoreMode to RESTORE_NORMAL if maybeCleanupGuestProfile() is true, so that implies that the return value should be the current guest mode status. This patch fixes that.

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Don't always create the guest profile at startup

Oh great, looks like we've been working on the same bug! If only we hadn't just had that fire drill, I would have gotten mine up first.... Anyway, I'll just post the rest and we can decide which ones to take after that.

Here's the second part. getGuestProfile() was creating the profile if it didn't exist, so the guest profile was still getting set at every startup. This changes getGuestProfile() to only call get() if the profile actually exists already.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 794283 [details] [diff] [review]
Part 1: Fix return values for maybeCleanupGuestProfile

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

::: mobile/android/base/GeckoProfile.java
@@ +181,5 @@
> +    /**
> +     * Removes the guest profile if guest mode is no longer active.
> +     *
> +     * @param context Context for retrieving profile path
> +     * @return        Whether the user is still in guest mode

This isn't what I imagined this return was for. In my mind, since this method was a "maybe" method, the return told you whether or not it actually did what you asked. i.e. return true if we deleted a guest profile. Then browserApp would restore session if we were deleting the profile (i.e. coming out of guest mode).

With this patch, we'll restore session if the guest profile exists but isn't locked?

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Don't remove guest profile on background thread

There's a race condition removing the guest profile since it happens in the background thread. If getGuestProfile() is called any time before the background thread has finished, mInGuestMode will be set to true even though we're not actually in guest mode anymore.

This could be fixed by adding synchronization logic to make sure we don't try to call getGuestProfile() before a remove has finished, but that would add more overhead to every startup and wouldn't be a good idea. We could also maybe add some booleans and/or reorder the code to fix this, but I chose to just get rid of the background thread altogether. We'll only hit this code path to begin with when we're leaving guest mode (not that common), plus we already create the guest profile synchronously anyway.

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: 0004-Bug-906030-Part-4-Don-t-catch-exceptions-in-createGu.patch

Final patch. This removes the exception catching in createGuestProfile(). In BrowserApp, we set the result to mProfile whether we succeed or not, so we'll crash either way. Catching generic exceptions here only makes things more difficult to debug since we'll get a NPE instead of the actual stack trace.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 794286 [details] [diff] [review]
Part 2: Don't always create the guest profile at startup

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

I think I like this (although it conflicts with my patch).

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 794283 [details] [diff] [review]
Part 1: Fix return values for maybeCleanupGuestProfile

(In reply to Wesley Johnston (:wesj) from comment #11)
> Comment on attachment 794283 [details] [diff] [review]
> Part 1: Fix return values for maybeCleanupGuestProfile
> 
> With this patch, we'll restore session if the guest profile exists but isn't
> locked?

Oh right, I was thinking we wanted to restore the session if we're still in guest mode, but it's the other way around. Forget this patch!

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 794292 [details] [diff] [review]
0004-Bug-906030-Part-4-Don-t-catch-exceptions-in-createGu.patch

We have null checks, so I think you can ignore this patch too.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 794292 [details] [diff] [review]
0004-Bug-906030-Part-4-Don-t-catch-exceptions-in-createGu.patch

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

::: mobile/android/base/GeckoProfile.java
@@ -150,5 @@
> -            return profile;
> -        } catch (Exception ex) {
> -            Log.e(LOGTAG, "Error creating guest profile", ex);
> -        }
> -        return null;

Hmm.. returning null here won't actually cause a crash though. Most of the time, BrowserApp doesn't assign a GeckoProfile at this point, and life goes on fine.

So with this, rather than gracefully moving on using your normal profile, we'll crash. Maybe we'd be better to show a toast or notification saying "Can't create a guest session :(" to the user. Or an alert dialog?

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 794290 [details] [diff] [review]
Part 3: Don't remove guest profile on background thread

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

I think I'm fine with this too.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 794290 [details] [diff] [review]
Part 3: Don't remove guest profile on background thread

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

I think I'm fine with this too.

Comment 20

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/d3d43286b8e0
https://hg.mozilla.org/integration/fx-team/rev/8cdfa0b22222

Marking this as [leave open] for Wes's patch.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d3d43286b8e0
https://hg.mozilla.org/mozilla-central/rev/8cdfa0b22222

Comment 22

[Aaron Train [:aaronmt]]

Verified Fixed on Nightly (08.27)

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 794286 [details] [diff] [review]
Part 2: Don't always create the guest profile at startup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 901903
User impact if declined: Stuck on guest mode
Testing completed (on m-c, etc.): Landed on mc last week.
Risk to taking this patch (and alternatives if risky): Low risk. Fixes a regression from 901903. Needed if we want to land that on aurora.
String or IDL/UUID changes made by this patch: None.

Comment 24

[Wesley Johnston (:wesj)]

Comment on attachment 794290 [details] [diff] [review]
Part 3: Don't remove guest profile on background thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 901903
User impact if declined: Stuck on guest mode
Testing completed (on m-c, etc.): Landed on mc last week.
Risk to taking this patch (and alternatives if risky): Low risk. Fixes a regression from 901903. Needed if we want to land that on aurora.
String or IDL/UUID changes made by this patch: None.

Comment 25

[Brian Nicholson (:bnicholson)]

Bug 901903 exposes this bug, and since bug 901903 is going to land on Aurora, this affects 25.

Comment 26

[Brian Nicholson (:bnicholson)]

Wes, before I look anymore at your patch, do we still want it considering the other patches here landed?

Comment 27

[Wesley Johnston (:wesj)]

Hmm.. Maybe? That patch makes the locking code a little smarter, but more complex, to avoid this problem. Its pretty simple, so I'd be happy to get your take on it. Low priority?

I'm hoping that once profiles.ini is dead we can look for a way to do this without the locking bits, but we'll still need to store the prev profile somewhere so that when you leave guest mode, we have some idea what to load (maybe in profiles.ini?). Anyway, separate bug...

Comment 28

[Brian Nicholson (:bnicholson)]

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

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

I'm not sure why I took so long to review this...it's not as confusing as I thought when I first looked at it. LGTM as long as you make sure to keep the null check.

::: mobile/android/base/GeckoProfile.java
@@ -68,5 @@
>              }
>  
>              GeckoProfile guest = GeckoProfile.getGuestProfile(context);
> -            // if the guest profile exists and is locked, return it
> -            if (guest != null && guest.locked()) {

The other patches in this bug changed getGuestProfile() so that it can actually return null, so I think you'll need to keep the null check here.

@@ +233,5 @@
>          if (mLocked != LockState.UNDEFINED) {
>              return mLocked == LockState.LOCKED;
>          }
>  
> +        // Be careful not to create a dir just to see if its locked

This comment makes sense looking at it in the before/after context of this bug, but you might want to be more explicit about what you mean (e.g., "Don't use getDir() here because..."), so that it's clear what you're talking about for someone trying to follow the code.

@@ +264,5 @@
>          return false;
>      }
>  
>      public boolean unlock() {
> +        // Be careful not to create a dir if it doesn't exist yet

Same here with this comment.

Comment 29

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ebc7be739ce
https://hg.mozilla.org/releases/mozilla-aurora/rev/79caa25bdfaf

Comment 30

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/0b479df9a59a

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b479df9a59a

Comment 32

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a2 (2013-09-16) and Firefox for Android 26.0a1 (2013-09-16)
Device: LG Nexus 4
OS: Android 4.2.2