1316021 - [findbugs] [ST] Write to static field from instance method

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

* Write to static field org.mozilla.gecko.GeckoApplication.instance from instance method new org.mozilla.gecko.GeckoApplication()

* Write to static field org.mozilla.gecko.media.MediaControlService.mTabReference from instance method org.mozilla.gecko.media.MediaControlService.onTabChanged(Tab, Tabs$TabEvents, String)

"This instance method writes to a static field. This is tricky to get correct if multiple instances are being manipulated, and generally bad practice."

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

To start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Then, you'll need to upload a patch - see:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and other helpful folks in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Comment 2

[Cosm]

I want to work on this bug.

Comment 3

[Shan[:Shan11812]]

Hi Sebastian, I'm kinda new to Open source community. I want to work on this bug. I've setup artifact mode of firefox. Could you give more info about the bug.

TIA!

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, this one is a little bit tricky.

GeckoApplication: After a quick look at the code I think we don't need the static reference to the instance at all. All callers should have some kind of access to a Context and could just cast the return value of getApplication() to GeckoApplication and then perform whatever call they need.

MediaControlService: This one is a little bit more complicated. I'm not sure anymore whether mTabReference actually needs to be static. We might want to keep it even if the Service is destroyed and re-created. However between service restarts the process can be killed too and then this would be lost anyways. @Alastor: What was the backstory here? Does mTabReference need to be static?

Comment 5

[Alastor Wu [:alwu]]

I think mTabReference might not need to be static. The service would only be destroyed when the media is not playing because of startForeground(). When the service was destroyed, it would also remove the media notification, so we don't need to care whether mTabReference exists or not.

Comment 6

[Shan[:Shan11812]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> Okay, this one is a little bit tricky.
> 
> GeckoApplication: After a quick look at the code I think we don't need the
> static reference to the instance at all. All callers should have some kind
> of access to a Context and could just cast the return value of
> getApplication() to GeckoApplication and then perform whatever call they
> need.

GeckoApp has a static method getTempDirectory() which depends on GeckoApplication static instance.
getTempDirectory() is used in GeckoActoinProvider, still looking at any other way to call it.

> MediaControlService: This one is a little bit more complicated. I'm not sure
> anymore whether mTabReference actually needs to be static. We might want to
> keep it even if the Service is destroyed and re-created. However between
> service restarts the process can be killed too and then this would be lost
> anyways. @Alastor: What was the backstory here? Does mTabReference need to
> be static?

As Alastor confirmed, there is no need for mTabReference to be static.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Shan from comment #6)
> (In reply to Sebastian Kaspari (:sebastian) from comment #4)
> > Okay, this one is a little bit tricky.
> > 
> > GeckoApplication: After a quick look at the code I think we don't need the
> > static reference to the instance at all. All callers should have some kind
> > of access to a Context and could just cast the return value of
> > getApplication() to GeckoApplication and then perform whatever call they
> > need.
> 
> GeckoApp has a static method getTempDirectory() which depends on
> GeckoApplication static instance.
> getTempDirectory() is used in GeckoActoinProvider, still looking at any
> other way to call it.

I think we could add a Context parameter to getTempDirectory(). It's called in two places:

* GeckoApp.deleteTempFiles(): Could have a Context parameter too. It's called from BrowserApp (is a Context) and GeckoActionProvider (has mContext).

* GeckoActionProvider (has mContext).

Comment 8

[Shan[:Shan11812]]

Attachment: Bug 1316021 - Remove unnecessary static fields;

Review commit: https://reviewboard.mozilla.org/r/95740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95740/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8814534 [details]
Bug 1316021 - Remove unnecessary static fields;

https://reviewboard.mozilla.org/r/95740/#review95956

Nice refactoring! I added some comments below.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:770
(Diff revision 1)
>                  final HealthRecorder rec = mHealthRecorder;
>                  if (rec != null) {
>                    rec.recordGeckoStartupTime(mGeckoReadyStartupTimer.getElapsed());
>                  }
>  
> -                GeckoApplication.get().onDelayedStartup();
> +                ((GeckoApplication)getApplicationContext()).onDelayedStartup();

nit: space before getApplicationContext():

> ((GeckoApplication) getApplicationContext())...

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2294
(Diff revision 1)
>          Toast.makeText(this, message, Toast.LENGTH_LONG).show();
>      }
>  
>      // Get a temporary directory, may return null
> -    public static File getTempDirectory() {
> -        File dir = GeckoApplication.get().getExternalFilesDir("temp");
> +    public static File getTempDirectory(Context context) {
> +        GeckoApplication app = (GeckoApplication)context;

There's no guarantee that the passed in Context is a GeckoApplication. So we should either make the parameter a GeckoApplication or inside the method call: context.getApplicationContext() and then cast this to GeckoApplication.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2295
(Diff revision 1)
> -        return dir;
> +        if (app != null)
> +            app.getExternalFilesDir("temp");
> +        return null;

Why did you add the null check here? Did this crash while testing? I'd rather prefer we annotate the parameter with @NonNull.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

I pushed this version of the patch to try in order to see if this fails any tests so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5de2e42ef7a5

Comment 11

[Shan[:Shan11812]]

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> > ((GeckoApplication) getApplicationContext())...
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2294
> (Diff revision 1)
> >          Toast.makeText(this, message, Toast.LENGTH_LONG).show();
> >      }
> >  
> >      // Get a temporary directory, may return null
> > -    public static File getTempDirectory() {
> > -        File dir = GeckoApplication.get().getExternalFilesDir("temp");
> > +    public static File getTempDirectory(Context context) {
> > +        GeckoApplication app = (GeckoApplication)context;
> 
> There's no guarantee that the passed in Context is a GeckoApplication. So we
> should either make the parameter a GeckoApplication or inside the method
> call: context.getApplicationContext() and then cast this to GeckoApplication.
> 

Thats the reason for adding the below null check. Getting ApplicationContext from given context seems to solve that issue.

> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2295
> (Diff revision 1)
> > -        return dir;
> > +        if (app != null)
> > +            app.getExternalFilesDir("temp");
> > +        return null;
> 
> Why did you add the null check here? Did this crash while testing? I'd
> rather prefer we annotate the parameter with @NonNull.

Comment 12

[Shan[:Shan11812]]

Comment on attachment 8814534 [details]
Bug 1316021 - Remove unnecessary static fields;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95740/diff/1-2/

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8814534 [details]
Bug 1316021 - Remove unnecessary static fields;

https://reviewboard.mozilla.org/r/95740/#review96412

This part is looking good!

Are you going to add a second commit for MediaControlService?

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Please mark the open issues in reviewboard as fixed after updating the code. This will make reviewing and landing patches easier. :)

Comment 15

[Shan[:Shan11812]]

I've stripped the commit with keep flag to make changes. I've no idea why it changes MediaControlService code. I'll push one with all changes.

I had no idea we can make changes on reviewboard also!

Comment 16

[Shan[:Shan11812]]

Comment on attachment 8814534 [details]
Bug 1316021 - Remove unnecessary static fields;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95740/diff/2-3/

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8814534 [details]
Bug 1316021 - Remove unnecessary static fields;

https://reviewboard.mozilla.org/r/95740/#review99730

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8cddd5f743e

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a74fd7f6dddecd85da653dd1fab9e546fecb2e0
Bug 1316021 - Remove unnecessary static fields; r=sebastian

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a74fd7f6ddd