Adding checks in sandbox code to get some data on AppContainer based CreateProcess failures.

Adding checks in sandbox code to get some data on AppContainer based CreateProcess failures.
Due to limited debugging options, given it doesn't repro locally, adding some checks in sandbox code to gather more data.

BUG=467920
R=jschuh@chromium.org,cpu@chromium.org

Committed: https://crrev.com/cb0da85dcf65824f96233ed77fd04d99b4c031cb
Cr-Commit-Position: refs/heads/master@{#325663}

[Will Harris, 2015-04-15 23:56:29]

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

[Will Harris, 2015-04-15 23:56:29]

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
sandbox/win/src/target_process.cc:164: CHECK(FALSE);
Have you considered just returning a new SBOX error code here? Then the error
should appear in Process.Sandbox.Launch.Error UMA histogram

also - is this the same issue as 385714 and 453541, it certainly looks like it
from the bug...

[Shrikant Kelkar, 2015-04-16 00:06:36]

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
sandbox/win/src/target_process.cc:164: CHECK(FALSE);
On 2015/04/15 23:56:29, Will Harris wrote:
> Have you considered just returning a new SBOX error code here? Then the error
> should appear in Process.Sandbox.Launch.Error UMA histogram
> 
> also - is this the same issue as 385714 and 453541, it certainly looks like it
> from the bug...

Yes, that's what I was communicating with reporter earlier. I think reporter
definitely is stuck with issues 385714, but besides that I am receiving 122
ERROR_INSUFFICIENT_BUFFER which I want to find reason for.

This function returns GetLastError codes, sorry but didn't get what you mean by
return SBOX error code.

[Will Harris, 2015-04-16 00:13:37]

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
sandbox/win/src/target_process.cc:164: CHECK(FALSE);
On 2015/04/16 00:06:36, Shrikant Kelkar wrote:
> On 2015/04/15 23:56:29, Will Harris wrote:
> > Have you considered just returning a new SBOX error code here? Then the
error
> > should appear in Process.Sandbox.Launch.Error UMA histogram
> > 
> > also - is this the same issue as 385714 and 453541, it certainly looks like
it
> > from the bug...
> 
> Yes, that's what I was communicating with reporter earlier. I think reporter
> definitely is stuck with issues 385714, but besides that I am receiving 122
> ERROR_INSUFFICIENT_BUFFER which I want to find reason for.
> 
> This function returns GetLastError codes, sorry but didn't get what you mean
by
> return SBOX error code.

ah okay I meant instead of returning GetLastError you could create six new
SBOX_* error codes in sandbox_types.h then return a different one from each
potential failure point, in order to know where it's failing, rather than doing
a CHECK() which will terminate the browser process.

However, this would mean losing the GetLastError() since you can only return one
thing - so perhaps just if(::GetLastError() == ERROR_INSUFFICIENT_BUFFER) checks
around the new error codes?

You could also consider using base::debug::DumpWithoutCrashing() if you want the
diagnosis code not to terminate the browser process.

[Shrikant Kelkar, 2015-04-16 18:25:34]

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/1093443002/diff/1/sandbox/win/src/target_proc...
sandbox/win/src/target_process.cc:164: CHECK(FALSE);
On 2015/04/16 00:13:36, Will Harris wrote:
> On 2015/04/16 00:06:36, Shrikant Kelkar wrote:
> > On 2015/04/15 23:56:29, Will Harris wrote:
> > > Have you considered just returning a new SBOX error code here? Then the
> error
> > > should appear in Process.Sandbox.Launch.Error UMA histogram
> > > 
> > > also - is this the same issue as 385714 and 453541, it certainly looks
like
> it
> > > from the bug...
> > 
> > Yes, that's what I was communicating with reporter earlier. I think reporter
> > definitely is stuck with issues 385714, but besides that I am receiving 122
> > ERROR_INSUFFICIENT_BUFFER which I want to find reason for.
> > 
> > This function returns GetLastError codes, sorry but didn't get what you mean
> by
> > return SBOX error code.
> 
> ah okay I meant instead of returning GetLastError you could create six new
> SBOX_* error codes in sandbox_types.h then return a different one from each
> potential failure point, in order to know where it's failing, rather than
doing
> a CHECK() which will terminate the browser process.
> 
> However, this would mean losing the GetLastError() since you can only return
one
> thing - so perhaps just if(::GetLastError() == ERROR_INSUFFICIENT_BUFFER)
checks
> around the new error codes?
> 
> You could also consider using base::debug::DumpWithoutCrashing() if you want
the
> diagnosis code not to terminate the browser process.

Basically, I don't want to make these changes permanent in src/sandbox/.. It
just get more diagnostic information and then revert, as I am running out of
option finding the root cause.

DumpWithoutCrashing was the option I tried before, but that was in
content/../sandbox_win.cc, the mystery remains which exact function from
TargetProcess::Create fails with ERROR_INSUFFICIENT_BUFFER. IIRC cpu/rvargas had
a different take on adding DumpWithoutCrashing into src/sandbox/.. (Bringing in
dependency). I guess.. if I am going to revert immediately cpu@ may allow this
:).

[jschuh, 2015-04-16 23:59:39]

Just to clarify, do we think every tab is crashing for these users today?
Because if so, then I can see landing this for a day or two. If not, do we know
roughly how many unique users are hitting this crash? Because, I'm worried we're
going to turn a small nuisance into a big nuisance.

[Shrikant Kelkar, 2015-04-17 00:12:28]

On 2015/04/16 23:59:39, jschuh (out April 2015) wrote:
> Just to clarify, do we think every tab is crashing for these users today?
> Because if so, then I can see landing this for a day or two. If not, do we
know
> roughly how many unique users are hitting this crash? Because, I'm worried
we're
> going to turn a small nuisance into a big nuisance.

For 44.0.2368.0 64-W 7day around 1480 users are reporting this type of error
(Not sure out of how many though). Instead of causing actual crash how do you
feel about pulling in dependency of DumpWithoutCrash to src/sandbox for couple
of days before revert happens.
From users who communicated to us, it doesn't seem a consistent repro either,
the video one person sent suggests that he has to try multiple times
opening/closing tabs before he gets error and even after closing that failed tab
he still can continue opening new tabs in same session.

[jschuh, 2015-04-17 00:29:30]

Yeah, I'm not okay with this then because it would turn an occasional sad tab
into a full browser crash. I'm far more comfortable with temporarily pulling the
dependency and then reverting it in a day or two.

[Shrikant Kelkar, 2015-04-17 01:10:09]

On 2015/04/17 00:29:30, jschuh (out April 2015) wrote:
> Yeah, I'm not okay with this then because it would turn an occasional sad tab
> into a full browser crash. I'm far more comfortable with temporarily pulling
the
> dependency and then reverting it in a day or two.

Changed CHECK() into DumpWithoutCrash(). ptal.

[jschuh, 2015-04-17 10:58:22]

lgtm. just make sure it gets reverted right after you get the data.

[Shrikant Kelkar, 2015-04-17 15:56:40]

The CQ bit was checked by shrikant@chromium.org

[commit-bot: I haz the power, 2015-04-17 15:57:56]

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

[commit-bot: I haz the power, 2015-04-17 17:34:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-17 17:35:42]

Patchset 2 (id:??) landed as
https://crrev.com/cb0da85dcf65824f96233ed77fd04d99b4c031cb
Cr-Commit-Position: refs/heads/master@{#325663}

[Will Harris, 2015-04-18 23:43:32]

I see 169 reports on canary 44.0.2374.0 so far (top crasher), can I revert this
now?

[Shrikant Kelkar, 2015-04-18 23:47:46]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1057083006/ by shrikant@chromium.org.

The reason for reverting is: Collected dumps, reverting..