Bug 1230910 Remove some __try blocks that are unneeded in our use case draft
authorTom Ritter <tom@mozilla.com>
Wed, 08 Mar 2017 19:01:01 +0000
changeset 496061 75e210bf94466b32a4e009d9206335a44c214fb6
parent 496060 7d1aeb930578fd5409e1e22a89295111319f074a
child 496062 ba608486cd0e9ad48388e8db4249249b621408db
push id48518
push userbmo:tom@mozilla.com
push dateThu, 09 Mar 2017 19:30:51 +0000
bugs1230910
milestone52.0.1
Bug 1230910 Remove some __try blocks that are unneeded in our use case MozReview-Commit-ID: FHtNniEp8pQ
security/sandbox/chromium/base/threading/platform_thread_win.cc
security/sandbox/chromium/sandbox/win/src/handle_closer_agent.cc
--- a/security/sandbox/chromium/base/threading/platform_thread_win.cc
+++ b/security/sandbox/chromium/base/threading/platform_thread_win.cc
@@ -25,30 +25,37 @@ const DWORD kVCThreadNameException = 0x4
 
 typedef struct tagTHREADNAME_INFO {
   DWORD dwType;  // Must be 0x1000.
   LPCSTR szName;  // Pointer to name (in user addr space).
   DWORD dwThreadID;  // Thread ID (-1=caller thread).
   DWORD dwFlags;  // Reserved for future use, must be zero.
 } THREADNAME_INFO;
 
-// This function has try handling, so it is separated out of its caller.
+#if 0
+  //This function is only used for debugging purposes, as you can find by its caller
+  //Here is the commit it was added in:
+  //https://codereview.chromium.org/8565036
+  //https://codereview.chromium.org/8565036/diff/6002/base/threading/platform_thread_win.cc
+
+  // This function has try handling, so it is separated out of its caller.
 void SetNameInternal(PlatformThreadId thread_id, const char* name) {
   THREADNAME_INFO info;
   info.dwType = 0x1000;
   info.szName = name;
   info.dwThreadID = thread_id;
   info.dwFlags = 0;
 
   __try {
     RaiseException(kVCThreadNameException, 0, sizeof(info)/sizeof(DWORD),
                    reinterpret_cast<DWORD_PTR*>(&info));
   } __except(EXCEPTION_CONTINUE_EXECUTION) {
   }
 }
+#endif
 
 struct ThreadParams {
   PlatformThread::Delegate* delegate;
   bool joinable;
   ThreadPriority priority;
 };
 
 DWORD __stdcall ThreadFunc(void* params) {
@@ -176,17 +183,19 @@ void PlatformThread::SetName(const std::
 
   // The debugger needs to be around to catch the name in the exception.  If
   // there isn't a debugger, we are just needlessly throwing an exception.
   // If this image file is instrumented, we raise the exception anyway
   // to provide the profiler with human-readable thread names.
   if (!::IsDebuggerPresent() && !base::debug::IsBinaryInstrumented())
     return;
 
+  #if 0
   SetNameInternal(CurrentId(), name.c_str());
+  #endif
 }
 
 // static
 const char* PlatformThread::GetName() {
   return ThreadIdNameManager::GetInstance()->GetName(CurrentId());
 }
 
 // static
--- a/security/sandbox/chromium/sandbox/win/src/handle_closer_agent.cc
+++ b/security/sandbox/chromium/sandbox/win/src/handle_closer_agent.cc
@@ -19,22 +19,36 @@ namespace {
 NTSTATUS QueryObjectTypeInformation(HANDLE handle,
                                     void* buffer,
                                     ULONG* size) {
   static NtQueryObject QueryObject = NULL;
   if (!QueryObject)
     ResolveNTFunctionPtr("NtQueryObject", &QueryObject);
 
   NTSTATUS status = STATUS_UNSUCCESSFUL;
-  __try {
+  /*
+   * Removing this exception handler should be safe. It is used to suppress 
+   * STATUS_INVALID_HANDLE exceptions and continue execution normally.
+   * Those exceptions are only raised when handle tracing is enabled.
+   * (This may be done by AppVerifier for example)
+   *
+   * If we don't enable handle tracing, we shouldn't see these exceptions
+   * and therefore it's as if this __except wasn't here (since it passes
+   * all other exceptions up the chain.)
+   *
+   * Ref:
+   * https://chromium.googlesource.com/chromium/src/+blame/4ec79b7f2379a60cdc15599e93255c0fa417f1ed/sandbox/win/src/handle_closer_agent.cc
+   * https://chromium.googlesource.com/chromium/src/+/4d3f6a99781f7bdca5237c49fccdc6c796e6abcc
+   */
+    //__try {
     status = QueryObject(handle, ObjectTypeInformation, buffer, *size, size);
-  } __except(GetExceptionCode() == STATUS_INVALID_HANDLE ?
-                 EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
-    status = STATUS_INVALID_HANDLE;
-  }
+    //} __except(GetExceptionCode() == STATUS_INVALID_HANDLE ?
+    //             EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
+    //status = STATUS_INVALID_HANDLE;
+    //}
   return status;
 }
 
 }  // namespace
 
 namespace sandbox {
 
 // Memory buffer mapped from the parent, with the list of handles.