Closed Bug 1116668 Opened 5 years ago Closed 4 years ago

getGuestProfile does file access on every call

Categories

(Firefox for Android :: Profile Handling, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: rnewman, Assigned: dnirm, Mentored)

References

(Blocks 3 open bugs)

Details

(Keywords: perf, Whiteboard: [good next bug][lang=java])

Attachments

(1 file, 2 obsolete files)

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)
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.
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
Blocks: 1130550
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.
Assignee: nobody → dnirm.moz
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.
Flags: needinfo?(rnewman)
Attachment #8594271 - Flags: review?(michael.l.comella)
> 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!
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
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.
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.
Attachment #8594271 - Attachment is obsolete: true
Attachment #8594271 - Flags: review?(michael.l.comella)
Attachment #8595142 - Flags: review?(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.
Attachment #8595142 - Flags: review?(rnewman) → feedback+
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'.
Attachment #8602486 - Flags: review?(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
Attachment #8602486 - Flags: review?(rnewman) → review+
Attachment #8595142 - Attachment is obsolete: true
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.
Flags: needinfo?(michael.l.comella)
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.
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.
Flags: needinfo?(michael.l.comella)
Blocks: 1169419
Thanks Richard. I'd look into Bug 1169419.
Should I add checkin-needed keyword to this bug?
Nope, it hit fx-team in Comment 12; it'll be merged to mozilla-central automatically soon.  Thanks!
https://hg.mozilla.org/mozilla-central/rev/890a96908f5b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.