Gin: Make it easier to implement Wrappable

Gin: Make it easier to implement Wrappable.

BUG=

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

[Aaron Boodman, 2013-12-06 06:36:24]

https://codereview.chromium.org/105743007/diff/1/gin/function_template.cc
File gin/function_template.cc (right):

https://codereview.chromium.org/105743007/diff/1/gin/function_template.cc#new...
gin/function_template.cc:13: &WrappableTraits<internal::CallbackHolderBase>::
I'm going to automate function template registration too. Different CL.

https://codereview.chromium.org/105743007/diff/1/gin/wrappable.h
File gin/wrappable.h (left):

https://codereview.chromium.org/105743007/diff/1/gin/wrappable.h#oldcode41
gin/wrappable.h:41: // To customize the wrapper created for a subclass, override
GetWrapperInfo()
I don't get this comment. What would overriding GetWrapperInfo achieve?

[abarth-chromium, 2013-12-06 07:44:25]

LGTM if the compiler will dedup all the WrapperInfo objects to have the same
address.

https://codereview.chromium.org/105743007/diff/20001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/105743007/diff/20001/gin/wrappable.h#newcode22
gin/wrappable.h:22: };
We rely on the address being unique.  Will all the copies in each translation
unit end up with the same address?

[Aaron Boodman, 2013-12-06 22:40:54]

https://codereview.chromium.org/105743007/diff/20001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/105743007/diff/20001/gin/wrappable.h#newcode22
gin/wrappable.h:22: };
On 2013/12/06 07:44:25, abarth wrote:
> We rely on the address being unique.  Will all the copies in each translation
> unit end up with the same address?

I tested this by LOG'ing the address of
WrappableTraits<internal::CallbackHolderBase>::kWrapperInfo from three different
.cc files within the unittest binary, and they all logged the same value (on
Clang/Linux).

However the spec here is suitable confusing that I'm worried, so I added a unit
test that ensures this behavior is consistent across all the platforms we run on
and doesn't change in the future.

PTAL at the unit test change and convince yourself it ensures the correct
behavior.

https://codereview.chromium.org/105743007/diff/40001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/105743007/diff/40001/gin/wrappable.h#newcode74
gin/wrappable.h:74: virtual WrapperInfo* GetWrapperInfo() {
I am kind of grumpy about the extra inheritance and especially this virtual call
which will be on the critical path for every JS->C++ call.

I can fix it, but I guess I'm not clear if that would defeat some security thing
you were trying to do. What exactly is the point again of the
CHECK(result->GetWrapperInfo() == this) in FromV8()? If that fails, it seems
like it indicates one of either:

a) Coding bug on our part.
b) Malicious JavaScript/content.

But why are we taking extra precaution against either of those for this
particular code -- that's a general concern with all content inside the sandbox.

[abarth-chromium, 2013-12-06 23:03:10]

Test LGTM

https://codereview.chromium.org/105743007/diff/40001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/105743007/diff/40001/gin/wrappable.h#newcode74
gin/wrappable.h:74: virtual WrapperInfo* GetWrapperInfo() {
On 2013/12/06 22:40:55, Aaron Boodman wrote:
> What exactly is the point again of the
> CHECK(result->GetWrapperInfo() == this) in FromV8()?

It's a security check to catch a class of use-after-free vulnerabilities.  The
security team added something similar to the Blink bindings.  We can remove it
for now and consider re-adding it in the future once we can measure any
performance impact.

[Aaron Boodman, 2013-12-07 02:58:15]

I asked jyasskin about the previous patch, and he said it actually has the
reverse problem: in MSVC, every one of the template instances, for any T, will
all have the same static data member.

So I had to do something different.

[abarth-chromium, 2013-12-07 03:42:54]

ok

[commit-bot: I haz the power, 2013-12-07 14:27:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/105743007/100001

[commit-bot: I haz the power, 2013-12-08 07:11:02]

Change committed as 239387