Reland: Reland: Implement new Blink IDL attribute [SetReference]

Reland: Reland: Implement new Blink IDL attribute [SetReference]

SetReference is a new Blink IDL attribute which can be used to specify dependency from a wrapper to another. This is implemented by calling v8::Isolate::SetReference on minor GC prologues. This has a lower overhead compared to old technique using SetHiddenValueTo.

This patch also modifies DOMDataStore::Current to support call outside any active context.

[Reland] Marked NodeFilter dependent.

BUG=307341
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161018

[kouhei (in TOK), 2013-10-10 05:12:14]

haraken: PTAL@ Source/bindings/v8/custom/V8NodeIteratorCustom.cpp

Does this make sense to you?

Obviously I need to this in cleaner way, so I plan to have codegen generate a
toV8Persistent which basically resembles toV8 but returns const v8::Persistent&
to the wrapper instead.

[haraken, 2013-10-10 06:08:32]

The approach looks good.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/scripts/code_...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/26792002/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:117: use Data::Dumper;

Remove this.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/ScriptWrap...
File Source/bindings/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/ScriptWrap...
Source/bindings/v8/ScriptWrappable.h:153: public://private:

Fix this.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/V8GCContro...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:242: v8::Persistent<v8::Object>*
wrapperpers = reinterpret_cast<v8::Persistent<v8::Object>*>(value);

You can write this just before you use |wrapperpers|.

Also, please rename |wrapperpers| to something more descriptive.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:289: m_isolate->SetObjectGroupId(*value,
v8::UniqueId(reinterpret_cast<intptr_t>(root)));

This is a bit confusing. resolveWrapperReachability adds some reachability and
returns an opaque GC root. Then we're doing object grouping outside
resolveWrapperReachability.

Ideally I hope all reachability are handled in resolveWrapperReachability.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/WrapperTyp...
File Source/bindings/v8/WrapperTypeInfo.h (right):

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/WrapperTyp...
Source/bindings/v8/WrapperTypeInfo.h:63: typedef void*
(*ResolveWrapperReachabilityFunction)(void*, const v8::Persistent<v8::Object>&
wrapper, v8::Isolate*);

ResolveWrapperReachabilityFunction => AddWrapperReachabilityFunction ?

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/WrapperTyp...
Source/bindings/v8/WrapperTypeInfo.h:125: void* resolveWrapperReachability(void*
object, const v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)

resolveWrapperReachability => addWrapperReachability ?

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/custom/V8N...
File Source/bindings/v8/custom/V8NodeIteratorCustom.cpp (right):

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/custom/V8N...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:48:
isolate->SetReference(wrapper,
*reinterpret_cast<v8::Persistent<v8::Value>*>(filter->unsafePersistent().persistent()));

 
*reinterpret_cast<v8::Persistent<v8::Value>*>(filter->unsafePersistent().persistent())

This is ugly. Can't we make it simpler?

[kouhei (in TOK), 2013-10-10 06:20:02]

This time with toV8Persistent impl. I'm done with the cpp code, so now I'm going
to generate this by codegen.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/ScriptWrap...
File Source/bindings/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/ScriptWrap...
Source/bindings/v8/ScriptWrappable.h:153: public://private:
On 2013/10/10 06:08:32, haraken wrote:
> 
> Fix this.

Done.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/WrapperTyp...
File Source/bindings/v8/WrapperTypeInfo.h (right):

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/WrapperTyp...
Source/bindings/v8/WrapperTypeInfo.h:63: typedef void*
(*ResolveWrapperReachabilityFunction)(void*, const v8::Persistent<v8::Object>&
wrapper, v8::Isolate*);
On 2013/10/10 06:08:32, haraken wrote:
> 
> ResolveWrapperReachabilityFunction => AddWrapperReachabilityFunction ?

Ack. Will do this later.

https://codereview.chromium.org/26792002/diff/1/Source/bindings/v8/WrapperTyp...
Source/bindings/v8/WrapperTypeInfo.h:125: void* resolveWrapperReachability(void*
object, const v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)
On 2013/10/10 06:08:32, haraken wrote:
> 
> resolveWrapperReachability => addWrapperReachability ?

Ack. Will do this later.

[kouhei (in TOK), 2013-10-15 04:45:39]

haraken: PTAL. I added v8codegen impl.

[jochen (gone - plz use gerrit), 2013-10-15 05:26:42]

https://codereview.chromium.org/26792002/diff/15001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/26792002/diff/15001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:917: inline
UnsafePersistent<v8::Object> toV8Persistent(${nativeType}* impl,
v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)
we shouldn't introduce code using UnsafePersistent

what about returning a local and creating a persistent again from it when we
need it?

[haraken, 2013-10-15 05:31:58]

- Looks like the diff in the patch set 6 is broken. Would you reupload?

- Is the patch set 6 still a WIP? Or are you going to land it?

[kouhei (in TOK), 2013-10-15 05:35:22]

On 2013/10/15 05:26:42, jochen wrote:
>
https://codereview.chromium.org/26792002/diff/15001/Source/bindings/scripts/c...
> File Source/bindings/scripts/code_generator_v8.pm (right):
> 
>
https://codereview.chromium.org/26792002/diff/15001/Source/bindings/scripts/c...
> Source/bindings/scripts/code_generator_v8.pm:917: inline
> UnsafePersistent<v8::Object> toV8Persistent(${nativeType}* impl,
> v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)
> we shouldn't introduce code using UnsafePersistent
> 
> what about returning a local and creating a persistent again from it when we
> need it?

This is to be called every time in minor gc prologue, so I think creating a
"temporary persistent" will work, but will be too slow.

[kouhei (in TOK), 2013-10-15 05:35:47]

On 2013/10/15 05:31:58, haraken wrote:
> - Looks like the diff in the patch set 6 is broken. Would you reupload?
> 
> - Is the patch set 6 still a WIP? Or are you going to land it?

Re-uploaded. I'd like to land this patch.

[haraken, 2013-10-15 05:48:28]

Would you add a test case for [ReachableTo] to TestInterface.idl?

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:457: sub
NeedResolveWrapperReachability

Nit: NeedResolveWrapperReachability => NeedsResolveWrapperReachability

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:490: isolate->SetReference(wrapper,
*reinterpret_cast<v8::Persistent<v8::Value>*>(target.persistent()));

I agree with jochen that using UnsafePersistent is ugly. On the other hand, as
kouhei mentioned, we should avoid the overhead of recreating persistent handles.

jochen, marja: do you have any better idea?

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:917: inline
UnsafePersistent<v8::Object> toV8Persistent(${nativeType}* impl,
v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)

I'm afraid that generating this method to all header files will increase the
binary size unacceptably.

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:927: wrapperPersistent =
DOMDataStore::getWrapperPersistent<${v8ClassName}>(impl, isolate);

Why can't you write:

  wrapperPersistent = wrap(impl, creationContext, isolate);

?

Either way, we should invent a better way and avoid toV8Persistent and
getWrapperPersistent.

[kouhei (in TOK), 2013-10-15 05:54:10]

> Would you add a test case for [ReachableTo] to TestInterface.idl?
Ack.

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:457: sub
NeedResolveWrapperReachability
On 2013/10/15 05:48:29, haraken wrote:
> 
> Nit: NeedResolveWrapperReachability => NeedsResolveWrapperReachability

Ack.

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:917: inline
UnsafePersistent<v8::Object> toV8Persistent(${nativeType}* impl,
v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)
On 2013/10/15 05:48:29, haraken wrote:
> 
> I'm afraid that generating this method to all header files will increase the
> binary size unacceptably.

Wouldn't this be an inline?

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:927: wrapperPersistent =
DOMDataStore::getWrapperPersistent<${v8ClassName}>(impl, isolate);
On 2013/10/15 05:48:29, haraken wrote:
> 
> Why can't you write:
> 
>   wrapperPersistent = wrap(impl, creationContext, isolate);
> 
> ?
> 
> Either way, we should invent a better way and avoid toV8Persistent and
> getWrapperPersistent.

Because "wrap" returns Local.

[kouhei (in TOK), 2013-10-15 06:03:08]

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/26792002/diff/49001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:457: sub
NeedResolveWrapperReachability
On 2013/10/15 05:54:11, kouhei wrote:
> On 2013/10/15 05:48:29, haraken wrote:
> > 
> > Nit: NeedResolveWrapperReachability => NeedsResolveWrapperReachability
> 
> Ack.

Done.

[kouhei (in TOK), 2013-10-15 06:07:32]

> I agree with jochen that using UnsafePersistent is ugly. On the other hand, as
> kouhei mentioned, we should avoid the overhead of recreating persistent
handles.
> jochen, marja: do you have any better idea?
One possible refactor here is to hide the implementation details in
V8DOMDataStore.
getWrapperPersistent-ish method will not return UnsafePersistent, but will also
call SetReference there.

> Would you add a test case for [ReachableTo] to TestInterface.idl?
Done.

[marja, 2013-10-15 13:22:51]

On 2013/10/15 06:07:32, kouhei wrote:
> > I agree with jochen that using UnsafePersistent is ugly. On the other hand,
as
> > kouhei mentioned, we should avoid the overhead of recreating persistent
> handles.
> > jochen, marja: do you have any better idea?
> One possible refactor here is to hide the implementation details in
> V8DOMDataStore.
> getWrapperPersistent-ish method will not return UnsafePersistent, but will
also
> call SetReference there.

The long term plan is to have move semantics for Persistent, and store them
directly in the map (instead of UnsafePersistent), and at this point we wouldn't
have this problem afaict.

One possible option would be to have the "set reference" function in
UnsafePersistent, and then route through DOMDataStore and DOMWrapperMap, but
it's ugly. :( (Maybe still better than passing UnsafePersistents around.)

[marja, 2013-10-15 13:26:56]

Another comment: The name "ReachableTo" doesn't make sense to me.
"ReachableFrom" does, but this one not.

"A ReachableFrom B" means from B, we can reach A, but "A ReachableTo B" means...
what? The opposite of ReachableFrom? That from A, we can reach B? Can it be
called "CanReach"?

[kouhei (in TOK), 2013-10-16 04:10:50]

On 2013/10/15 13:22:51, marja wrote:
> On 2013/10/15 06:07:32, kouhei wrote:
> > > I agree with jochen that using UnsafePersistent is ugly. On the other
hand,
> as
> > > kouhei mentioned, we should avoid the overhead of recreating persistent
> > handles.
> > > jochen, marja: do you have any better idea?
> > One possible refactor here is to hide the implementation details in
> > V8DOMDataStore.
> > getWrapperPersistent-ish method will not return UnsafePersistent, but will
> also
> > call SetReference there.
> 
> The long term plan is to have move semantics for Persistent, and store them
> directly in the map (instead of UnsafePersistent), and at this point we
wouldn't
> have this problem afaict.
> 
> One possible option would be to have the "set reference" function in
> UnsafePersistent, and then route through DOMDataStore and DOMWrapperMap, but
> it's ugly. :( (Maybe still better than passing UnsafePersistents around.)
Updated patch. I made DOMDataStore::setWrapperReferenceFrom to avoid exposing
UnsafePersistent.

> Another comment: The name "ReachableTo" doesn't make sense to me.
> "ReachableFrom" does, but this one not.
> "A ReachableFrom B" means from B, we can reach A, but "A ReachableTo B"
means...
> what? The opposite of ReachableFrom? That from A, we can reach B? Can it be
> called "CanReach"?

I understand that "CanReach" sounds more natural, but I plan to implement
[ReachableFrom] after this and want to have synmmetric names for them.

haraken: PTAL

[haraken, 2013-10-16 04:46:37]

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:490: if(!
DOMDataStore::containsWrapper<${reachableToV8Type}>(${reachableToName},
isolate))

Nit: Unnecessary space after '!'.

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestInterface.cpp:939: setObjectGroup(object,
wrapper, isolate);

It looks inconsistent that setObjectGroup is a method of WrapperTypeInfo whereas
setWrapperReferenceFrom is a method of DOMDataStore.

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMDat...
File Source/bindings/v8/DOMDataStore.h (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMDat...
Source/bindings/v8/DOMDataStore.h:144: inline void setReferenceFrom(const
v8::Persistent<v8::Object>& parent, T* child, v8::Isolate* isolate)

hmm, the implementation of setReferenceFrom looks ugly.

- setReferenceFrom takes a C++ pointer in the second argument. In terms of
semantics, setReferenceFrom should take two Persistent handles.

- We need to implement setReferenceFrom in UnsafePersistent and DOMWrapperMap as
well.

Instead of encapsulating all persistent handle operations in DOMDataStore, can
we implement a method in DOMDataStore that returns a reference of Persistent
handle? Then we can pass the Persistent reference around in generated code.

[haraken, 2013-10-16 05:37:30]

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMDat...
File Source/bindings/v8/DOMDataStore.h (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMDat...
Source/bindings/v8/DOMDataStore.h:144: inline void setReferenceFrom(const
v8::Persistent<v8::Object>& parent, T* child, v8::Isolate* isolate)
On 2013/10/16 04:46:38, haraken wrote:
> 
> hmm, the implementation of setReferenceFrom looks ugly.
> 
> - setReferenceFrom takes a C++ pointer in the second argument. In terms of
> semantics, setReferenceFrom should take two Persistent handles.
> 
> - We need to implement setReferenceFrom in UnsafePersistent and DOMWrapperMap
as
> well.
> 
> Instead of encapsulating all persistent handle operations in DOMDataStore, can
> we implement a method in DOMDataStore that returns a reference of Persistent
> handle? Then we can pass the Persistent reference around in generated code.

Discussed offline with kouhei-san. I understand that these ugly methods are
necessary to encapsulate the usage of UnsafePersistents into DOMDataStore. Even
worse, we will also add setReferenceTo() when implementing [ReferenceFrom].

marja, dan, jochen: If we have unique pointers for Persistents, we can get rid
of all the ugliness. I thought you were saying that you have a plan to implement
Unique handles, but what's the current status? Another solution might be to
implement OwnPersistent in the Blink side. (I'm OK with both ideas.)

[marja, 2013-10-16 15:43:52]

On 2013/10/16 04:10:50, kouhei wrote:
> I understand that "CanReach" sounds more natural, but I plan to implement
> [ReachableFrom] after this and want to have synmmetric names for them.

If you want symmetry, you could combine symmetry and sensemaking by choosing
"CanReach" and "CanBeReachedFrom". :) Or maybe you could ask a native English
speaker if "ReachableTo" makes sense to them; I'm not sure if it's just me or if
it doesn't make sense in general.

[marja, 2013-10-16 16:06:27]

On 2013/10/16 05:37:30, haraken wrote:
> marja, dan, jochen: If we have unique pointers for Persistents, we can get rid
> of all the ugliness. I thought you were saying that you have a plan to
implement
> Unique handles, but what's the current status? Another solution might be to
> implement OwnPersistent in the Blink side. (I'm OK with both ideas.)

I don't know the current status (i.e., what Dan had in mind for this feature),
sorry :/ Dan's back next week.

[marja, 2013-10-16 16:09:13]

Also, fyi, the templatized SetReference et al:
https://codereview.chromium.org/27512003/

[haraken, 2013-10-17 00:47:30]

Thanks marja. Given it will take time to implement Unique handles in v8 (or
OwnPersistent in the Blink side), I'm fine with the current approach in this CL.

I'll LG if all comments marja and I have added above are resolved.

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMWra...
File Source/bindings/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMWra...
Source/bindings/v8/DOMWrapperMap.h:68: m_map.get(key).setReferenceFrom(parent,
isolate);

It looks OK to use UnsafePersistent in DOMWrapperMap. Shall we use an
UnsafePersistent here and remove UnsafePersistent::setReferenceFrom?

Also, shall we rename the method to setReference"To" ? This is a method used for
[ReachableTo].

[kouhei (in TOK), 2013-10-17 02:30:08]

Thanks marja! I'll remove the casts once its landed.

I renamed [ReachableTo] to [SetReference] in this patch. I think it is more
obvious that it ends up calling Isolate::SetReference.

haraken: PTAL.

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:490: if(!
DOMDataStore::containsWrapper<${reachableToV8Type}>(${reachableToName},
isolate))
On 2013/10/16 04:46:38, haraken wrote:
> 
> Nit: Unnecessary space after '!'.

Done.

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestInterface.cpp:939: setObjectGroup(object,
wrapper, isolate);
On 2013/10/16 04:46:38, haraken wrote:
> 
> It looks inconsistent that setObjectGroup is a method of WrapperTypeInfo
whereas
> setWrapperReferenceFrom is a method of DOMDataStore.

setObjectGroup is not a method of WrapperTypeInfo. It's a WebCore:: global
function. I placed it here because it doesn't touch wrapper internals and
because I need to use it as a default implementation of
resolveWrapperReachability in WrapperTypeInfo.h.

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMWra...
File Source/bindings/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/v8/DOMWra...
Source/bindings/v8/DOMWrapperMap.h:68: m_map.get(key).setReferenceFrom(parent,
isolate);
On 2013/10/17 00:47:31, haraken wrote:
> 
> It looks OK to use UnsafePersistent in DOMWrapperMap. Shall we use an
> UnsafePersistent here and remove UnsafePersistent::setReferenceFrom?
> 
> Also, shall we rename the method to setReference"To" ? This is a method used
for
> [ReachableTo].

Discussed offline.

This is also called from DOMWrapperMap. I think it is better not to expose
persistent() anymore.

[haraken, 2013-10-17 03:28:21]

LGTM. Please wait for marja's approval.

Looking at this CL, I'm pretty sure it's time to implement Unique handles or
OwnPersistents.

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/26792002/diff/85001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestInterface.cpp:939: setObjectGroup(object,
wrapper, isolate);
On 2013/10/17 02:30:09, kouhei wrote:
> On 2013/10/16 04:46:38, haraken wrote:
> > 
> > It looks inconsistent that setObjectGroup is a method of WrapperTypeInfo
> whereas
> > setWrapperReferenceFrom is a method of DOMDataStore.
> 
> setObjectGroup is not a method of WrapperTypeInfo. It's a WebCore:: global
> function. I placed it here because it doesn't touch wrapper internals and
> because I need to use it as a default implementation of
> resolveWrapperReachability in WrapperTypeInfo.h.

I still think these methods are misplaced, but it's OK for now. We have to allow
the mis-placement in order to encapsulate UnsafePersistent.

Let's clean up these methods when removing UnsafePersistent.

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/tests/idl...
File Source/bindings/tests/idls/TestInterface.idl (right):

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestInterface.idl:37: SetReference(ReferencedType
ReferencedName)

ReferencedName => referencedName

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/v8/V8GCCo...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/v8/V8GCCo...
Source/bindings/v8/V8GCController.cpp:232: virtual void
VisitPersistentHandle(v8::Persistent<v8::Value>* value, uint16_t classId)
OVERRIDE

Not related to your CL, I think the type of |value| should
v8::Persistent<v8::Object>. A V8 side fix would be necessary to fix it.

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/v8/Wrappe...
File Source/bindings/v8/WrapperTypeInfo.h (right):

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/v8/Wrappe...
Source/bindings/v8/WrapperTypeInfo.h:63: typedef void
(*ResolveWrapperReachabilityFunction)(void*, const v8::Persistent<v8::Object>&
wrapper, v8::Isolate*);

|wrapper| is not needed.

[kouhei (in TOK), 2013-10-17 05:17:22]

haraken: Thanks for review!
marja: Would you take a look?

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/tests/idl...
File Source/bindings/tests/idls/TestInterface.idl (right):

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestInterface.idl:37: SetReference(ReferencedType
ReferencedName)
On 2013/10/17 03:28:22, haraken wrote:
> 
> ReferencedName => referencedName

Done.

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/v8/Wrappe...
File Source/bindings/v8/WrapperTypeInfo.h (right):

https://codereview.chromium.org/26792002/diff/98001/Source/bindings/v8/Wrappe...
Source/bindings/v8/WrapperTypeInfo.h:63: typedef void
(*ResolveWrapperReachabilityFunction)(void*, const v8::Persistent<v8::Object>&
wrapper, v8::Isolate*);
On 2013/10/17 03:28:22, haraken wrote:
> 
> |wrapper| is not needed.

Done.

[marja, 2013-10-17 07:59:46]

lgtm

[kouhei (in TOK), 2013-10-17 08:04:36]

On 2013/10/17 07:59:46, marja wrote:
> lgtm

Thanks for the review!

[commit-bot: I haz the power, 2013-10-17 08:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26792002/106001

[commit-bot: I haz the power, 2013-10-17 08:18:14]

Failed to apply patch for Source/bindings/scripts/code_generator_v8.pm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/scripts/code_generator_v8.pm
  Hunk #5 FAILED at 4073.
  Hunk #7 succeeded at 4928 (offset -12 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
Source/bindings/scripts/code_generator_v8.pm.rej

Patch:       Source/bindings/scripts/code_generator_v8.pm
Index: Source/bindings/scripts/code_generator_v8.pm
diff --git a/Source/bindings/scripts/code_generator_v8.pm
b/Source/bindings/scripts/code_generator_v8.pm
index
886ed7d7003417adba21f53f45d0ac51515a35f9..18a9e496c1bd0b84afb42a8c3cfba0a134705fd5
100644
--- a/Source/bindings/scripts/code_generator_v8.pm
+++ b/Source/bindings/scripts/code_generator_v8.pm
@@ -454,13 +454,13 @@ sub HeaderFilesForInterface
     return @includes;
 }
 
-sub NeedsOpaqueRootForGC
+sub NeedsResolveWrapperReachability
 {
     my $interface = shift;
-    return $interface->extendedAttributes->{"GenerateIsReachable"} ||
$interface->extendedAttributes->{"CustomIsReachable"};
+    return $interface->extendedAttributes->{"GenerateIsReachable"} ||
$interface->extendedAttributes->{"CustomIsReachable"} ||
$interface->extendedAttributes->{"SetReference"};
 }
 
-sub GenerateOpaqueRootForGC
+sub GenerateResolveWrapperReachability
 {
     my $interface = shift;
     my $implClassName = GetImplName($interface);
@@ -471,22 +471,43 @@ sub GenerateOpaqueRootForGC
     }
 
     my $code = <<END;
-void* ${v8ClassName}::opaqueRootForGC(void* object, v8::Isolate* isolate)
+void ${v8ClassName}::resolveWrapperReachability(void* object, const
v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)
 {
     ${implClassName}* impl = fromInternalPointer(object);
 END
+    if ($interface->extendedAttributes->{"SetReference"}) {
+        $code .= "    v8::Local<v8::Object> creationContext =
v8::Local<v8::Object>::New(isolate, wrapper);\n";
+    }
+    for my $setReference (@{$interface->extendedAttributes->{"SetReference"}})
{
+        my $setReferenceType = $setReference->type;
+        my $setReferenceName = $setReference->name;
+        my $setReferenceV8Type = "V8".$setReferenceType;
+
+        AddIncludesForType($setReferenceType);
+        $code .= <<END;
+    ${setReferenceType}* ${setReferenceName} = impl->${setReferenceName}();
+    if (${setReferenceName}) {
+       
if(!DOMDataStore::containsWrapper<${setReferenceV8Type}>(${setReferenceName},
isolate))
+            wrap(${setReferenceName}, creationContext, isolate);
+        DOMDataStore::setWrapperReference<${setReferenceV8Type}>(wrapper,
${setReferenceName}, isolate);
+    }
+END
+    }
+
     my $isReachableMethod =
$interface->extendedAttributes->{"GenerateIsReachable"};
     if ($isReachableMethod) {
         AddToImplIncludes("bindings/v8/V8GCController.h");
         AddToImplIncludes("core/dom/Element.h");
         $code .= <<END;
-    if (Node* owner = impl->${isReachableMethod}())
-        return V8GCController::opaqueRootForGC(owner, isolate);
+    if (Node* owner = impl->${isReachableMethod}()) {
+        setObjectGroup(V8GCController::opaqueRootForGC(owner, isolate),
wrapper, isolate);
+        return;
+    }
 END
     }
 
     $code .= <<END;
-    return object;
+    setObjectGroup(object, wrapper, isolate);
 }
 
 END
@@ -669,8 +690,8 @@ END
     static WrapperTypeInfo info;
 END
 
-    if (NeedsOpaqueRootForGC($interface)) {
-        $header{classPublic}->add("    static void* opaqueRootForGC(void*,
v8::Isolate*);\n");
+    if (NeedsResolveWrapperReachability($interface)) {
+        $header{classPublic}->add("    static void
resolveWrapperReachability(void*, const v8::Persistent<v8::Object>&,
v8::Isolate*);\n");
     }
 
     if (InheritsExtendedAttribute($interface, "ActiveDOMObject")) {
@@ -4002,7 +4023,7 @@ sub GenerateImplementation
 
     my $toActiveDOMObject = InheritsExtendedAttribute($interface,
"ActiveDOMObject") ? "${v8ClassName}::toActiveDOMObject" : "0";
     my $toEventTarget = InheritsInterface($interface, "EventTarget") ?
"${v8ClassName}::toEventTarget" : "0";
-    my $rootForGC = NeedsOpaqueRootForGC($interface) ?
"${v8ClassName}::opaqueRootForGC" : "0";
+    my $resolveWrapperReachability =
NeedsResolveWrapperReachability($interface) ?
"${v8ClassName}::resolveWrapperReachability" : "0";
 
     # Find the super descriptor.
     my $parentClass = "";
@@ -4052,7 +4073,7 @@ END
     }
 
     my $code = "WrapperTypeInfo ${v8ClassName}::info = {
${v8ClassName}::GetTemplate, ${v8ClassName}::derefObject, $toActiveDOMObject,
$toEventTarget, ";
-    $code .= "$rootForGC, ${v8ClassName}::installPerContextPrototypeProperties,
$parentClassInfo, $WrapperTypePrototype };\n";
+    $code .= "$resolveWrapperReachability,
${v8ClassName}::installPerContextPrototypeProperties, $parentClassInfo,
$WrapperTypePrototype };\n";
     $implementation{nameSpaceWebCore}->addHeader($code);
 
     $implementation{nameSpaceInternal}->add("template <typename T> void
V8_USE(T) { }\n\n");
@@ -4109,8 +4130,8 @@ END
         GenerateReplaceableAttributeSetterCallback($interface);
     }
 
-    if (NeedsOpaqueRootForGC($interface)) {
-        GenerateOpaqueRootForGC($interface);
+    if (NeedsResolveWrapperReachability($interface)) {
+        GenerateResolveWrapperReachability($interface);
     }
 
     if ($interface->extendedAttributes->{"CheckSecurity"} && $interface->name
ne "Window") {
@@ -4919,8 +4940,7 @@ sub GenerateToV8Converters
     my $wrapperConfiguration = "WrapperConfiguration::Independent";
     if (InheritsExtendedAttribute($interface, "ActiveDOMObject")
         || InheritsExtendedAttribute($interface, "DependentLifetime")
-        || InheritsExtendedAttribute($interface, "GenerateIsReachable")
-        || InheritsExtendedAttribute($interface, "CustomIsReachable")
+        || NeedsResolveWrapperReachability($interface)
         || $v8ClassName =~ /SVG/) {
         $wrapperConfiguration = "WrapperConfiguration::Dependent";
     }

[commit-bot: I haz the power, 2013-10-17 08:37:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26792002/113001

[commit-bot: I haz the power, 2013-10-17 11:32:01]

Change committed as 159861

[marja, 2013-10-17 13:32:20]

Thanks for changing the name, SetReference is much better :)

[eseidel, 2013-10-19 20:32:43]

Karen says this is causing Canary to blow up in all sorts of ways and have
little kittens and poodles.

crbug.com/309378

[kouhei (in TOK), 2013-10-25 01:56:53]

haraken: PTAL again.

I'm trying to add a repro test for the revert, but I'd appreciate it if you
would take a look and see if this fix is ok for you. 

I modified DOMDataStore to support being called from NULL active context.

BTW, the issue we discussed offline about non-mainWorld wrappers incorrectly
being GC-ed should not happen. Thus this fix should be enough. This is because
all wrappers attached to same C++ object belongs to same obj group, so they are
kept alive as long as mainWorld wrapper is alive.

[kouhei (in TOK), 2013-10-25 02:07:25]

> I'm trying to add a repro test for the revert, but I'd appreciate it if you
> would take a look and see if this fix is ok for you. 
I forgot to mension the reason why this got reverted.

This causes crash when isolated worlds exist (e.g. some contentScript is
running) && gc is triggered without active v8::Context, that is from cpu idle
codepath. I added a "if" to handle case without active v8::Context in
DOMDataStore.cpp

[kouhei (in TOK), 2013-10-25 03:11:39]

On 2013/10/25 02:07:25, kouhei wrote:
> > I'm trying to add a repro test for the revert, but I'd appreciate it if you
> > would take a look and see if this fix is ok for you. 

added the repro testcase.

[haraken, 2013-10-25 05:21:22]

> BTW, the issue we discussed offline about non-mainWorld wrappers incorrectly
> being GC-ed should not happen. Thus this fix should be enough. This is because
> all wrappers attached to same C++ object belongs to same obj group, so they
are
> kept alive as long as mainWorld wrapper is alive.

This is true in object grouping, but this isn't true if we use SetReference.
Currently, SetReference just adds a reference to a wrapper in one world. If
you're planning to change the behavior so that SetReference adds references to
wrappers in all worlds, this CL makes sense to me. WDYT?

https://codereview.chromium.org/26792002/diff/303001/Source/bindings/v8/DOMDa...
File Source/bindings/v8/DOMDataStore.cpp (right):

https://codereview.chromium.org/26792002/diff/303001/Source/bindings/v8/DOMDa...
Source/bindings/v8/DOMDataStore.cpp:69: if (!context.IsEmpty()) {

This change makes sense. "If there is no entered context, let's think that the
main world store is the current world store", which makes sense to me.

[haraken, 2013-10-25 08:54:11]

https://codereview.chromium.org/26792002/diff/303001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/26792002/diff/303001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2292:
v8::V8::IdleNotification(largeEnoughValueToTriggerMajorGC);

This looks fragile. Probably you can use V8GCController::collectGarbage to
trigger a major GC.

[kouhei (in TOK), 2013-10-28 03:36:49]

On 2013/10/25 08:54:11, haraken wrote:
>
https://codereview.chromium.org/26792002/diff/303001/Source/core/testing/Inte...
> File Source/core/testing/Internals.cpp (right):
> 
>
https://codereview.chromium.org/26792002/diff/303001/Source/core/testing/Inte...
> Source/core/testing/Internals.cpp:2292:
> v8::V8::IdleNotification(largeEnoughValueToTriggerMajorGC);
> 
> This looks fragile. Probably you can use V8GCController::collectGarbage to
> trigger a major GC.

The point is that we want a gc() without any context.
V8GCController::collectGarbage will create a valid v8::Context.

[kouhei (in TOK), 2013-10-28 08:48:15]

On 2013/10/25 02:07:25, kouhei wrote:
> > I'm trying to add a repro test for the revert, but I'd appreciate it if you
> > would take a look and see if this fix is ok for you. 
> I forgot to mension the reason why this got reverted.
> 
> This causes crash when isolated worlds exist (e.g. some contentScript is
> running) && gc is triggered without active v8::Context, that is from cpu idle
> codepath. I added a "if" to handle case without active v8::Context in
> DOMDataStore.cpp

I updated the patch with the creationContext approach we discussed offline.

This would ensure that we are in the creationContext of the wrapper being
"resolveWrapperReachability" called, thus enabling a correct resolution of
DOMWrapperWorld.

[kouhei (in TOK), 2013-10-29 00:05:59]

On 2013/10/28 08:48:15, kouhei wrote:
> On 2013/10/25 02:07:25, kouhei wrote:
> > > I'm trying to add a repro test for the revert, but I'd appreciate it if
you
> > > would take a look and see if this fix is ok for you. 
> > I forgot to mension the reason why this got reverted.
> > 
> > This causes crash when isolated worlds exist (e.g. some contentScript is
> > running) && gc is triggered without active v8::Context, that is from cpu
idle
> > codepath. I added a "if" to handle case without active v8::Context in
> > DOMDataStore.cpp
> 
> I updated the patch with the creationContext approach we discussed offline.
> 
> This would ensure that we are in the creationContext of the wrapper being
> "resolveWrapperReachability" called, thus enabling a correct resolution of
> DOMWrapperWorld.

haraken: r?

[haraken, 2013-10-29 01:14:52]

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestInterface.cpp:796:
V8WrapperInstantiationScope scope(creationContext, isolate);

You can move this into the if branch.

This is OK in this CL, but it's error-prone that we have to create an
instantiation scope every time we access DOMDataStore. (As I mentioned yesterday
offline, there is already a bug.) We should invent a better programming model to
access a correct DOMDataStore.

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/DOMDa...
File Source/bindings/v8/DOMDataStore.h (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/DOMDa...
Source/bindings/v8/DOMDataStore.h:104: static void setWrapperReference(const
v8::Persistent<v8::Object>& parent, T* child, v8::Isolate* isolate)

Since you're using "setWrapperReference" here...

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/DOMDa...
Source/bindings/v8/DOMDataStore.h:114: current(isolate)->template
setReference<V8T>(parent, child, isolate);

...shall we rename:

setReference => setWrapperReference

[SetReference] => [SetWrapperReference

?

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Unsaf...
File Source/bindings/v8/UnsafePersistent.h (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Unsaf...
Source/bindings/v8/UnsafePersistent.h:93: // FIXME: remove As() after v8 roll.

Hasn't the V8 roll already done?

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Wrapp...
File Source/bindings/v8/WrapperTypeInfo.h (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Wrapp...
Source/bindings/v8/WrapperTypeInfo.h:71: inline void setObjectGroup(void*
object, const v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)

Can this be a static method?

Also it would be better to put this method in DOMDataStore.

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Wrapp...
Source/bindings/v8/WrapperTypeInfo.h:73: // FIXME: remove reinterpret_cast after
http://crrev.com/27512003 has rolled.

Hasn't the V8 CL already rolled?

https://codereview.chromium.org/26792002/diff/463001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/26792002/diff/463001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2244: void
Internals::garbageCollectV8WithoutContext()

garbageCollectV8WithoutContext => collectGarbageWithoutV8Context

https://codereview.chromium.org/26792002/diff/463001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2246: m_gcWithoutContextTimer.startOneShot(0);

Why do you need a timer?

https://codereview.chromium.org/26792002/diff/463001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:2253:
v8::V8::IdleNotification(largeEnoughValueToTriggerMajorGC);

This is not a reliable way to trigger GC. Instead you can call:

  V8ScriptRunner::compileAndRunInternalScript(v8String("if (gc) gc();",
isolate), isolate);

[kouhei (in TOK), 2013-10-29 04:19:57]

I removed the LayoutTest from offline discussion with haraken. It seems
dangerous to rely on V8::IdleNotification to trigger gc.

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestInterface.cpp:796:
V8WrapperInstantiationScope scope(creationContext, isolate);
On 2013/10/29 01:14:53, haraken wrote:
> 
> You can move this into the if branch.
> 
> This is OK in this CL, but it's error-prone that we have to create an
> instantiation scope every time we access DOMDataStore. (As I mentioned
yesterday
> offline, there is already a bug.) We should invent a better programming model
to
> access a correct DOMDataStore.

There may be multiple [SetReference]s, so I think this is the correct place for
InstantiationScope.

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/DOMDa...
File Source/bindings/v8/DOMDataStore.h (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/DOMDa...
Source/bindings/v8/DOMDataStore.h:114: current(isolate)->template
setReference<V8T>(parent, child, isolate);
On 2013/10/29 01:14:53, haraken wrote:
> 
> ...shall we rename:
> 
> setReference => setWrapperReference
> 
> [SetReference] => [SetWrapperReference
> 
> ?

I guess this issue is resolved from offline discussion.

The main logic is in "setReference" method. This method is named
set"Wrapper"Reference to follow naming convensions for static methods.

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Unsaf...
File Source/bindings/v8/UnsafePersistent.h (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Unsaf...
Source/bindings/v8/UnsafePersistent.h:93: // FIXME: remove As() after v8 roll.
On 2013/10/29 01:14:53, haraken wrote:
> 
> Hasn't the V8 roll already done?

Done.

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Wrapp...
File Source/bindings/v8/WrapperTypeInfo.h (right):

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Wrapp...
Source/bindings/v8/WrapperTypeInfo.h:71: inline void setObjectGroup(void*
object, const v8::Persistent<v8::Object>& wrapper, v8::Isolate* isolate)
On 2013/10/29 01:14:53, haraken wrote:
> 
> Can this be a static method?
> 
> Also it would be better to put this method in DOMDataStore.

This can't be in DOMDataStore as this need to be referenced from
resolveWrapperReachability defined below.

https://codereview.chromium.org/26792002/diff/463001/Source/bindings/v8/Wrapp...
Source/bindings/v8/WrapperTypeInfo.h:73: // FIXME: remove reinterpret_cast after
http://crrev.com/27512003 has rolled.
On 2013/10/29 01:14:53, haraken wrote:
> 
> Hasn't the V8 CL already rolled?

Done.

[haraken, 2013-10-29 04:22:19]

LGTM

[kouhei (in TOK), 2013-10-29 04:23:30]

Thanks for review!

[commit-bot: I haz the power, 2013-10-29 04:23:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26792002/563001

[commit-bot: I haz the power, 2013-10-29 04:51:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_blink.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-29 06:04:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26792002/733001

[commit-bot: I haz the power, 2013-10-29 07:05:28]

Change committed as 160800

[kouhei (in TOK), 2013-10-31 02:29:47]

Quick fix. Marked NodeFilter as Dependent.

haraken: r?

[haraken, 2013-10-31 02:33:48]

LGTM.

[commit-bot: I haz the power, 2013-10-31 02:34:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26792002/823001

[commit-bot: I haz the power, 2013-10-31 03:02:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-10-31 03:04:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26792002/823001

[commit-bot: I haz the power, 2013-10-31 03:40:36]

Retried try job too often on linux_blink_rel for step(s) webkit_lint,
webkit_python_tests, webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-31 04:56:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26792002/823001

[commit-bot: I haz the power, 2013-10-31 04:56:56]

Change committed as 161018