ScopedNativeLibrary.Basic test fix

ScopedNativeLibrary.Basic test fix

IsBadCodePtr() function was returning zero value randomly and the test was
raising false alarms sometimes. This fix changes the test to use
GetFunctionPointerFromNativeLibrary() instead.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232192

[Mark Mentovai, 2013-10-25 20:13:26]

https://codereview.chromium.org/32303002/diff/1/base/scoped_native_library_un...
File base/scoped_native_library_unittest.cc (right):

https://codereview.chromium.org/32303002/diff/1/base/scoped_native_library_un...
base/scoped_native_library_unittest.cc:25: const char* kFunctionName =
"DirectDrawCreate";
Use const char kFunctionName[] instead.

https://codereview.chromium.org/32303002/diff/1/base/scoped_native_library_un...
base/scoped_native_library_unittest.cc:39:
GetFunctionPointerFromNativeLibrary(native_library, kFunctionName));
Is this safe? You’re expecting native_library to have been freed here (based on
the comment above stating that ddraw should not have been loaded to begin with),
and thus once unloaded by the ScopedNativeLibrary going out of scope, the
native_library handle will be invalid.

[Tomasz Moniuszko, 2013-10-28 10:24:21]

https://codereview.chromium.org/32303002/diff/1/base/scoped_native_library_un...
File base/scoped_native_library_unittest.cc (right):

https://codereview.chromium.org/32303002/diff/1/base/scoped_native_library_un...
base/scoped_native_library_unittest.cc:39:
GetFunctionPointerFromNativeLibrary(native_library, kFunctionName));
On 2013/10/25 20:13:26, Mark Mentovai wrote:
> Is this safe? You’re expecting native_library to have been freed here (based
on
> the comment above stating that ddraw should not have been loaded to begin
with),
> and thus once unloaded by the ScopedNativeLibrary going out of scope, the
> native_library handle will be invalid.

GetProcAddress() returns NULL when invalid handle is passed. GetLastError()
returns ERROR_MOD_NOT_FOUND in such case.

[Tomasz Moniuszko, 2013-10-28 10:34:56]

https://codereview.chromium.org/32303002/diff/1/base/scoped_native_library_un...
File base/scoped_native_library_unittest.cc (right):

https://codereview.chromium.org/32303002/diff/1/base/scoped_native_library_un...
base/scoped_native_library_unittest.cc:25: const char* kFunctionName =
"DirectDrawCreate";
On 2013/10/25 20:13:26, Mark Mentovai wrote:
> Use const char kFunctionName[] instead.

Done.

[Mark Mentovai, 2013-10-29 22:17:17]

I’m not convinced that it’s safe to use a closed handle this way (the analogous
operation isn’t safe on Linux or Mac OS X). Can we get a Windows expert’s
opinion? Scott, what do you think?

[sky, 2013-10-29 23:10:08]

Carlos knows this better than I.

-sky
+cpu

[cpu_(ooo_6.6-7.5), 2013-10-30 20:08:44]

https://codereview.chromium.org/32303002/diff/70001/base/scoped_native_librar...
File base/scoped_native_library_unittest.cc (right):

https://codereview.chromium.org/32303002/diff/70001/base/scoped_native_librar...
base/scoped_native_library_unittest.cc:27: {
the code here is different, the scope blocks of 26, 32 in the old code is to (I
assume) force the destruction of |library| so it seems we are testing different
cases.

As for removing IsBadCodePtr yes, please, that function is evil.

[Tomasz Moniuszko, 2013-10-31 09:13:37]

https://codereview.chromium.org/32303002/diff/70001/base/scoped_native_librar...
File base/scoped_native_library_unittest.cc (right):

https://codereview.chromium.org/32303002/diff/70001/base/scoped_native_librar...
base/scoped_native_library_unittest.cc:27: {
On 2013/10/30 20:08:45, cpu wrote:
> the code here is different, the scope blocks of 26, 32 in the old code is to
(I
> assume) force the destruction of |library| so it seems we are testing
different
> cases.

Why do you think we are testing different cases? In new code we also have scope
(lines 27 to 37) and |library| is also destructed.

The difference is in the way of checking if library has been really unloaded.
Old code (line 33) is using IsBadCodePtr() function which randomly returns 0
while test expects non-zero return value. New code (lines 38 and 39) tries to
get function pointer from the library which has been unloaded already
(FreeLibrary() was called in ScopedNativeLibrary destructor making
|native_library| handle invalid).

[cpu_(ooo_6.6-7.5), 2013-10-31 16:31:28]

I see it now. lgtm

[Mark Mentovai, 2013-10-31 16:32:00]

base OWNERS LGTM based on Carlos’ LGTM.

[commit-bot: I haz the power, 2013-10-31 16:33:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmoniuszko@opera.com/32303002/70001

[commit-bot: I haz the power, 2013-10-31 20:28:54]

Change committed as 232192