1116633 - Unclosed BufferedReader in GeckoAppShell.getPluginDirectories

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[surabhi anand]

Attachment: 1116633.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TBPL]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 2

[surabhi anand]

Hello Richard ,i have attached patch file please review it.

Comment 3

[Richard Newman [:rnewman]]

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

Comment 4

[surabhi anand]

Attachment: 1116633_4.patch

Please review this patch ,here i have included try and catch around complete block.

Comment 5

[Richard Newman [: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.

Comment 6

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/cc52e6438c2f

Comment 7

[surabhi anand]

Sorry but i dint understand which line needs to be removed here, and do i have to remove it or is it final..

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Richard Newman [:rnewman]]

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

Comment 10

[surabhi anand]

Thanx Richard :)