A couple of possible options, both with drawbacks:

1) Use a helper class (ChromeStrictMode or a new class) that implements java.io.Closeable with static allowDiskReads() and allowDiskWrites() methods that return an instance of the class. Clients can use try-with-resources syntax to add StrictMode exceptions. This will result in unused variables since try-with-resource  requires this.

e.g. try (ChromeStrictMode unused = ChromeStrictMode.allowDiskWrites()) {...

2) Another option would just be to add utility methods that take a callback of some sort that runs when the StrictMode violation is allowed - however this isn't great for readability and it also makes debugging more difficult.

e.g. ChromeStrictMode.allowDiskWrites(() -> {...

I'm not sure either of these is actually worth doing - any other ideas?

Option 2 would mean you're defining a new class for each block, which we certainly don't want.

Option 1 sgtm though. Compare:

Before:
        StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
        try {
            return ChromePreferenceManager.getInstance().getCachedChromeDefaultBrowser();
        } finally {
            StrictMode.setThreadPolicy(oldPolicy);
        }


After:

        try (ChromeStrictMode _ = ChromeStrictMode.allowDiskReads()) {
            return ChromePreferenceManager.getInstance().getCachedChromeDefaultBrowser();
        }

Option 1 is much better than our current method. +1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69f336c303c06133576d7d2a1b253c464ccdd3c7

commit 69f336c303c06133576d7d2a1b253c464ccdd3c7
Author: Eric Stevenson <estevenson@chromium.org>
Date: Mon Sep 18 23:29:09 2017

Android: Add try-with-resources compatible ThreadPolicy methods.

This CL adds a simple utility class with static methods for
permitting StrictMode disk-reads and disk-writes. There is no behavior
change, the convenience methods just make StrictMode white-listing
more readable.

Before:
StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
try {
  return methodThatWritesToDisk();
} finally {
  StrictMode.setThreadPolicy(oldPolicy);
}

After:
try (StrictModeContext unused = StrictModeContext.allowDiskReads()) {
  return methodThatWritesToDisk();
}

Bug:  766228 
Change-Id: If07525e95236d1533531275c7b1564e3ae9f3433
Reviewed-on: https://chromium-review.googlesource.com/671550
Commit-Queue: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502710}
[modify] https://crrev.com/69f336c303c06133576d7d2a1b253c464ccdd3c7/base/BUILD.gn
[modify] https://crrev.com/69f336c303c06133576d7d2a1b253c464ccdd3c7/base/android/java/src/org/chromium/base/PathUtils.java
[add] https://crrev.com/69f336c303c06133576d7d2a1b253c464ccdd3c7/base/android/java/src/org/chromium/base/StrictModeContext.java

Do we want to convert all raw uses of StrictMode.allowThreadDiskWrites/Reads to use the try-with-resources method? There are about 67 instances upstream (https://cs.chromium.org/search/?q=StrictMode.allowThreadDisk+-file:third_party&sq=package:chromium&type=cs) and 10 downstream.

I think it's worthwhile if we add a presubmit check preventing use of StrictMode directly. If you guys agree I'll post a question on java@chromium.org before moving forward.

 

I don't think it's worth your time to do conversions. Time would be better spent fixing them :P. I like the idea of adding a presubmit to disallow new ones that don't use the helper

Unfortunately many of them cannot be fixed due to being something not directly the fault of Chrome. It's hard to find exactly the ones that we can do something about.

+1 for adding presubmit check to prevent new ones being added without the helper.

I do think it might be worthwhile for readability and code health to convert the existing ones. People write new code based on existing code, and if that's all they see they won't be inclined to understand/use the new way.

Also, to add to the value of doing the conversions, by going through each one and making sure (to a reasonable degree and no more) that there's nothing we can do, we can actually find if any of them have become actionable or were added wrongly. See https://crbug.com/398264.

Created issue 767058 for tracking converting StrictMode usage to StrictModeContext.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9a98097b01ede57be91cec0daa72c46350dec09

commit a9a98097b01ede57be91cec0daa72c46350dec09
Author: Eric Stevenson <estevenson@chromium.org>
Date: Sat Sep 23 00:04:41 2017

Android: Add presubmit check for StrictMode.

New violations should be white-listed using the try-with-resources
method in StrictModeContext.

Bug:  766228 
Change-Id: Ic420189aaa2eb2e3fbb9cbc06250d09e4364e251
Reviewed-on: https://chromium-review.googlesource.com/673846
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Eric Stevenson <estevenson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503913}
[modify] https://crrev.com/a9a98097b01ede57be91cec0daa72c46350dec09/PRESUBMIT.py

Thanks a lot for adding this, Eric!
Do you plan to add tests for StrictModeContext?

I initially had some tests but they were pretty much testing "does try-with-resources work" and "does Android StrictMode work" so I left them out. I can add some though as a way to make sure we don't change the behavior of StrictModeContext and mess up resetting the ThreadPolicy.

Any ideas for more meaningful tests?

No ideas for more meaningful tests, sorry.

I see your point that "does try-with-resources work" and "does Android StrictMode work" doesn't bring much value, but considering the importance of StrictModeContext adding them still makes sense to me.
I'm leaving final decision to your discretion.

Again, thanks for adding this! With or without tests it's way better than old try/finally blocks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9cb1fdf5e3fc5eafc742c55d25d03b07af70198

commit d9cb1fdf5e3fc5eafc742c55d25d03b07af70198
Author: Eric Stevenson <estevenson@chromium.org>
Date: Sat Oct 07 01:14:45 2017

Android: Add tests for StrictModeContext.

Bug:  766228 
Change-Id: I9638166611101518aef7d5d6168b8d7ff06c4888
Reviewed-on: https://chromium-review.googlesource.com/705974
Commit-Queue: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507253}
[modify] https://crrev.com/d9cb1fdf5e3fc5eafc742c55d25d03b07af70198/base/BUILD.gn
[add] https://crrev.com/d9cb1fdf5e3fc5eafc742c55d25d03b07af70198/base/android/javatests/src/org/chromium/base/StrictModeContextTest.java