Closed
Bug 1116668
Opened 10 years ago
Closed 10 years ago
getGuestProfile does file access on every call
Categories
(Firefox for Android Graveyard :: Profile Handling, defect)
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: rnewman, Assigned: dnirm, Mentored)
References
Details
(Keywords: perf, Whiteboard: [good next bug][lang=java])
Attachments
(1 file, 2 obsolete files)
2.89 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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)
Whiteboard: [good second bug][lang=java] → [good next bug][lang=java]
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 years ago
|
||
> 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 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
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)
Reporter | ||
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
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)
Reporter | ||
Comment 11•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Attachment #8595142 -
Attachment is obsolete: true
Reporter | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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.
Reporter | ||
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
Thanks Richard. I'd look into Bug 1169419.
Should I add checkin-needed keyword to this bug?
Reporter | ||
Comment 17•10 years ago
|
||
Nope, it hit fx-team in Comment 12; it'll be merged to mozilla-central automatically soon. Thanks!
Comment 18•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•