1116668 - getGuestProfile does file access on every call

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Profile Handling, defect)

Description

[Richard Newman [:rnewman]]

I see us hit the filesystem (and trigger strictmode) four times during startup.

We shouldn't do this; just as we record that this *is* a guest session, we should be recording that this is *not*.


From ActivityChooserModel:

 	at org.mozilla.gecko.GeckoProfile.getGuestProfile(GeckoProfile.java:304)
 	at org.mozilla.gecko.GuestSession.shouldUse(GuestSession.java:34)
 	at org.mozilla.gecko.GeckoProfile.get(GeckoProfile.java:103)

From GeckoApp.loadStartupTab:

 	at org.mozilla.gecko.GeckoProfile.getGuestProfile(GeckoProfile.java:304)
 	at org.mozilla.gecko.GuestSession.shouldUse(GuestSession.java:34)
 	at org.mozilla.gecko.GeckoProfile.get(GeckoProfile.java:103)

From Tabs, every time we persist!

 	at org.mozilla.gecko.GeckoProfile.getGuestProfile(GeckoProfile.java:304)
 	at org.mozilla.gecko.GuestSession.shouldUse(GuestSession.java:34)
 	at org.mozilla.gecko.GeckoProfile.get(GeckoProfile.java:103)
 	at org.mozilla.gecko.Tabs$PersistTabsRunnable.<init>(Tabs.java:80)
 	at org.mozilla.gecko.Tabs.queuePersistAllTabs(Tabs.java:669)

From BrowserApp.onResume:

 	at org.mozilla.gecko.GeckoProfile.getGuestProfile(GeckoProfile.java:304)
 	at org.mozilla.gecko.GuestSession.shouldUse(GuestSession.java:34)
 	at org.mozilla.gecko.BrowserApp.onResume(BrowserApp.java:754)

Comment 1

[Michael Comella (:mcomella) [needinfo or I won't see it]]

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html

Comment 2

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

Why would this happen over and over? It looks like we use a static to hold the "state" of the GuestProfile. I would only expect one hit.

Comment 3

[Richard Newman [:rnewman]]

We call into GP.get.

mProfile hasn't been set, or the context isn't a GeckoApp, so we call 

        if (GuestSession.shouldUse(context, args)) {

That calls into getGuestProfile to check if there is one that's locked.

Doing so runs this code:

    public static GeckoProfile getGuestProfile(Context context) {
        if (sGuestProfile == null) {
            File guestDir = getGuestDir(context);
            if (guestDir.exists()) {
                sGuestProfile = get(context, GUEST_PROFILE, guestDir);
                sGuestProfile.mInGuestMode = true;
            }
        }

        return sGuestProfile;
    }

As you can see, if there isn't one -- no sGuestProfile, no guest dir -- this will return null, and will result in a stat each time.

Fixes, as I mentioned in Comment 0, going from inside out:

* Record a failure value for sGuestProfile, so we only stat once.
* Don't check the filesystem at all if we can help it. I imagine this is a crash mitigation situation.
* Optionally: set mProfile sooner. This probably comes down to using the wrong context for a profile lookup.
* Additionally: we should be passing explicit profiles to more places. Ideally GeckoApp will fetch the profile once, and will then pass it as a constructor argument to each of its child/attached objects. Less global coupling, and less bullshit like this.

Comment 4

[Dipti Nirmale [:dnirm]]

I'd like to work on this one. 
I'm trying to reproduce this bug by logging each invocation of getGuestProfile(). Once I reproduce the bug, I'll work on the patch. Please let me know if there is any other better way to start on this.

Comment 5

[Dipti Nirmale [:dnirm]]

Attachment: For initial review. Need more information.

The added patch is not working solution. I tried to cache guestDirs for different contexts but it didn't work. Then I added a static flag, since I noticed that it doesn't reset sGuestProfile and instead it loads new vm for each profile. But now it's failing with null pointer exception on creating guest session. 

- For context 'org.mozilla.gecko.BrowserApp@42094ea0' guestDir exists? false
- Error creating guest profile
- java.lang.NullPointerException
-  at org.mozilla.gecko.GeckoProfile.createGuestProfile(GeckoProfile.java:345)
-  at org.mozilla.gecko.GeckoProfile.get(GeckoProfile.java:181)

While reproducing the issue, I noticed that the vm loads the class multiple times for each profile. Added static block in code logs the stack trace at lease twice for default/guest profile.
Is it expected? How many times should it load the class? The number of executions of this block differed in different runs. Knowing this might also help me understand the reason for NullPointerException above. 

Also, I observed StrictMode policy violation occurring many times but couldn't figure out which violation is triggered by file system access. 

Please let me know if am in right direction.

Comment 6

[Richard Newman [:rnewman]]

> The added patch is not working solution. I tried to cache guestDirs for
> different contexts but it didn't work. Then I added a static flag, since I
> noticed that it doesn't reset sGuestProfile and instead it loads new vm for
> each profile.

That shouldn't matter. The problem here is that when we're *not* in guest mode, we check each time -- we cache "yes, we're in guest mode", but not the reverse!

> Is it expected? How many times should it load the class?

If you're switching between guest mode and regular browsing by hand, Firefox will kill the old process.


Comment 0 and Comment 3 give a pretty good overview of what's going on here; if you haven't already read those thoroughly, you should.

Hope that helps!

Comment 7

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

Comment on attachment 8594271 [details] [diff] [review]
For initial review. Need more information.

Why not add an enum for ProfileType { PROFILE_NORMAL, PROFILE_GUEST } and use that to cache the type of the profile. We might add more profile types in the future.

Comment 8

[Dipti Nirmale [:dnirm]]

Attachment: Added flag sIsGuestProfile to cache failure value for sGuestProfile

Due to the newly added flag "non-guest profile", file system is hit only once at the start (guestDir.exists()). Is this the expected behavior or am I missing anything?

Mark- Should we have a new bug for adding Enum ProfileType as it will be easy to track new enhancement.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8595142 [details] [diff] [review]
Added flag sIsGuestProfile to cache failure value for sGuestProfile

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

Sorry for the delayed review, Dipti.

I'm not convinced by the naming here. Saying "yes, we're in guest mode" by default is incorrect; it's really "do we not know that we're not in guest mode" or some other confusing double negative. That flag is only correct after we've called getGuestProfile (or createGuestProfile) at least once.

So let's do one of two things:

* Change that name to 'sShouldCheckForGuestProfile'. What this flag really does is dictate whether you can trust sGuestProfile (i.e., that if sSCFGP is false and sGuestProfile is null, we're not in Guest Browsing mode.).

* Switch to a real state flag: UNKNOWN -> { GUEST, NORMAL } (maybe even CUSTOM, too). That makes our expectations here very clear, and will be easier for future extension, but I won't block on that. That's also a decent follow-up bug.

Let's also file a follow-up bug to avoid digging for a profile at all -- most of my observations in Comment 0 shouldn't need to fetch the profile again.

Finally, when you've updated the patch, let's push this through Try so we know it doesn't break anything. Flag me for review again and I'll do the push!

Thanks!

::: mobile/android/base/GeckoProfile.java
@@ +368,5 @@
>      }
>  
>      public static GeckoProfile getGuestProfile(Context context) {
>          if (sGuestProfile == null) {
> +            if(sIsGuestProfile) {

Nit: space between 'if' and '('.

@@ +374,5 @@
> +                if (guestDir.exists()) {
> +                    sGuestProfile = get(context, GUEST_PROFILE, guestDir);
> +                    sGuestProfile.mInGuestMode = true;
> +                }
> +                else {

Nit: cuddle } else {.

@@ +381,2 @@
>              }
>          }

Nit: don't kill this blank line.

Comment 10

[Dipti Nirmale [:dnirm]]

Attachment: Changed flag name to 'sShouldCheckForGuestProfile'

Thank you for the review, Richard.

I have corrected the previous patch. Please let me know if I should file follow-up bug 'to avoid digging for a profile'.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8602486 [details] [diff] [review]
Changed flag name to 'sShouldCheckForGuestProfile'

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

This looks good to me; apologies for the delay.

Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea07589c6fc

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/890a96908f5b

Comment 13

[Richard Newman [:rnewman]]

Try run looked good, Dipti, so I landed this. Thanks for the patch and your patience!

If you're looking for another bug to work on:

http://www.joshmatthews.net/bugsahoy/?mobileandroid=1&java=1&unowned=1

mcomella might have some good ideas on the tip of his tongue, too.

Comment 14

[Dipti Nirmale [:dnirm]]

Thank you Richard.
I'd like to work on the follow-up 'to avoid digging for a profile'. Is there any bug filed for this? I checked the list and found Bug 1145192 which sounds similar but am not sure if the expected solution is same.

Comment 15

[Richard Newman [:rnewman]]

Let's save that bug for cutting down on the logging; I'll file a new bug with some details and assign it to you, Dipti.

Comment 16

[Dipti Nirmale [:dnirm]]

Thanks Richard. I'd look into Bug 1169419.
Should I add checkin-needed keyword to this bug?

Comment 17

[Richard Newman [:rnewman]]

Nope, it hit fx-team in Comment 12; it'll be merged to mozilla-central automatically soon.  Thanks!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/890a96908f5b