Utility functions to interact with the NT loader's data structures and associ...

Utility functions to interact with the NT loader's data structures and associated tests.
This is in preparation for squelching false positive crash reports during DLL load.

BUG=31980
TEST=Unittests in this change.


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

[amit, 2010-03-11 16:08:51]

drive by lgtm

http://codereview.chromium.org/882001/diff/1/3
File chrome_frame/crash_reporting/crash_dll.h (right):

http://codereview.chromium.org/882001/diff/1/3#newcode9
chrome_frame/crash_reporting/crash_dll.h:9: static const wchar_t*
kCrashOnLoadMode = L"CRASH_DLL_CRASH_ON_LOAD";
nit: declspec(SELECTANY) or definitions in .cc?

http://codereview.chromium.org/882001/diff/1/6
File chrome_frame/crash_reporting/nt_loader.cc (right):

http://codereview.chromium.org/882001/diff/1/6#newcode14
chrome_frame/crash_reporting/nt_loader.cc:14:
DCHECK(OwnsCriticalSection(GetLoaderLock()));
Shouldn't this an 'if' instead? We probably do not want to go through the list
if loader lock is not held by us?

[Sigurður Ásgeirsson, 2010-03-11 16:16:35]

Thanks!

http://codereview.chromium.org/882001/diff/1/3
File chrome_frame/crash_reporting/crash_dll.h (right):

http://codereview.chromium.org/882001/diff/1/3#newcode9
chrome_frame/crash_reporting/crash_dll.h:9: static const wchar_t*
kCrashOnLoadMode = L"CRASH_DLL_CRASH_ON_LOAD";
On 2010/03/11 16:08:51, amit wrote:
> nit: declspec(SELECTANY) or definitions in .cc?

The static declaration serves the same purpose as selectany, e.g. the symbol is
duplicated as a non-conflicting static into every compilation unit that includes
the header.

http://codereview.chromium.org/882001/diff/1/6
File chrome_frame/crash_reporting/nt_loader.cc (right):

http://codereview.chromium.org/882001/diff/1/6#newcode14
chrome_frame/crash_reporting/nt_loader.cc:14:
DCHECK(OwnsCriticalSection(GetLoaderLock()));
On 2010/03/11 16:08:51, amit wrote:
> Shouldn't this an 'if' instead? We probably do not want to go through the list
> if loader lock is not held by us?

The DCHECK is asserting on the documented precondition of the function, that the
loader lock must be held on entry. We should never go in here without the lock
held, but if we ever do, I want the *developer* to be tripped immediately.

[amit, 2010-03-11 16:52:11]

On 2010/03/11 16:16:35, Ruðrugis wrote:
> Thanks!
> 
> http://codereview.chromium.org/882001/diff/1/3
> File chrome_frame/crash_reporting/crash_dll.h (right):
> 
> http://codereview.chromium.org/882001/diff/1/3#newcode9
> chrome_frame/crash_reporting/crash_dll.h:9: static const wchar_t*
> kCrashOnLoadMode = L"CRASH_DLL_CRASH_ON_LOAD";
> On 2010/03/11 16:08:51, amit wrote:
> > nit: declspec(SELECTANY) or definitions in .cc?
> 
> The static declaration serves the same purpose as selectany, e.g. the symbol
is
> duplicated as a non-conflicting static into every compilation unit that
includes
> the header.
> 
> http://codereview.chromium.org/882001/diff/1/6
> File chrome_frame/crash_reporting/nt_loader.cc (right):
> 
> http://codereview.chromium.org/882001/diff/1/6#newcode14
> chrome_frame/crash_reporting/nt_loader.cc:14:
> DCHECK(OwnsCriticalSection(GetLoaderLock()));
> On 2010/03/11 16:08:51, amit wrote:
> > Shouldn't this an 'if' instead? We probably do not want to go through the
list
> > if loader lock is not held by us?
> 
> The DCHECK is asserting on the documented precondition of the function, that
the
> loader lock must be held on entry. We should never go in here without the lock
> held, but if we ever do, I want the *developer* to be tripped immediately.

It seems like we are going to use this helper in the field. In that case we
won't this cause deadlock if we walk the module list w/o holding the lock?

[Sigurður Ásgeirsson, 2010-03-11 16:57:58]

On Thu, Mar 11, 2010 at 11:52 AM, <amit@chromium.org> wrote:

> On 2010/03/11 16:16:35, Ruðrugis wrote:
>
>> Thanks!
>>
>
>  http://codereview.chromium.org/882001/diff/1/3
>> File chrome_frame/crash_reporting/crash_dll.h (right):
>>
>
>  http://codereview.chromium.org/882001/diff/1/3#newcode9
>> chrome_frame/crash_reporting/crash_dll.h:9: static const wchar_t*
>> kCrashOnLoadMode = L"CRASH_DLL_CRASH_ON_LOAD";
>> On 2010/03/11 16:08:51, amit wrote:
>> > nit: declspec(SELECTANY) or definitions in .cc?
>>
>
>  The static declaration serves the same purpose as selectany, e.g. the
>> symbol
>>
> is
>
>> duplicated as a non-conflicting static into every compilation unit that
>>
> includes
>
>> the header.
>>
>
>  http://codereview.chromium.org/882001/diff/1/6
>> File chrome_frame/crash_reporting/nt_loader.cc (right):
>>
>
>  http://codereview.chromium.org/882001/diff/1/6#newcode14
>> chrome_frame/crash_reporting/nt_loader.cc:14:
>> DCHECK(OwnsCriticalSection(GetLoaderLock()));
>> On 2010/03/11 16:08:51, amit wrote:
>> > Shouldn't this an 'if' instead? We probably do not want to go through
>> the
>>
> list
>
>> > if loader lock is not held by us?
>>
>
>  The DCHECK is asserting on the documented precondition of the function,
>> that
>>
> the
>
>> loader lock must be held on entry. We should never go in here without the
>> lock
>> held, but if we ever do, I want the *developer* to be tripped immediately.
>>
>
> It seems like we are going to use this helper in the field. In that case we
> won't this cause deadlock if we walk the module list w/o holding the lock?
>
> We might get a crash if anyone calls this without holding the loader's
lock. The conditions under which we'll call this, will however always have
the loader's lock held.

>
> http://codereview.chromium.org/882001
>

[robertshield, 2010-03-11 18:30:38]

LGTM, cool stuff, couple of nits

http://codereview.chromium.org/882001/diff/1/7
File chrome_frame/crash_reporting/nt_loader.h (right):

http://codereview.chromium.org/882001/diff/1/7#newcode137
chrome_frame/crash_reporting/nt_loader.h:137: void* LoadedImports;  // 0x044
tiny nit: indent

http://codereview.chromium.org/882001/diff/1/5
File chrome_frame/crash_reporting/nt_loader_unittest.cc (right):

http://codereview.chromium.org/882001/diff/1/5#newcode70
chrome_frame/crash_reporting/nt_loader_unittest.cc:70:
EXPECT_FALSE(OwnsCriticalSection(&cs));
I believe this should be an ASSERT_FALSE. Reason being, that if we did still own
it, then the other thread would block on the EnterCriticalSection and then we'd
deadlock waiting for the Event to be set.

http://codereview.chromium.org/882001/diff/1/5#newcode87
chrome_frame/crash_reporting/nt_loader_unittest.cc:87: EXPECT_EQ(0,
::TryEnterCriticalSection(&cs));
ASSERT here too? should we Leave it if this succeeds or does that not matter
terribly much?

http://codereview.chromium.org/882001/diff/1/5#newcode281
chrome_frame/crash_reporting/nt_loader_unittest.cc:281: EXPECT_EQ(1,
exceptions_handled);
cool tests :)

[Sigurður Ásgeirsson, 2010-03-11 18:40:21]

Thanks.

http://codereview.chromium.org/882001/diff/1/7
File chrome_frame/crash_reporting/nt_loader.h (right):

http://codereview.chromium.org/882001/diff/1/7#newcode137
chrome_frame/crash_reporting/nt_loader.h:137: void* LoadedImports;  // 0x044
On 2010/03/11 18:30:38, robertshield wrote:
> tiny nit: indent

ah, you have a good eye for indent. Will fix in an upcoming CL  as I submitted
this one on Amit's LGTM.

http://codereview.chromium.org/882001/diff/1/5
File chrome_frame/crash_reporting/nt_loader_unittest.cc (right):

http://codereview.chromium.org/882001/diff/1/5#newcode70
chrome_frame/crash_reporting/nt_loader_unittest.cc:70:
EXPECT_FALSE(OwnsCriticalSection(&cs));
On 2010/03/11 18:30:38, robertshield wrote:
> I believe this should be an ASSERT_FALSE. Reason being, that if we did still
own
> it, then the other thread would block on the EnterCriticalSection and then
we'd
> deadlock waiting for the Event to be set.

Well, the ownership of the CS is not in question here, that's quite well-defined
so it doesn't really matter whether I assert or expect, except for the little
detail that the CS will be orphaned on early exit.
What is in question here is the functionality of the OwnsCriticalSection
function, which is being tested against a well defined set of states on a CS
under test control.

http://codereview.chromium.org/882001/diff/1/5#newcode87
chrome_frame/crash_reporting/nt_loader_unittest.cc:87: EXPECT_EQ(0,
::TryEnterCriticalSection(&cs));
On 2010/03/11 18:30:38, robertshield wrote:
> ASSERT here too? should we Leave it if this succeeds or does that not matter
> terribly much?

Yeah, same deal, this is more to verify that I didn't screw up on the
posttasks...

http://codereview.chromium.org/882001/diff/1/5#newcode281
chrome_frame/crash_reporting/nt_loader_unittest.cc:281: EXPECT_EQ(1,
exceptions_handled);
On 2010/03/11 18:30:38, robertshield wrote:
> cool tests :)

Thanks, but the CRT bug is totally uncool, and it cost me a couple of hours to
work around it :(.