1316009 - [findbugs] [MS] non-final

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

* 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

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 3

[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 4

[Tomislav Jurin]

I would like to try if you are willing to guide me. Thanks

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Sure! Did you already setup a build environment? See comment 3 for helpful links.

Comment 6

[Tomislav Jurin]

I will do it right now, can you assign this to me please ? Thank you

Comment 7

[Tomislav Jurin]

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

Comment 8

[Tomislav Jurin]

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.

Comment 9

[Tomislav Jurin]

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

Comment 10

[Tomislav Jurin]

Everything is setup to start so I am awating for further instructions.

Comment 11

[Tomislav Jurin]

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

Comment 12

[Richard Newman [:rnewman]]

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 13

[Tomislav Jurin]

Attachment: Bug 1316009 - Adding 'final' keyword to static fields where appropriate.

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

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

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() {
>  ...
> }

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 16

[Richard Newman [:rnewman]]

(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.

Comment 17

[Richard Newman [:rnewman]]

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.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 19

[Tomislav Jurin]

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.

Comment 20

[Richard Newman [:rnewman]]

> 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…

Comment 21

[Tomislav Jurin]

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

Thank you.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

(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 23

[Tomislav Jurin]

Attachment: Bug 1316009 - adding finals where needed

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

Comment 24

[Tomislav Jurin]

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

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

Comment 25

[Tomislav Jurin]

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

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

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)?

Comment 27

[Tomislav Jurin]

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 ?

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

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. :)

Comment 29

[Tomislav Jurin]

I did hg update --clean central and just made changes again, hope I did it right this time.

Comment 30

[Tomislav Jurin]

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

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

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 32

[Tomislav Jurin]

(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

Comment 33

[Tomislav Jurin]

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 34

[Tomislav Jurin]

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

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

Comment 35

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 36

[Tomislav Jurin]

should I remove final keyword for boolean DEBUG?

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Yes, please. :)

Comment 38

[Tomislav Jurin]

I think this should be it, I got bit dangled up in hg, but I think I managed to solve it :)

Comment 39

[Tomislav Jurin]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94182/diff/4-5/

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

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. :)

Comment 41

[Sebastian Kaspari (:sebastian; :pocmo)]

Pushed it to try again to make sure we didn't miss anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b1f5aecf9d4

Comment 42

[Tomislav Jurin]

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

Any example what I should write ?

Thanks

Comment 43

[Sebastian Kaspari (:sebastian; :pocmo)]

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 44

[Tomislav Jurin]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94182/diff/5-6/

Comment 45

[Tomislav Jurin]

I think I managed to do it. I will become mercurial expert at this rate :D

Comment 46

[Sebastian Kaspari (:sebastian; :pocmo)]

(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. :)

Comment 47

[Sebastian Kaspari (:sebastian; :pocmo)]

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

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

Comment 48

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e51bad5f8bdf26b6e0d09f55e49206ad9e11377
Bug 1316009 - Adding 'final' keyword to static fields where appropriate. r=sebastian

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e51bad5f8bd