Try a new approach to fixing IAT unpatch crashes when the DLL is gone.

Try a new approach to fixing IAT unpatch crashes when the DLL is gone.

Have the IAT patcher take some "ownership" of the DLL, by taking a library name and then calling LoadLibrary() / FreeLibrary() to manage the reference count.  This means as long is there isn't some other reference count balancing bug happening in the process, the DLL will never be unloaded while we are patched.

This effectively reverts r9929, the VirtualQuery additional checks are removed.

BUG=7701

[Dean McNamee, 2009-02-18 16:24:57]

I like this approach, and it also simplified both callers, where they were
handling the HMODULE only for the patcher code.  We have to be a bit careful to
not allow the static destruction sequence to try to unload, since I imagine
FreeLibrary() while holding the loader lock (static destruction probably happens
from DllMain) is not the greatest idea.  I modified both callers so that
hopefully that will not be the case, they should be torn down at AtExitManager
shutdown, which is happening from a return form main.

We could do some friendship / private constructor thing with iat_patcher so that
it's only used with a LazyInstance, or make it inherently lazy or something. 
Probably not worth it for now, there are only two callers...

PS:  I didn't even try to compile this code yet.

[amit, 2009-02-18 16:50:49]

http://codereview.chromium.org/21453/diff/1010/6
File base/iat_patch.cc (right):

http://codereview.chromium.org/21453/diff/1010/6#newcode192
Line 192: HMODULE module_handle = LoadLibraryW(module);
Holds a reference on the DLL and will affect when the DLL gets unloaded.
Ideally, patching code should not interfere with the lifetime of a DLL.

http://codereview.chromium.org/21453/diff/1010/8
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/21453/diff/1010/8#newcode714
Line 714: ++patcher->refcount;
How about moving this logic inside a PaintPatchState::Initialize(...)?

http://codereview.chromium.org/21453/diff/1010/8#newcode813
Line 813: }
PaintPatchState::Uninitialize(...) or maybe something better named.

[amit, 2009-02-18 17:39:15]

LGTM.

Also, go ahead and BeginPaintIntercept and EndPaintIntercept public :)

[Dean McNamee, 2009-02-26 14:02:28]

Hey Amit,

I know you said LG, but I made a few bigger changes so I wanted you to look
again.

I moved Begin/EndPaintIntercept completely out of the class (it didn't need the
access for anything).  Right now they are just floating, but now I am thinking
that I could have made them static methods on the patcher, not a big deal either
way.

Let me know what you think.

On 2009/02/18 17:39:15, amit wrote:
> LGTM.
> 
> Also, go ahead and BeginPaintIntercept and EndPaintIntercept public :)

[amit, 2009-02-26 15:00:32]

Looks good!

Since we now patch/unpatch more frequently, a test case where an autocomplete
view is created/destroyed and we patch unpatch multiple times would be nice.

http://codereview.chromium.org/21453/diff/2009/2012
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/21453/diff/2009/2012#newcode673
Line 673: class PaintPatcher {
maybe derive privately from RefCounted and just call AddRef from RefPatch()?

http://codereview.chromium.org/21453/diff/2009/2012#newcode700
Line 700: private:
DISABLE_COPY_AND_ASSIGN