Closed
Bug 851373
Opened 12 years ago
Closed 11 years ago
Eliminate GeckoApp/Tabs lifecycle issues
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 25
People
(Reporter: rnewman, Assigned: bnicholson)
References
Details
Attachments
(1 file)
(deleted),
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Reporter | ||
Comment 2•11 years ago
|
||
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.
Attachment #773611 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 3•11 years ago
|
||
(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.
Reporter | ||
Comment 4•11 years ago
|
||
Sounds good!
Assignee | ||
Comment 5•11 years ago
|
||
Comment 6•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
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
•