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

sandbox/win/src/target_process.cc

Index: sandbox/win/src/target_process.cc
diff --git a/sandbox/win/src/target_process.cc b/sandbox/win/src/target_process.cc
index e0284c3924b27cd8f2419b7eda5ea3dd06471267..8e1e938eefedc8641ba2c9dd4004cf91ec72b9b8 100644
--- a/sandbox/win/src/target_process.cc
+++ b/sandbox/win/src/target_process.cc
@@ -5,6 +5,7 @@
 #include "sandbox/win/src/target_process.h"
 
 #include "base/basictypes.h"
+#include "base/debug/alias.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/win/pe_image.h"
 #include "base/win/startup_information.h"
@@ -157,6 +158,10 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
                          NULL,   // Use current directory of the caller.
                          startup_info.startup_info(),
                          &temp_process_info)) {
+      // TODO(shrikant): Remove following code once we gather some crashes for
+      // debugging appcontainer related failures (crbug.com/467920).
+      base::debug::Alias(exe_path);
+      CHECK(FALSE);

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

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]

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:36]

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]

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
:).

       return ::GetLastError();
     }
   } else {
@@ -182,6 +187,12 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
     // Assign the suspended target to the windows job object.
     if (!::AssignProcessToJobObject(job_, process_info.process_handle())) {
       win_result = ::GetLastError();
+      if (set_lockdown_token_after_create) {
+        // TODO(shrikant): Remove this code once we gather some crashes for
+        // debugging appcontainer related failures (crbug.com/467920).
+        base::debug::Alias(&win_result);
+        CHECK(FALSE);
+      }
       ::TerminateProcess(process_info.process_handle(), 0);
       return win_result;
     }
@@ -194,6 +205,12 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
     HANDLE temp_thread = process_info.thread_handle();
     if (!::SetThreadToken(&temp_thread, initial_token_.Get())) {
       win_result = ::GetLastError();
+      if (set_lockdown_token_after_create) {
+        // TODO(shrikant): Remove this code once we gather some crashes for
+        // debugging appcontainer related failures (crbug.com/467920).
+        base::debug::Alias(&win_result);
+        CHECK(FALSE);
+      }
       // It might be a security breach if we let the target run outside the job
       // so kill it before it causes damage.
       ::TerminateProcess(process_info.process_handle(), 0);
@@ -217,6 +234,10 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
         sizeof(process_access_token));
     if (!NT_SUCCESS(status)) {
       win_result = ::GetLastError();
+      // TODO(shrikant): Remove this code once we gather some crashes for
+      // debugging appcontainer related failures (crbug.com/467920).
+      base::debug::Alias(&win_result);
+      CHECK(FALSE);
       ::TerminateProcess(process_info.process_handle(), 0);  // exit code
       return win_result;
     }
@@ -226,6 +247,12 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
   context.ContextFlags = CONTEXT_ALL;
   if (!::GetThreadContext(process_info.thread_handle(), &context)) {
     win_result = ::GetLastError();
+    if (set_lockdown_token_after_create) {
+      // TODO(shrikant): Remove this code once we gather some crashes for
+      // debugging appcontainer related failures (crbug.com/467920).
+      base::debug::Alias(&win_result);
+      CHECK(FALSE);
+    }
     ::TerminateProcess(process_info.process_handle(), 0);
     return win_result;
   }
@@ -242,6 +269,12 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
 
   if (!target_info->DuplicateFrom(process_info)) {
     win_result = ::GetLastError();  // This may or may not be correct.
+    if (set_lockdown_token_after_create) {
+      // TODO(shrikant): Remove this code once we gather some crashes for
+      // debugging appcontainer related failures (crbug.com/467920).
+      base::debug::Alias(&win_result);
+      CHECK(FALSE);
+    }
     ::TerminateProcess(process_info.process_handle(), 0);
     return win_result;
   }