Closed Bug 1262625 Opened 4 years ago Closed 4 years ago

Consider refactoring GeckoProfile lazy initialization for clarity

Categories

(Firefox for Android :: Profile Handling, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(1 file)

Disclaimer: I only briefly looked into this code while investigating bug 1255657.

Currently, GeckoProfile is initialized through `GeckoProfile.forceCreate`, which is called from `GeckoThread.addCustomProfileArg` and `GeckoProfile.getDir`. These are both lazy methods of initialization, called at various times throughout the Activity start-up process under a series of seemingly unrelated conditions. This makes profile initialization hard to reason about and inconsistent (e.g. was it created on the GeckoThread? the UI thread? How did this affect our startup time?).

We need `forceCreate` to be called from Java before Gecko has a chance to start – e.g. we write the profile creation time into times.json and the client ID for telemetry purposes (bug 1255657), meaning the benefits of lazily initialization are dubious.

Can we simplify things and explicitly initialize the GeckoProfile, removing the lazy initialization code?

Concern: how does GeckoProfile follow the Activity lifecycle? Do we lazy init the profile in case the profile is removed from memory and misses the re-initialization code path in onCreate?
jchen has been doing a lot of work with GeckoProfile recently.
Flags: needinfo?(nchen)
I'm not sure there is a single good place to initialize our profile. We want to avoid main thread IO, so we push profile init to either the background thread [1] or GeckoThread [2]. But obviously we can't guarantee that those profile init will happen before the main thread needs a profile, so we are forced to lazy-init the profile on the main thread if needed.

And GeckoProfile doesn't follow the Activity lifecycle, because a GeckoProfile is an application object that can live beyond an Activity (i.e. we may need a GeckoProfile even if there is not an Activity at the moment).

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java?rev=b6683e141c47#1190
[2] mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoThread.java?rev=b6683e141c47#378
Flags: needinfo?(nchen)
If we're looking for optimal performance (which I'm sure we are), I can see the concerns in comment 2.

That being said, I think we can be clearer by using Java convention – get the profile *only* with a getter:

GeckoProfile getProfile() {
  if (mProfile == null) {
    mProfile = fnToGetNewProfile();
  }
  return mProfile;
}

This ensures all attempted accesses to the profile go through the same code path, rather than just hoping we called `forceCreate` before we needed it. This might be the intent with `getDir`, but it's unclear to me (maybe more comments? or use getGeckoProfile* ?) and `forceCreate` is called from elsewhere.

Given how our code works, however, this might look something more like:

class GeckoProfile {
  ProfileState mState = null;

  ProfileState getProfileState() {
    if (mState == null) {
      mState = new ProfileState(...);
    }
    return mState;
  }

  class ProfileState {
    // All the stuff that needs an inited profile
    // ...
  }
}

What about this approach, Jim?
Flags: needinfo?(nchen)
Summary: Consider initing GeckoProfile non-lazily → Consider refactoring GeckoProfile lazy initialization for clarity
(In reply to Michael Comella (:mcomella) from comment #3)
> This ensures all attempted accesses to the profile go through the same code
> path, rather than just hoping we called `forceCreate` before we needed it.
> This might be the intent with `getDir`, but it's unclear to me (maybe more
> comments? or use getGeckoProfile* ?) and `forceCreate` is called from
> elsewhere.

Yeah I think that's more or less the intent of getDir. GekcoProfile makes the assumption (true or not) that, to a consumer of GeckoProfile, the only time a profile must be initialized is when the consumer is accessing the profile dir. (I.e. if you're not accessing the profile dir, it doesn't matter if the profile is "initialized" or not.) So following that assumption, it's reasonable to create the profile when getDir is called.

BTW, forceCreate is only called in two places, getDir and GeckoThread. The GeckoThread call can be replaced by a getDir call, so only getDir really needs to call forceCreate.

> Given how our code works, however, this might look something more like:
> 
> class GeckoProfile {
>   ProfileState mState = null;
> 
>   ProfileState getProfileState() {
>     if (mState == null) {
>       mState = new ProfileState(...);
>     }
>     return mState;
>   }
> 
>   class ProfileState {
>     // All the stuff that needs an inited profile
>     // ...
>   }
> }
> 
> What about this approach, Jim?

I think that's more of a front-end decision. Note that if you replace "ProfileState" with a "File" object representing the profile directory, you're essentially describing what we do right now with getDir/forceCreate.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> (I.e. if you're not accessing the profile dir, it doesn't
> matter if the profile is "initialized" or not.) So following that
> assumption, it's reasonable to create the profile when getDir is called.

It could be true now but may not be true in the future – the ambiguity is what bothers me, particularly as someone new to the code. I think a lazy getter that follows the accepted patterns would be more beneficial.

> BTW, forceCreate is only called in two places, getDir and GeckoThread. The
> GeckoThread call can be replaced by a getDir call, so only getDir really
> needs to call forceCreate.

I think this would be clearer, but it'd be hard to enforce.

> > class GeckoProfile {
> >   ProfileState mState = null;
> > 
> >
> > What about this approach, Jim?
> 
> I think that's more of a front-end decision. Note that if you replace
> "ProfileState" with a "File" object representing the profile directory,
> you're essentially describing what we do right now with getDir/forceCreate.

I don't think `ProfileState` is particularly clear either, but it could be good to make it more obvious this is a lazy initialization.

I'll do the simple thing and add some comments here.
Assignee: nobody → michael.l.comella
Attachment #8741158 - Flags: review?(nchen) → review+
Comment on attachment 8741158 [details]
MozReview Request: Bug 1262625 - Only call getDir to initialize profile. r=jchen

https://reviewboard.mozilla.org/r/46233/#review42797
https://hg.mozilla.org/mozilla-central/rev/6b0f8e84b00c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.