851373 - Eliminate GeckoApp/Tabs lifecycle issues

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

In Bug 844407 we determined that there are some ownership/lifecycle assumptions here. Our current code pretty much works by accident.

We should untangle this, eliminate the Tabs singleton (and others), and clarify lifecycle assumptions. This will improve thread-safety, improve testability, improve the compartmentalization (and maintainability) of the code, and generally stop me sobbing into my tea.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Have Tabs use the application context instead of the activity

I don't think there's any reason the tabs needs to be attached to Activity specifically; it appears to need only a Context. Tabs stays around as long as Gecko is around, and that matches the lifetime of the application -- not an Activity. By using the application context (which is, as expected, alive for the duration of the application), we can prevent leaking the Activity reference and get rid of these lifecycle issues.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 773611 [details] [diff] [review]
Have Tabs use the application context instead of the activity

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

r+ with some questions or extensions to consider.

::: mobile/android/base/Tab.java
@@ +74,4 @@
>      public static final int STATE_ERROR = 3;
>  
>      public Tab(Context context, int id, String url, boolean external, int parentId, String title) {
> +        mAppContext = context.getApplicationContext();

Same point as elsewhere.

::: mobile/android/base/Tabs.java
@@ +98,5 @@
>          registerEventListener("DesktopMode:Changed");
>      }
>  
> +    public synchronized void attachToContext(Context context) {
> +        final Context appContext = context.getApplicationContext();

Why not take the application context as the argument?

@@ +104,5 @@
>              return;
>          }
>  
> +        mAppContext = appContext;
> +        mAccountManager = AccountManager.get(appContext);

It's not really clear to me whether doing *anything* is the right call here if the application context has changed! Certainly we should log at WARN.

@@ +335,5 @@
>       * @return the current GeckoApp instance, or throws if
>       *         we aren't correctly initialized.
>       */
> +    private synchronized Context getAppContext() {
> +        if (mAppContext == null) {

One way to avoid this entirely is to have a Tabs instance have the app context as a final member, and have the initial getInstance call take a context argument. That way you know that if you have a Tabs instance, it's got a context.

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #2)
> Comment on attachment 773611 [details] [diff] [review]
> Have Tabs use the application context instead of the activity
> 
> Review of attachment 773611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Tabs.java
> @@ +98,5 @@
> >          registerEventListener("DesktopMode:Changed");
> >      }
> >  
> > +    public synchronized void attachToContext(Context context) {
> > +        final Context appContext = context.getApplicationContext();
> 
> Why not take the application context as the argument?

Why rely on the caller to provide the right context when we can ensure that we're using the one we want here? This is a technique used frequently in Android (some examples: http://androidxref.com/4.1.2/search?q=context.getapplicationcontext&defs=&refs=&path=&hist=&project=packages).

> @@ +104,5 @@
> >              return;
> >          }
> >  
> > +        mAppContext = appContext;
> > +        mAccountManager = AccountManager.get(appContext);
> 
> It's not really clear to me whether doing *anything* is the right call here
> if the application context has changed! Certainly we should log at WARN.

True -- I'm almost inclined to do a Log.wtf() here since I can think of no circumstances where this could ever actually happen.

> 
> @@ +335,5 @@
> >       * @return the current GeckoApp instance, or throws if
> >       *         we aren't correctly initialized.
> >       */
> > +    private synchronized Context getAppContext() {
> > +        if (mAppContext == null) {
> 
> One way to avoid this entirely is to have a Tabs instance have the app
> context as a final member, and have the initial getInstance call take a
> context argument. That way you know that if you have a Tabs instance, it's
> got a context.

Yeah, I was going to get these issues sorted out as part of bug 889100 which kills the Tabs singleton.

Comment 4

[Richard Newman [:rnewman]]

Sounds good!

Comment 5

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eae8efb94b6f

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eae8efb94b6f