Get GenericScopedHandle::Set to preserve LastError code

base/win/scoped_handle.cc

Index: base/win/scoped_handle.cc
diff --git a/base/win/scoped_handle.cc b/base/win/scoped_handle.cc
index ce944e43b9fe3fb30a7b699863efeb9ab0c8ee13..f37cc4bc2484be41cf80ed4f172a762cd5acb60a 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -228,20 +228,39 @@ void* GetHandleVerifier() {
 namespace base {
 namespace win {
 
+// A common pattern in Chrome is to create a handle using an OS function,

[grt (UTC plus 2), 2015/09/10 01:36:49]

this is possibly at the wrong layer. either GenericScopedHandle should document
that all Verifiers must preserve the l-e-c (and this should be tested somehow),
or the preservation should be done up in GenericScopedHandle. in either case,
the docs for GenericScopedHandle::Set (and Take, if that's the intent) should
explicitly state that the l-e-c is preserved so that it's clear that the
existing pattern broken by VS2015 is supported.

if you think it's best to keep this here rather than in GenericScopedHandle,
please move it into the unnamed namespace within this .cc file.

+// store it in a ScopedHandle, and then call GetLastError(). With VC++ 2015
+// this can fail (crbug.com/528394) because the handle tracking code may
+// allocate memory and that may zero out the LastError value. The
+// PreserveLastError object is used to save and restore the LastError code so
+// that this intuitive pattern works.
+class PreserveLastError {
+ public:
+  PreserveLastError() : last_error_(GetLastError()) {}
+  ~PreserveLastError() { SetLastError(last_error_); }
+
+ private:
+  unsigned last_error_;

[rvargas (doing something else), 2015/09/10 01:39:35]

DWORD (or unsigned int)

[brucedawson, 2015/09/10 21:52:49]

In the new code I went with 'auto'. Could also do DWORD, but 'auto' feels like
the cleanest way to indicate that no type-changing is happening.

+  DISALLOW_COPY_AND_ASSIGN(PreserveLastError);
+};
+
 // Static.
 bool HandleTraits::CloseHandle(HANDLE handle) {
+  PreserveLastError error_preserver;

[grt (UTC plus 2), 2015/09/10 01:36:49]

isn't this the wrong thing here? the last-error-code will be restored to
whatever it was before the call to CloseHandle below in case of failure, no? it
seems that we would want the l-e-c set by CloseHandle to persist outside of this
call. it's a bit moot since we'll crash the process if CloseHandle fails anyway,
but still.

[rvargas (doing something else), 2015/09/10 01:39:35]

I don't think we need this anywhere else but on Set(). (aka, StartTracking...
and that would mean that a class would not be needed either).

Note that the issue of last error preservation on base code (as making it a
guarantee of an API exposed by base) has come up in the past and has never had a
warm welcome.

Part of the argument has been that it's better to be consistent in that base
never cares about preserving last error and if someone needs that it should be
done when GetLastError is used.

But I see how it is easier to do it here, especially since we are dealing with
bugs that would be hard to track.

[brucedawson, 2015/09/10 01:46:27]

The bugs are hard to track and difficult to avoid. This change lets us continue
to use RAII on handles in a natural way, which would otherwise be complex.

The alternative would be something like:

HANDLE GetLEC(HANDLE handle, DWORD* error_code) {
  *error_code = GetLastError();
  return handle;
}

DWORD last_error = 0;
ScopedHandle a(GetLEC(CreateMutex(), &last_error));

It's not *too* terrible but it would have to be placed in an estimated ~30-40
places.

It is good to hear the historical perspective.

[brucedawson, 2015/09/10 01:46:27]

Having been bitten by this I felt compelled to preserve l-e-c everywhere, but I
think you're correct that in the close cases it is inappropriate.

[brucedawson, 2015/09/10 21:52:49]

CloseHandle failures only cause failures under the debugger. See bug 524267.

   return ActiveVerifier::Get()->CloseHandle(handle);
 }
 
 // Static.
 void VerifierTraits::StartTracking(HANDLE handle, const void* owner,
                                    const void* pc1, const void* pc2) {
+  PreserveLastError error_preserver;
   return ActiveVerifier::Get()->StartTracking(handle, owner, pc1, pc2);
 }
 
 // Static.
 void VerifierTraits::StopTracking(HANDLE handle, const void* owner,
                                   const void* pc1, const void* pc2) {
+  PreserveLastError error_preserver;
   return ActiveVerifier::Get()->StopTracking(handle, owner, pc1, pc2);
 }
 
@@ -250,6 +269,7 @@ void DisableHandleVerifier() {
 }
 
 void OnHandleBeingClosed(HANDLE handle) {
+  PreserveLastError error_preserver;

[grt (UTC plus 2), 2015/09/10 01:36:49]

this function is called before the handle is closed. why does the l-e-c need to
be preserved here?

[brucedawson, 2015/09/10 21:52:49]

I agree that the error codes should not be preserved when closing handles.

   return ActiveVerifier::Get()->OnHandleBeingClosed(handle);
 }