Fix CloseHandle blunder. I was thinking that the handle would be closed by S...

Fix CloseHandle blunder.  I was thinking that the handle would be closed by Set, but of course I need to close the handle before the CreateFile call... doh!


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=6178

[Nicolas Sylvain, 2008-12-01 23:12:50]

LGTM

[darin (slow to review), 2008-12-08 18:18:44]

http://codereview.chromium.org/13027/diff/202/203
File base/test_file_util_win.cc (right):

http://codereview.chromium.org/13027/diff/202/203#newcode88
Line 88: file_handle.Set(CreateFile(file, GENERIC_WRITE, 0, NULL, OPEN_EXISTING,
maybe the smart pointer class should be made smarter?  this seems like something
that could easily be misused again in the future, and it would suck if that were
actually in production code!

[tommi (sloooow) - chröme, 2008-12-09 10:07:45]

http://codereview.chromium.org/13027/diff/202/203
File base/test_file_util_win.cc (right):

http://codereview.chromium.org/13027/diff/202/203#newcode88
Line 88: file_handle.Set(CreateFile(file, GENERIC_WRITE, 0, NULL, OPEN_EXISTING,
in this case I think it was just the engineer that should have been smarter ;)
although (arguably), the Set() method might be too prone to this kind of thing. 
When I tested the code, I was using ATL's CHandle class and not ScopedHandle. 
CHandle exposes the Close() method, which is what I called.  Close() is private
for ScopeHandle, so I couldn't call it and when I then switched out CHandle for
ScopeHandle, I thought "oh, Set() already closes the handle for me, so all I
need is Set()"... big mistake.
Since handles are slightly different from regular memory pointers (i.e. no
mutual exclusion) it might be better to have no [re]Set() method, but instead
have Close() and Attach() where Attach() only DCHECKs that it's not already
valid.  That way we get away with as little generated code as possible and avoid
cases like this.  In this particular case, since I have to use Set(), two
possible calls to CloseHandle will be generated but only one will ever execute. 
That's a few wasted bytes :)