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

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: sebastian, Assigned: Shan11812, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [lang=java][good first bug])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

* 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."
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

8 months ago
I want to work on this bug.
(Assignee)

Comment 3

7 months ago
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!
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?
Flags: needinfo?(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.
Flags: needinfo?(alwu)
(Assignee)

Comment 6

7 months ago
(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.
(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 hidden (mozreview-request)
Assignee: nobody → 11812r
Status: NEW → ASSIGNED
(Reporter)

Comment 9

7 months ago
mozreview-review
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.
Attachment #8814534 - Flags: review?(s.kaspari)
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
(Assignee)

Comment 11

7 months ago
(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 hidden (mozreview-request)
(Reporter)

Comment 13

7 months ago
mozreview-review
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?
Attachment #8814534 - Flags: review?(s.kaspari) → review+
Please mark the open issues in reviewboard as fixed after updating the code. This will make reviewing and landing patches easier. :)
(Assignee)

Comment 15

7 months ago
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 hidden (mozreview-request)
(Reporter)

Comment 17

6 months ago
mozreview-review
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a74fd7f6dddecd85da653dd1fab9e546fecb2e0
Bug 1316021 - Remove unnecessary static fields; r=sebastian

Comment 19

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a74fd7f6ddd
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.