Closed Bug 1116633 Opened 5 years ago Closed 5 years ago

Unclosed BufferedReader in GeckoAppShell.getPluginDirectories

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: rnewman, Assigned: san13692, Mentored)

Details

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

Attachments

(1 file, 1 obsolete file)

if (vfile.canRead()) {
                    vreader = new FileReader(vfile);
                    String version = new BufferedReader(vreader).readLine();
                    if (version.indexOf("CM9") != -1 ||
                        version.indexOf("cyanogen") != -1 ||
                        version.indexOf("Nova") != -1)
                    {
                        Log.w(LOGTAG, "Blocking plugins because of Tegra 2 + unofficial ICS bug (bug 736421)");
                        return null;
                    }
                }

`vreader` is closed, but not `version`. The BR will close its reader for you, so skip futzing with vreader altogether and just do the BR:

if (vfile.canRead()) {
    final BufferedReader reader = new BufferedReader(new FileReader(vfile));
    try {
        final String version = reader.readLine();
        if (...) {
            ///
        }
    } finally {
        reader.close();
    }
}


See Bug 1105316 for this pattern.
Attached patch 1116633.patch (obsolete) — Splinter Review
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TBPL]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8543284 - Flags: review?(rnewman)
Hello Richard ,i have attached patch file please review it.
Comment on attachment 8543284 [details] [diff] [review]
1116633.patch

Review of attachment 8543284 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Let's keep the catch to avoid exceptions.

::: mobile/android/base/GeckoAppShell.java
@@ -1876,4 @@
>                      }
>                  }
> -            } catch (IOException ex) {
> -                // nothing

We still want to keep the enclosing try..catch around the whole block -- it just doesn't need a `finally`.
Attachment #8543284 - Flags: review?(rnewman) → feedback+
Attached patch 1116633_4.patchSplinter Review
Please review this patch ,here i have included try and catch around complete block.
Attachment #8545947 - Flags: review?(rnewman)
Comment on attachment 8545947 [details] [diff] [review]
1116633_4.patch

Review of attachment 8545947 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks! I'll land this.

::: mobile/android/base/GeckoAppShell.java
@@ +1891,3 @@
>              try {
>                  if (vfile.canRead()) {
>                      vreader = new FileReader(vfile);

This line can go.
Attachment #8545947 - Flags: review?(rnewman) → review+
Attachment #8543284 - Attachment is obsolete: true
Assignee: nobody → san13692
Status: NEW → ASSIGNED
Sorry but i dint understand which line needs to be removed here, and do i have to remove it or is it final..
https://hg.mozilla.org/mozilla-central/rev/cc52e6438c2f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 38
(In reply to surabhi anand from comment #7)
> Sorry but i dint understand which line needs to be removed here, and do i
> have to remove it or is it final..

I took care of it when I landed -- see the commit link above.
Thanx Richard :)
You need to log in before you can comment on or make changes to this bug.