Replace system IO calls in chromecast/crash with Chrome utilities.

Replace system IO calls in chromecast/crash with Chrome utilities.

System IO calls had been used to avoid thread IO restrictions in the
crash handler. This change replaces each of these calls with the more
platform-independent, stable Chrome IO utilities from ::base. Thread
restrictions are disabled in crash_util.cc.

BUG= b/22329428

Committed: https://crrev.com/4927df40d0342522fc268f884835b43f9004549e
Cr-Commit-Position: refs/heads/master@{#339161}

[slan, 2015-07-16 14:53:45]

slan@chromium.org changed reviewers:
+ bcf@google.com, gfhuang@chromium.org, wzhong@chromium.org

[slan, 2015-07-16 15:57:36]

PTAL.

[gfhuang, 2015-07-16 18:39:50]

gfhuang@chromium.org changed reviewers:
+ gunsch@chromium.org
- gfhuang@chromium.org

[gfhuang, 2015-07-16 18:39:58]

gfhuang@chromium.org changed reviewers:
+ gfhuang@chromium.org

[gfhuang, 2015-07-16 18:39:59]

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
this has to be scopedIO or you need to SetIOAllowed(false) when it all ends.

[gunsch, 2015-07-16 18:49:18]

lgtm

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
On 2015/07/16 18:39:59, gfhuang wrote:
> this has to be scopedIO or you need to SetIOAllowed(false) when it all ends.

ScopedAllowIO is banned.

I think this is only used in the context of while we're actively crashing,
right? If not, we should definitely SetIOAllowed(true) afterwards.

[wzhong, 2015-07-16 18:52:19]

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
On 2015/07/16 18:49:18, gunsch wrote:
> On 2015/07/16 18:39:59, gfhuang wrote:
> > this has to be scopedIO or you need to SetIOAllowed(false) when it all ends.
> 
> ScopedAllowIO is banned.
> 
> I think this is only used in the context of while we're actively crashing,
> right? If not, we should definitely SetIOAllowed(true) afterwards.

SetIOAllowed() returns previous state.

Save it and call SetIOAllowed() again.

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
On 2015/07/16 18:49:18, gunsch wrote:
> On 2015/07/16 18:39:59, gfhuang wrote:
> > this has to be scopedIO or you need to SetIOAllowed(false) when it all ends.
> 
> ScopedAllowIO is banned.
> 
> I think this is only used in the context of while we're actively crashing,
> right? If not, we should definitely SetIOAllowed(true) afterwards.

SetIOAllowed() returns previous state.

Save it and call SetIOAllowed() again afterwards.

[wzhong, 2015-07-16 18:54:19]

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
On 2015/07/16 18:52:19, wzhong wrote:
> On 2015/07/16 18:49:18, gunsch wrote:
> > On 2015/07/16 18:39:59, gfhuang wrote:
> > > this has to be scopedIO or you need to SetIOAllowed(false) when it all
ends.
> > 
> > ScopedAllowIO is banned.
> > 
> > I think this is only used in the context of while we're actively crashing,
> > right? If not, we should definitely SetIOAllowed(true) afterwards.
> 
> SetIOAllowed() returns previous state.
> 
> Save it and call SetIOAllowed() again.

It should be used only around base::Move().

[gfhuang, 2015-07-16 18:57:01]

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
On 2015/07/16 18:49:18, gunsch wrote:
> On 2015/07/16 18:39:59, gfhuang wrote:
> > this has to be scopedIO or you need to SetIOAllowed(false) when it all ends.
> 
> ScopedAllowIO is banned.
> 
> I think this is only used in the context of while we're actively crashing,
> right? If not, we should definitely SetIOAllowed(true) afterwards.

We need to set it back to false then.

This could be called in browser process when rendered process is crashed.
so browser process is still alive after all these ends.

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
On 2015/07/16 18:54:18, wzhong wrote:
> On 2015/07/16 18:52:19, wzhong wrote:
> > On 2015/07/16 18:49:18, gunsch wrote:
> > > On 2015/07/16 18:39:59, gfhuang wrote:
> > > > this has to be scopedIO or you need to SetIOAllowed(false) when it all
> ends.
> > > 
> > > ScopedAllowIO is banned.
> > > 
> > > I think this is only used in the context of while we're actively crashing,
> > > right? If not, we should definitely SetIOAllowed(true) afterwards.
> > 
> > SetIOAllowed() returns previous state.
> > 
> > Save it and call SetIOAllowed() again.

+1, the process could be alive after these (browser process for example)
so need to restore the value.

> 
> It should be used only around base::Move().

base:FileExists also needs it.

[slan, 2015-07-16 20:06:24]

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/100001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42:
base::ThreadRestrictions::SetIOAllowed(true);
> +1, the process could be alive after these (browser process for example)
> so need to restore the value.

I was not aware that this could be posted to the UI thread, for example. Done.

> base:FileExists also needs it.

I set and remove the permissions in the top-level function, rather than wrapping
each call individually. This will be beneficial when many other Chrome IO calls
are substituted for existing system calls in future CLs. (Most notably in
SynchronizedMinidumpManager)

[wzhong, 2015-07-16 21:08:20]

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42: bool io_allowed =
base::ThreadRestrictions::SetIOAllowed(true);
const bool

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/dummy_minidump_generator_unittest.cc (right):

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/dummy_minidump_generator_unittest.cc:15: // This value
should stay in sync with the internal buffer size used for copying
The comment seems incorrect.

[slan, 2015-07-16 22:39:19]

The patch has been verified with and without IO restrictions on production
hardware.

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:42: bool io_allowed =
base::ThreadRestrictions::SetIOAllowed(true);
On 2015/07/16 21:08:20, wzhong wrote:
> const bool

Done.

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
File chromecast/crash/linux/dummy_minidump_generator_unittest.cc (right):

https://codereview.chromium.org/1227963002/diff/120001/chromecast/crash/linux...
chromecast/crash/linux/dummy_minidump_generator_unittest.cc:15: // This value
should stay in sync with the internal buffer size used for copying
On 2015/07/16 21:08:20, wzhong wrote:
> The comment seems incorrect.

Okay. Now that we are using the Chrome utility, we no longer need to know or
test implementation details of the move operation.

[wzhong, 2015-07-16 22:46:44]

lgtm

https://codereview.chromium.org/1227963002/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/1227963002/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:159: LOG(WARNING) <<
"Ignoring invalid entry: " << entry;
Remove {} for simple if else statement.

[slan, 2015-07-16 22:57:31]

https://codereview.chromium.org/1227963002/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/synchronized_minidump_manager.cc (right):

https://codereview.chromium.org/1227963002/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/synchronized_minidump_manager.cc:159: LOG(WARNING) <<
"Ignoring invalid entry: " << entry;
On 2015/07/16 22:46:44, wzhong wrote:
> Remove {} for simple if else statement.

Done.

[slan, 2015-07-16 22:57:38]

The CQ bit was checked by slan@chromium.org

[slan, 2015-07-16 22:57:38]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
wzhong@chromium.org
Link to the patchset: https://codereview.chromium.org/1227963002/#ps160001
(title: "nit.")

[commit-bot: I haz the power, 2015-07-16 22:58:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1227963002/160001

[commit-bot: I haz the power, 2015-07-16 23:17:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-07-16 23:20:00]

Patchset 9 (id:??) landed as
https://crrev.com/4927df40d0342522fc268f884835b43f9004549e
Cr-Commit-Position: refs/heads/master@{#339161}

[dcaiafa, 2015-07-17 00:06:56]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1239083005/ by dcaiafa@chromium.org.

The reason for reverting is: Breaks
CastCrashReporterClientTest.EndToEndTestOnIORestrictedThread:

https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/2709.