Closed Bug 1315982 Opened 4 years ago Closed 4 years ago

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

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: sebastian, Assigned: sebastian, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

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."
Priority: -- → P3
Another one of this class:

org.mozilla.gecko.sync.repositories.android.FennecTabsRepository$FennecTabsRepositorySession.releaseProviders() might ignore java.lang.Exception
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
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?
Flags: needinfo?(s.kaspari)
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!
Flags: needinfo?(s.kaspari)
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!
Sure! Let me know if you need any help.
Hi sebastian!
If nobody is working on this bug can i work on it as my first bug?
Can you give me brief explanation of what changes i need to make?
Hi sebastian,
i want to work on this issue.If no-one's up for this one , i will work on it.
Hi sebastian,
i want to work on this issue.If no-one's up for this one , i will work on it.
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?
Flags: needinfo?(s.kaspari)
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.
Flags: needinfo?(s.kaspari)
Summary: [findbugs] [DE] org.mozilla.gecko.prompts.Prompt.cancelDialog() might ignore java.lang.Exception → [findbugs] [DE] FennecTabsRepository$FennecTabsRepositorySession.releaseProviders() might ignore java.lang.Exception
Just suppressed a warning in a different bug and will write a quick patch for this here too.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Comment on attachment 8824130 [details]
Bug 1315982 - findbugs: Ignore DE_MIGHT_IGNORE warning in FennecTabsRepositorySession.releaseProviders().

https://reviewboard.mozilla.org/r/102680/#review104114
Attachment #8824130 - Flags: review?(walkingice0204) → review+
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d9503aa65892
findbugs: Ignore DE_MIGHT_IGNORE warning in FennecTabsRepositorySession.releaseProviders(). r=walkingice
https://hg.mozilla.org/mozilla-central/rev/d9503aa65892
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.