Reimplementation of FunctionStub, to avoid rewriting potentially executing co...

Reimplementation of FunctionStub to avoid rewriting potentially executing code for a slight improvement in thread safety.
Make VTABLE patching treadsafe to the extent possible. As-is it's now safe against itself running on other threads at lease, as well as against other similar implementations, though the inherent VM operation race is resolved by retrying.

BUG=27415
TEST=Included unittests.


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

[tommi (sloooow) - chröme, 2010-03-17 17:49:20]

looks good so far.  I like the tests too

http://codereview.chromium.org/992008/diff/1/4
File chrome_frame/function_stub.cc (right):

http://codereview.chromium.org/992008/diff/1/4#newcode24
chrome_frame/function_stub.cc:24: class ExecutableHeap {
nice!

http://codereview.chromium.org/992008/diff/1/4#newcode85
chrome_frame/function_stub.cc:85: void FunctionStub::Init(FunctionStubAsm* stub)
{
DCHECK(stub != NULL)?
I guess it would crash pretty fast anyways

[Sigurður Ásgeirsson, 2010-03-17 19:06:25]

Thanks, added implementation of FromCode + tests and addressed your comments.

http://codereview.chromium.org/992008/diff/1/4
File chrome_frame/function_stub.cc (right):

http://codereview.chromium.org/992008/diff/1/4#newcode24
chrome_frame/function_stub.cc:24: class ExecutableHeap {
On 2010/03/17 17:49:20, tommi wrote:
> nice!

Thanks.

http://codereview.chromium.org/992008/diff/1/4#newcode85
chrome_frame/function_stub.cc:85: void FunctionStub::Init(FunctionStubAsm* stub)
{
On 2010/03/17 17:49:20, tommi wrote:
> DCHECK(stub != NULL)?
> I guess it would crash pretty fast anyways

Done.

[tommi (sloooow) - chröme, 2010-03-17 19:43:48]

lgtm

http://codereview.chromium.org/992008/diff/8001/9003
File chrome_frame/function_stub.cc (right):

http://codereview.chromium.org/992008/diff/8001/9003#newcode124
chrome_frame/function_stub.cc:124: return candidate;
nit: use {} if condition spans more than one line

http://codereview.chromium.org/992008/diff/8001/9001
File chrome_frame/function_stub.h (right):

http://codereview.chromium.org/992008/diff/8001/9001#newcode98
chrome_frame/function_stub.h:98: // @note likely means approximately 1/2^48
here.
:)

http://codereview.chromium.org/992008/diff/8001/9002
File chrome_frame/function_stub_unittest.cc (right):

http://codereview.chromium.org/992008/diff/8001/9002#newcode57
chrome_frame/function_stub_unittest.cc:57:
EXPECT_TRUE(FunctionStub::Destroy(stub_));
I don't trust macros enough (they could have more than one statement).  do you
mind using {}?

http://codereview.chromium.org/992008/diff/8001/9002#newcode60
chrome_frame/function_stub_unittest.cc:60: EXPECT_TRUE(::VirtualFree(playpen_,
0, MEM_RELEASE));
and here

[Sigurður Ásgeirsson, 2010-03-19 17:26:59]

Thanks, please take another look. 
I've added the VTable patch manager changes to this CL, as they're somewhat
co-dependent.

http://codereview.chromium.org/992008/diff/8001/9003
File chrome_frame/function_stub.cc (right):

http://codereview.chromium.org/992008/diff/8001/9003#newcode124
chrome_frame/function_stub.cc:124: return candidate;
On 2010/03/17 19:43:48, tommi wrote:
> nit: use {} if condition spans more than one line

Done.

http://codereview.chromium.org/992008/diff/8001/9001
File chrome_frame/function_stub.h (right):

http://codereview.chromium.org/992008/diff/8001/9001#newcode98
chrome_frame/function_stub.h:98: // @note likely means approximately 1/2^48
here.
On 2010/03/17 19:43:48, tommi wrote:
> :)

indeed :).

http://codereview.chromium.org/992008/diff/8001/9002
File chrome_frame/function_stub_unittest.cc (right):

http://codereview.chromium.org/992008/diff/8001/9002#newcode57
chrome_frame/function_stub_unittest.cc:57:
EXPECT_TRUE(FunctionStub::Destroy(stub_));
On 2010/03/17 19:43:48, tommi wrote:
> I don't trust macros enough (they could have more than one statement).  do you
> mind using {}?

Done.

http://codereview.chromium.org/992008/diff/8001/9002#newcode60
chrome_frame/function_stub_unittest.cc:60: EXPECT_TRUE(::VirtualFree(playpen_,
0, MEM_RELEASE));
On 2010/03/17 19:43:48, tommi wrote:
> and here

Done.

[tommi (sloooow) - chröme, 2010-03-19 17:41:36]

lgtm

http://codereview.chromium.org/992008/diff/15001/16007
File chrome_frame/vtable_patch_manager.cc (right):

http://codereview.chromium.org/992008/diff/15001/16007#newcode47
chrome_frame/vtable_patch_manager.cc:47: return false;
should we perhaps return an error value that can have more than one state?
E.g. HRESULT, S_OK, S_FALSE, E_ACCESSVIOLATION (or something that makes sense)

http://codereview.chromium.org/992008/diff/15001/16007#newcode117
chrome_frame/vtable_patch_manager.cc:117: HRESULT hr =
HRESULT_FROM_WIN32(::GetLastError());
nit: could use AtlHresultFromLastError to avoid calling GetLastError twice.

http://codereview.chromium.org/992008/diff/15001/16007#newcode128
chrome_frame/vtable_patch_manager.cc:128: stub->code(),
indent feels a bit strange

[Sigurður Ásgeirsson, 2010-03-22 20:13:49]

Thanks, please take one last look.

I found I needed to add a couple of project dependencies, and I forgot to
FlushInstructionCache. I did remember to wash my hands, however :).

http://codereview.chromium.org/992008/diff/15001/16007
File chrome_frame/vtable_patch_manager.cc (right):

http://codereview.chromium.org/992008/diff/15001/16007#newcode47
chrome_frame/vtable_patch_manager.cc:47: return false;
On 2010/03/19 17:41:36, tommi wrote:
> should we perhaps return an error value that can have more than one state?
> E.g. HRESULT, S_OK, S_FALSE, E_ACCESSVIOLATION (or something that makes sense)

I don't see that it matters to the user whether we've lost a VM operation race
(resulting in access violation), or a data modification race here.

http://codereview.chromium.org/992008/diff/15001/16007#newcode117
chrome_frame/vtable_patch_manager.cc:117: HRESULT hr =
HRESULT_FROM_WIN32(::GetLastError());
On 2010/03/19 17:41:36, tommi wrote:
> nit: could use AtlHresultFromLastError to avoid calling GetLastError twice.

Done.

http://codereview.chromium.org/992008/diff/15001/16007#newcode128
chrome_frame/vtable_patch_manager.cc:128: stub->code(),
On 2010/03/19 17:41:36, tommi wrote:
> indent feels a bit strange

Done.

[tommi (sloooow) - chröme, 2010-03-22 23:11:51]

lgtm