Get GenericScopedHandle::Set to preserve LastError code

Get GenericScopedHandle::Set to preserve LastError code

Code like this is elegant and clean and common in Chrome:

  event_.Set(CreateEvent(NULL, FALSE, FALSE, wname.c_str()));
  if (event_.Get() && GetLastError() != ERROR_ALREADY_EXISTS) {

However it will behave incorrectly if event_.Set() zeroes out the
Windows LastError code, which VC++ 2015 frequently does. This change
avoids that.

R=rvargas@chromium.org
BUG=528394, 529981, 440500
Committed: https://crrev.com/84f30fe06877193d39bb2cb9fe345c2e12e695b3
Cr-Commit-Position: refs/heads/master@{#348324}

[brucedawson, 2015-09-10 00:40:48]

I think we have to go with this fix - the evidence seems to
require it. Thoughts?

[Will Harris, 2015-09-10 00:52:09]

On 2015/09/10 00:40:48, brucedawson wrote:
> I think we have to go with this fix - the evidence seems to
> require it. Thoughts?

test?

[brucedawson, 2015-09-10 01:01:55]

Good point. I'll add.

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

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

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

wow. nice find! comments below.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:231: // A common pattern in Chrome is to create a
handle using an OS function,
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.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:249: PreserveLastError error_preserver;
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.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:272: PreserveLastError error_preserver;
this function is called before the handle is closed. why does the l-e-c need to
be preserved here?

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

If you land this then the other change should not land.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:243: unsigned last_error_;
DWORD (or unsigned int)

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:249: PreserveLastError error_preserver;
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:28]

Basic unit test added. I'll make changes tomorrow.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:249: PreserveLastError error_preserver;
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.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:249: PreserveLastError error_preserver;
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 21:52:49]

Updated code, comments, and title, and added unit tests.

The tests pass always with VS 2013, and pass with the fix on VS 2015.

PTAL.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:243: unsigned last_error_;
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.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:249: PreserveLastError error_preserver;
CloseHandle failures only cause failures under the debugger. See bug 524267.

https://codereview.chromium.org/1320673016/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:272: PreserveLastError error_preserver;
I agree that the error codes should not be preserved when closing handles.

[rvargas (doing something else), 2015-09-10 22:42:02]

lgtm, Thanks!

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.h:74: auto last_error = GetLastError();
nit: use ::GetLastError()

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle_...
File base/win/scoped_handle_unittest.cc (right):

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle_...
base/win/scoped_handle_unittest.cc:10: namespace win {
nit: Could you remove these namespaces?

long reason: I don't like to place tests under the same namespace of the code
under test because when someone is going through a random test to figure out why
it is failing, being as explicit as possible about where the code comes from is
a good thing. It may not be much of an issue in this specific case, but code may
come from the test file's anon namespace, or from the code under test, or from
the OS, or from any other file/library included here (more of an issue for tests
outside of base, but still).

It may be harder to write, but it is easier on the reader.

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle_...
base/win/scoped_handle_unittest.cc:17: HANDLE handle = CreateMutex(nullptr,
FALSE, nullptr);
nit: related to the previous one... use ::CreateMutex (etc)

[brucedawson, 2015-09-10 23:21:46]

Take one last look if you want.

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.h:74: auto last_error = GetLastError();
On 2015/09/10 22:42:02, rvargas (slow to respond) wrote:
> nit: use ::GetLastError()

Done. Same thing for SetLastError().

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle_...
File base/win/scoped_handle_unittest.cc (right):

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle_...
base/win/scoped_handle_unittest.cc:10: namespace win {
On 2015/09/10 22:42:02, rvargas (slow to respond) wrote:
> nit: Could you remove these namespaces?
> 
> long reason: I don't like to place tests under the same namespace of the code
> under test because when someone is going through a random test to figure out
why
> it is failing, being as explicit as possible about where the code comes from
is
> a good thing. It may not be much of an issue in this specific case, but code
may
> come from the test file's anon namespace, or from the code under test, or from
> the OS, or from any other file/library included here (more of an issue for
tests
> outside of base, but still).
> 
> It may be harder to write, but it is easier on the reader.

We should probably do a code-cleanup pass at some point because about two thirds
of base\win\*unittest.cc files use base\win namespaces.

https://codereview.chromium.org/1320673016/diff/60001/base/win/scoped_handle_...
base/win/scoped_handle_unittest.cc:17: HANDLE handle = CreateMutex(nullptr,
FALSE, nullptr);
On 2015/09/10 22:42:02, rvargas (slow to respond) wrote:
> nit: related to the previous one... use ::CreateMutex (etc)

Done, for CreateMutex(), SetLastError(), and GetLastError(), for all three
sub-tests.

[brucedawson, 2015-09-11 00:16:36]

The CQ bit was checked by brucedawson@chromium.org

[brucedawson, 2015-09-11 00:16:36]

The patchset sent to the CQ was uploaded after l-g-t-m from rvargas@chromium.org
Link to the patchset: https://codereview.chromium.org/1320673016/#ps80001
(title: "Remove namespaces")

[commit-bot: I haz the power, 2015-09-11 00:17:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320673016/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320673016/80001

[commit-bot: I haz the power, 2015-09-11 03:08:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-11 03:09:57]

Patchset 5 (id:??) landed as
https://crrev.com/84f30fe06877193d39bb2cb9fe345c2e12e695b3
Cr-Commit-Position: refs/heads/master@{#348324}

[grt (UTC plus 2), 2015-09-11 14:51:30]

lgtm

[dcheng, 2015-09-18 21:22:18]

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

[dcheng, 2015-09-18 21:22:19]

https://codereview.chromium.org/1320673016/diff/80001/base/win/scoped_handle_...
File base/win/scoped_handle_unittest.cc (right):

https://codereview.chromium.org/1320673016/diff/80001/base/win/scoped_handle_...
base/win/scoped_handle_unittest.cc:30: handle_holder = std::move(handle_source);
Shouldn't this be Pass() rather than std::move()? AFAIK, we've been banning the
use of C++11 library features, even on platforms that should support it.

[rvargas (doing something else), 2015-09-18 22:00:48]

https://codereview.chromium.org/1320673016/diff/80001/base/win/scoped_handle_...
File base/win/scoped_handle_unittest.cc (right):

https://codereview.chromium.org/1320673016/diff/80001/base/win/scoped_handle_...
base/win/scoped_handle_unittest.cc:30: handle_holder = std::move(handle_source);
On 2015/09/18 21:22:19, dcheng wrote:
> Shouldn't this be Pass() rather than std::move()? AFAIK, we've been banning
the
> use of C++11 library features, even on platforms that should support it.

oops. Yes.

[brucedawson, 2015-09-18 22:01:21]

https://codereview.chromium.org/1320673016/diff/80001/base/win/scoped_handle_...
File base/win/scoped_handle_unittest.cc (right):

https://codereview.chromium.org/1320673016/diff/80001/base/win/scoped_handle_...
base/win/scoped_handle_unittest.cc:30: handle_holder = std::move(handle_source);
On 2015/09/18 21:22:19, dcheng wrote:
> Shouldn't this be Pass() rather than std::move()? AFAIK, we've been banning
the
> use of C++11 library features, even on platforms that should support it.

You are correct. I will fix.

[commit-bot: I haz the power, 2015-09-23 12:18:18]

Patchset 5 (id:??) landed as
https://crrev.com/84f30fe06877193d39bb2cb9fe345c2e12e695b3
Cr-Commit-Position: refs/heads/master@{#348324}