[findbugs] [MS] non-final

RESOLVED FIXED in Firefox 53

Status

()

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

People

(Reporter: sebastian, Assigned: Tomislav Jurin, 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, 1 obsolete attachment)

* org.mozilla.gecko.AboutPages.DEFAULT_ICON_PAGES is a mutable array

* org.mozilla.gecko.background.common.GlobalConstants.DEFAULT_CIPHER_SUITES is a mutable array

* org.mozilla.gecko.background.common.GlobalConstants.DEFAULT_PROTOCOLS is a mutable array

* org.mozilla.gecko.background.common.log.Logger.LOG_PERSONAL_INFORMATION isn't final but should be

* org.mozilla.gecko.db.LocalBrowserDB.HISTORY_VISITS_COUNT isn't final but should be

* org.mozilla.gecko.db.LocalBrowserDB.HISTORY_VISITS_DATE isn't final but should be
	
* org.mozilla.gecko.db.LocalBrowserDB.HISTORY_VISITS_URL isn't final but should be

* org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.LICENSE_REQUEST_INITIAL isn't final but should be

* org.mozilla.gecko.media.MediaDrmProxy.mProxyList isn't final but should be
	
* org.mozilla.gecko.preferences.GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED isn't final but should be

* org.mozilla.gecko.sync.crypto.HKDF.HMAC_ALGORITHM isn't final but should be

* org.mozilla.gecko.sync.net.SyncStorageRequest.SERVER_ERROR_MESSAGES isn't final but should be
	
* org.mozilla.gecko.sync.net.SyncStorageResponse.SERVER_ERROR_MESSAGES isn't final but should be

* org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositoryDataAccessor.LOG_TAG isn't final but should be
	
* org.mozilla.gecko.sync.repositories.android.BookmarksInsertionManager.DEBUG isn't final but should be

* org.mozilla.gecko.sync.repositories.domain.PasswordRecord.PASSWORDS_TTL isn't final but should be

* org.mozilla.gecko.sync.ThreadPool.executorService isn't final but should be

* org.mozilla.mozstumbler.service.AppGlobals.appName isn't final but should be
	
* org.mozilla.mozstumbler.service.AppGlobals.appVersionName isn't final but should be
	
* org.mozilla.mozstumbler.service.stumblerthread.scanners.cellscanner.CellScannerImplementation.LOG_TAG isn't final but should be
"This static field public but not final, and could be changed by malicious code or by accident from another package. The field could be made final to avoid this vulnerability."
"A final static field references an array and can be accessed by malicious code or by accident from another package. This code can freely modify the contents of the array."
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
(Assignee)

Comment 4

8 months ago
I would like to try if you are willing to guide me. Thanks
Sure! Did you already setup a build environment? See comment 3 for helpful links.
(Assignee)

Comment 6

8 months ago
I will do it right now, can you assign this to me please ? Thank you
(Assignee)

Comment 7

8 months ago
on step wheere I have to clone repo with mercurial I get SSL error:

SSLError: [SSL: DECRYPTION_FAILED_OR_BAD_RECORD_MAC] decryption failed or bad record mac (_ssl.c:1754)
abort: error: DECRYPTION_FAILED_OR_BAD_RECORD_MAC

tried to google but without luck
(Assignee)

Comment 8

7 months ago
I have managed to solve upper problem by downloading bundle instead of using "hg clone". Now, after using hg unbundle and updating it, I try to build what I have. I go cd into mozzila-central directory and there try to execute ./mach but get following error:

bash: ./mach: No such file or directory

From here on I have no idea what to do. I have downloaded mozzila-central gzip(v1) from this site https://hg.cdn.mozilla.net/.

Thank in advance for help.
(Assignee)

Comment 9

7 months ago
Above errors are all fixed. I have again unbundled the package and that made the trick.

Now I fail on ./mach build

https://pastebin.mozilla.org/8928906
(Assignee)

Comment 10

7 months ago
Everything is setup to start so I am awating for further instructions.
(Assignee)

Comment 11

7 months ago
Hi Sebastian, I have made changes in all files except following:


org.mozilla.gecko.sync.repositories.android.BookmarksInsertionManager.DEBUG isn't final but should be

When I declare DEBUG to be final I get following error:

 Error:(57, 29) Gradle: error: cannot assign a value to final 
                variable DEBUG
That variable *is* assigned to, in tests:

http://searchfox.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/android/test/TestBookmarksInsertionManager.java#58


So in this case FindBugs is presumably being run with insufficient information. Don't make it final.
Comment hidden (mozreview-request)
Assignee: nobody → svezauzeto12
Status: NEW → ASSIGNED
(Reporter)

Comment 14

7 months ago
mozreview-review
Comment on attachment 8812417 [details]
Bug 1316009 - Adding 'final' keyword to static fields where appropriate.

https://reviewboard.mozilla.org/r/94182/#review94584

The places where you added 'final' look good. However we shouldn't remove it in some other places. See comments below.

::: mobile/android/base/java/org/mozilla/gecko/AboutPages.java:75
(Diff revision 1)
>      public static boolean isAboutPage(String page, String url) {
>          return url != null && url.toLowerCase().startsWith(page);
>  
>      }
>  
> -    public static final String[] DEFAULT_ICON_PAGES = new String[] {
> +    public static  String[] DEFAULT_ICON_PAGES = new String[] {

I don't think removing the final keyword is what we want here. The thing findbugs is warning about is that even though this array is final it can still be modified. In this case someone could for example call DEFAULT_ICON_PAGES[0] = "Hello World";

In this case here I guess it would be pretty straight forward to turn this into an unmodifiable list:

> public static final List<String> DEFAULT_ICON_PAGES = Collections.unmodifiableList(Arrays.asList(
>    ...
> ));

::: mobile/android/services/src/main/java/org/mozilla/gecko/background/common/GlobalConstants.java:41
(Diff revision 1)
> -  public static final String[] DEFAULT_CIPHER_SUITES;
> -  public static final String[] DEFAULT_PROTOCOLS;
> +  public static  String[] DEFAULT_CIPHER_SUITES;
> +  public static  String[] DEFAULT_PROTOCOLS;

Same here: Removing the final keyword is not really helpful. However in this case all callers require an array, so turning this into a list is a bit annoying. 

Instead this could be turned into two methods (there's already runtime code anyways):

> public static String[] getDefaultCipherSuites() {
>  ...
> }
Attachment #8812417 - Flags: review?(s.kaspari)
I'll see whether I can run findbugs in a way that it knows about BookmarksInsertionManager.DEBUG - or at least add it to the filter list.
(In reply to Sebastian Kaspari (:sebastian) from comment #14)

> Instead this could be turned into two methods (there's already runtime code
> anyways):
> 
> > public static String[] getDefaultCipherSuites() {
> >  ...
> > }

That's kinda worse, though -- either you pass a reference to the array, in which case the callee can mutate it and not realize, exactly as if it directly accessed the final array, or you have to copy the array each time we establish a TLS connection!

Neither of these is good. The right answer here is to muffle the warning, add annotations to cause errors if someone tries to mutate the array, etc. etc.
In general, I think FindBugs is too alarmist — take its input under advisement, but don't warp the structure of code around it. CAPITALIZED_FINAL_ARRAYS that are actually used as arrays are one of those things where it's good to audit uses, good to consider annotations, and silly to change the code.
Mentor: rnewman@mozilla.com
(In reply to Richard Newman [:rnewman] from comment #16)
> That's kinda worse, though -- either you pass a reference to the array, in
> which case the callee can mutate it and not realize, exactly as if it
> directly accessed the final array, or you have to copy the array each time
> we establish a TLS connection!

Yeah, I was thinking about building the array inside the method. Sharing the instance seemed like a (micro?) optimization not worth keeping at all costs.

> Neither of these is good. The right answer here is to muffle the warning,
> add annotations to cause errors if someone tries to mutate the array, etc.
> etc.

Suppressing the warning is definitely also an option. I haven't seen any annotation for "this should be immutable" yet - do you? Feels like it should be easy to write a (lint) check for that though.

(In reply to Richard Newman [:rnewman] from comment #17)
> In general, I think FindBugs is too alarmist — take its input under
> advisement, but don't warp the structure of code around it.

Yeah, that's true - especially for legacy code.
(Assignee)

Comment 19

7 months ago
I would still like to fix this even tho I failed miserably first time. Since I need little more guidance maybe you could write me what is best solution to this problem and then I can try again ?

Thanks in advance.
> Suppressing the warning is definitely also an option. I haven't seen any
> annotation for "this should be immutable" yet - do you? Feels like it should
> be easy to write a (lint) check for that though.

I haven't seen one, and I can't find one, but yeah, it seems like it should be easy enough…

The other common fix for this bug is to make a public/protected array private.

DEFAULT_ICON_PAGES is easy to fix for that, at least. GlobalConstants less so…
(Assignee)

Comment 21

7 months ago
I will make new commit adding only finals where needed. Please update me what to do with others that should be mutable.

Thank you.
(In reply to svezauzeto12 from comment #21)
> I will make new commit adding only finals where needed. Please update me
> what to do with others that should be mutable.

Thanks! Let's move the mutable array warnings to a new bug. We are probably going to add those to the filter list only. But the findbugs configuration hasn't landed in mozilla-central yet.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

7 months ago
Couldn't find  org.mozilla.gecko.media.MediaDrmProxy.mProxyList isn't final but should be this time since I have made hg clone again. Looks like someone renamed it so I didn't touch it.

Also please review only latest commit to, I have accidentally made 2 commits, and latest is the correct one.

Thank you
Attachment #8812417 - Attachment is obsolete: true
Attachment #8812417 - Flags: review?(s.kaspari)
Comment on attachment 8813343 [details]
Bug 1316009 - adding finals where needed

I can't really review this: There are both commits on mozreview and the patches still contain the old changes. Can you look into pushing just a single commit with you latest changes (without removing the 'final' keyword)?
Attachment #8813343 - Flags: review?(s.kaspari)
(Assignee)

Comment 27

7 months ago
Sure , how can I rollback to version I started with ( delete all changes I made ) and then start clean again to make new clean commit ?
Let's see. Assuming you are using mercurial..

If you have run bootstrap and are using our mercurial tools then this should reset the state to the one of mozilla-central:
> hg update --clean central

Otherwise I would open the log and look for the last commit before the changes and reset to this changeset:
> hg log -f
> hg update --clean CHANGESET-ID-FROM-LOG

Or remove commits from the top until you are back at the state you want:
> hg strip .

Or without losing what you have already done: You can use the histedit extension (See online docs) to squash the two commits to one. Then modify and "commit --amend" until the patch is in the shape you want it.

There are probably plenty of other ways to do this. :)
(Assignee)

Comment 29

7 months ago
I did hg update --clean central and just made changes again, hope I did it right this time.
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8813343 - Attachment is obsolete: true
(Reporter)

Comment 31

7 months ago
mozreview-review
Comment on attachment 8812417 [details]
Bug 1316009 - Adding 'final' keyword to static fields where appropriate.

https://reviewboard.mozilla.org/r/94182/#review95502

Thank you for the patch! This is looking pretty good already.

I just ran findbugs locally and the only warning missing (seems to be renamed since the last time) is:

> Bug type MS_SHOULD_BE_FINAL
> In class org.mozilla.gecko.media.MediaDrmProxy
> Field org.mozilla.gecko.media.MediaDrmProxy.sProxyList
> At MediaDrmProxy.java:[line 42]

Let's take care of this one and the two 'finals' mentioned below and I think we are ready to go. I'll push this patch to try already - in order to see whether we missed something (e.g. a test case modifying a static non-final value).

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:58
(Diff revision 3)
> -    public static int LICENSE_REQUEST_INITIAL = 0; /*MediaKeyMessageType::License_request*/
> +    public static final int LICENSE_REQUEST_INITIAL = 0; /*MediaKeyMessageType::License_request*/
>      public static int LICENSE_REQUEST_RENEWAL = 1; /*MediaKeyMessageType::License_renewal*/
>      public static int LICENSE_REQUEST_RELEASE = 2; /*MediaKeyMessageType::License_release*/

Even though findbugs only thinks the first one is a high priority issue: Let's make all of the LICENSE_REQUEST_* constants final.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/AppGlobals.java:33
(Diff revision 3)
>      /* These are set on startup. The appVersionName and code are not used in the service-only case. */
> -    public static String appVersionName = "0.0.0";
> +    public static final String appVersionName = "0.0.0";
>      public static int appVersionCode = 0;
> -    public static String appName = "StumblerService";
> +    public static final String appName = "StumblerService";

Looks like appVersionCode could be final too. Actually they do not seem to be used in the service-only case (should be true for fennec) according to the comment. But let's keep them for now.
Attachment #8812417 - Flags: review?(s.kaspari)
(Assignee)

Comment 32

7 months ago
(In reply to Sebastian Kaspari (:sebastian) from comment #31)
> Comment on attachment 8812417 [details]
> Bug 1316009 - adding finals where needed
> 
> https://reviewboard.mozilla.org/r/94182/#review95502
> 
> Thank you for the patch! This is looking pretty good already.
> 
> I just ran findbugs locally and the only warning missing (seems to be
> renamed since the last time) is:
> 
> > Bug type MS_SHOULD_BE_FINAL
> > In class org.mozilla.gecko.media.MediaDrmProxy
> > Field org.mozilla.gecko.media.MediaDrmProxy.sProxyList
> > At MediaDrmProxy.java:[line 42]
> 
> Let's take care of this one and the two 'finals' mentioned below and I think
> we are ready to go. I'll push this patch to try already - in order to see
> whether we missed something (e.g. a test case modifying a static non-final
> value).
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:
> 58
> (Diff revision 3)
> > -    public static int LICENSE_REQUEST_INITIAL = 0; /*MediaKeyMessageType::License_request*/
> > +    public static final int LICENSE_REQUEST_INITIAL = 0; /*MediaKeyMessageType::License_request*/
> >      public static int LICENSE_REQUEST_RENEWAL = 1; /*MediaKeyMessageType::License_renewal*/
> >      public static int LICENSE_REQUEST_RELEASE = 2; /*MediaKeyMessageType::License_release*/
> 
> Even though findbugs only thinks the first one is a high priority issue:
> Let's make all of the LICENSE_REQUEST_* constants final.
> 
> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/AppGlobals.java:
> 33
> (Diff revision 3)
> >      /* These are set on startup. The appVersionName and code are not used in the service-only case. */
> > -    public static String appVersionName = "0.0.0";
> > +    public static final String appVersionName = "0.0.0";
> >      public static int appVersionCode = 0;
> > -    public static String appName = "StumblerService";
> > +    public static final String appName = "StumblerService";
> 
> Looks like appVersionCode could be final too. Actually they do not seem to
> be used in the service-only case (should be true for fennec) according to
> the comment. But let's keep them for now.

Yeah I noticed someone renamed that sProxyList, it used to be mProxyList. But since this is large project and I didn't know what else is changed so I decided to leave it for now.

I will fix this now and push it. Do you want me to make now commit only with these changes ( mentioned in your post ) , or should I combine 2 commits together so we have everything in one commit ?

Thank you
(Assignee)

Comment 33

7 months ago
Okay I think I have managed to squash 2 commits ( this mercurial is killing me :-) ). I will push it now and then you can review it again.

Cheers
Comment hidden (mozreview-request)
Summary: [findbugs] [MS] mutable / non-final arrays → [findbugs] [MS] non-final arrays
See Also: → bug 1320298
(Reporter)

Comment 35

7 months ago
mozreview-review
Comment on attachment 8812417 [details]
Bug 1316009 - Adding 'final' keyword to static fields where appropriate.

https://reviewboard.mozilla.org/r/94182/#review95688

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksInsertionManager.java:43
(Diff revision 4)
>   * Note that this class is not thread safe. This should be fine: call it only
>   * from within a store runnable.
>   */
>  public class BookmarksInsertionManager {
>    public static final String LOG_TAG = "BookmarkInsert";
> -  public static boolean DEBUG = false;
> +  public static final boolean DEBUG = false;

Let's ignore this one. It's written from TestBookmarksInsertionManager and this test will fail with this change.

I filed bug 1320300 to investigate why findbugs is not looking at the test code.
Attachment #8812417 - Flags: review?(s.kaspari)
Summary: [findbugs] [MS] non-final arrays → [findbugs] [MS] non-final
(Assignee)

Comment 36

7 months ago
should I remove final keyword for boolean DEBUG?
(In reply to svezauzeto12 from comment #36)
> should I remove final keyword for boolean DEBUG?

Yes, please. :)
(Assignee)

Comment 38

7 months ago
I think this should be it, I got bit dangled up in hg, but I think I managed to solve it :)
Comment hidden (mozreview-request)
(Reporter)

Comment 40

7 months ago
mozreview-review
Comment on attachment 8812417 [details]
Bug 1316009 - Adding 'final' keyword to static fields where appropriate.

https://reviewboard.mozilla.org/r/94182/#review95942

This is looking good now.

But can you update the commit message? It contains a lot of text from all the previous commits and "removed final from boolean DEBUG" is not really true because it never existed in the first place - you added and removed it. :)
Attachment #8812417 - Flags: review?(s.kaspari)
Pushed it to try again to make sure we didn't miss anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b1f5aecf9d4
(Assignee)

Comment 42

7 months ago
yeah sure, I will make new commit ( same diff ) with new commit message . 

Any example what I should write ?

Thanks
I think your initial commit message (without the additional lines right now) is sufficient. However I'd probably write:

> Bug 1316009 - Adding 'final' keyword to static fields where appropriate. r?sebastian
Comment hidden (mozreview-request)
(Assignee)

Comment 45

7 months ago
I think I managed to do it. I will become mercurial expert at this rate :D
(In reply to svezauzeto12 from comment #45)
> I think I managed to do it. I will become mercurial expert at this rate :D

Heh. Yeah. :)
(Reporter)

Comment 47

7 months ago
mozreview-review
Comment on attachment 8812417 [details]
Bug 1316009 - Adding 'final' keyword to static fields where appropriate.

https://reviewboard.mozilla.org/r/94182/#review96040
Attachment #8812417 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e51bad5f8bdf26b6e0d09f55e49206ad9e11377
Bug 1316009 - Adding 'final' keyword to static fields where appropriate. r=sebastian

Comment 49

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e51bad5f8bd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.