1315982 - [findbugs] [DE] FennecTabsRepository$FennecTabsRepositorySession.releaseProviders() might ignore java.lang.Exception

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

org.mozilla.gecko.prompts.Prompt.cancelDialog() might ignore java.lang.Exception

"This method might ignore an exception.  In general, exceptions should be handled or reported in some way, or they should be thrown out of the method."

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Another one of this class:

org.mozilla.gecko.sync.repositories.android.FennecTabsRepository$FennecTabsRepositorySession.releaseProviders() might ignore java.lang.Exception

Comment 2

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

[swaroop.rao]

I would like to take this up if nobody's working on it.

I have a build ready to go and I have reviewed the two classes mentioned above. However, at least in Prompt.java, there are several places where there are empty catch() blocks but only one is mentioned in the above comment. Should I make changes to all the empty catch blocks or only for the one mentioned explicitly above?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, that's probably because I only filed bugs for the "high priority" warnings from findbugs - and sometimes the same type of warning has a different priority. But a patch that fixes all catch blocks in Prompt.java would be nice too!

Comment 5

[sangee29495]

Heya!

I observed that this bug has not been assigned to anyone! If no one is working on it currently, can i please take it up? Thanks!

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Sure! Let me know if you need any help.

Comment 7

[Owais Kazi]

Hi sebastian!
If nobody is working on this bug can i work on it as my first bug?

Comment 8

[Owais Kazi]

Can you give me brief explanation of what changes i need to make?

Comment 9

[ajaykumar005097]

Hi sebastian,
i want to work on this issue.If no-one's up for this one , i will work on it.

Comment 10

[ajaykumar005097]

Hi sebastian,
i want to work on this issue.If no-one's up for this one , i will work on it.

Comment 11

[Owais Kazi]

Hi sebastian!
private void cancelDialog() {
        final GeckoBundle ret = new GeckoBundle();
        ret.putInt("button", -1);
        addInputValues(ret);

        notifyClosing(ret);
    }
This function doesn't contain any catch block as the function org.mozilla.gecko.sync.repositories.android.FennecTabsRepository$FennecTabsRepositorySession.releaseProviders() contain empty class. So what need to done with both the classes?

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Prompt.java has been modified recently and doesn't need to catch an exception inside show() anymore:
https://hg.mozilla.org/mozilla-central/diff/10a6565daede/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java

FennecTabsRepositorySession.releaseProviders() still contains empty catch blocks. However it doesn't look like rethrowing or logging makes any sense here. We probably should just suppress the warning. Can you see whether we can do this in code or if the exclude configuration is the only option?

You can run findbugs locally too: ./mach gradle findbugsLocalDebug
However you need to enable the check for this bug in findbugs-exclude.xml - Otherwise you won't see the warning.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Just suppressed a warning in a different bug and will write a quick patch for this here too.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1315982 - findbugs: Ignore DE_MIGHT_IGNORE warning in FennecTabsRepositorySession.releaseProviders().

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

Comment 15

[Julian Chu [:walkingice]]

Comment on attachment 8824130 [details]
Bug 1315982 - findbugs: Ignore DE_MIGHT_IGNORE warning in FennecTabsRepositorySession.releaseProviders().

https://reviewboard.mozilla.org/r/102680/#review104114

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9503aa65892
findbugs: Ignore DE_MIGHT_IGNORE warning in FennecTabsRepositorySession.releaseProviders(). r=walkingice

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/d9503aa65892