VisitPersistentHandle should not call visitDOMWrapper

VisitPersistentHandle should not call visitDOMWrapper

It is not allowed to allocate a V8 object during VisitPersistentHandle.
visitDOMWrapper can allocate a V8 object. This means that VisitPersistentHandle
must not call visitDOMWrapper.

This CL delays calling visitDOMWrapper to MajorGCWrapperVisitor::notifyFinished.

BUG=

[haraken, 2015-06-20 09:29:48]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org

[haraken, 2015-06-20 09:29:49]

https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
File Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
Source/bindings/core/v8/V8GCController.cpp:292: ASSERT(!value->IsIndependent());

This CL is not correct. The access to |value| causes stack-use-after-free,
because v8::Persistent<v8::Value>* is pointing to an on-stack pointer. How can I
avoid the issue?

One way to avoid this would be to copy the persistent handles into
m_wrappersToBeVisited. However persistent handles are not copyable...

Jochen: Any idea?

[jochen (gone - plz use gerrit), 2015-06-22 08:45:49]

https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
File Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
Source/bindings/core/v8/V8GCController.cpp:292: ASSERT(!value->IsIndependent());
On 2015/06/20 at 09:29:49, haraken wrote:
> This CL is not correct. The access to |value| causes stack-use-after-free,
because v8::Persistent<v8::Value>* is pointing to an on-stack pointer. How can I
avoid the issue?
> 
> One way to avoid this would be to copy the persistent handles into
m_wrappersToBeVisited. However persistent handles are not copyable...
> 
> Jochen: Any idea?

what about unwrapping the handle and collecting ScriptWrappable* ?

https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
Source/bindings/core/v8/V8GCController.cpp:296:
wrapperTypeInfo->visitDOMWrapper(m_isolate, toScriptWrappable(wrapper),
persistentWrapper);
can this create new Persistent<> handles that need to be in the right object
group?

[haraken, 2015-06-22 10:54:36]

On 2015/06/22 08:45:49, jochen wrote:
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> File Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> Source/bindings/core/v8/V8GCController.cpp:292:
ASSERT(!value->IsIndependent());
> On 2015/06/20 at 09:29:49, haraken wrote:
> > This CL is not correct. The access to |value| causes stack-use-after-free,
> because v8::Persistent<v8::Value>* is pointing to an on-stack pointer. How can
I
> avoid the issue?
> > 
> > One way to avoid this would be to copy the persistent handles into
> m_wrappersToBeVisited. However persistent handles are not copyable...
> > 
> > Jochen: Any idea?
> 
> what about unwrapping the handle and collecting ScriptWrappable* ?

visitDOMWrapper needs a reference to Persitent<Object>, because it needs to call
SetReferenceFromGroup etc which needs a reference to Persistent<Object>.
ScriptWrappable* gives us a wrapper of the main world but doesn't give us
wrappers of isolated worlds... So we need to somehow collect the references to
Persistent<Object>s that are passed into the VisitPersistentHandle.

[jochen (gone - plz use gerrit), 2015-06-22 10:55:21]

On 2015/06/22 at 10:54:36, haraken wrote:
> On 2015/06/22 08:45:49, jochen wrote:
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > File Source/bindings/core/v8/V8GCController.cpp (right):
> > 
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > Source/bindings/core/v8/V8GCController.cpp:292:
ASSERT(!value->IsIndependent());
> > On 2015/06/20 at 09:29:49, haraken wrote:
> > > This CL is not correct. The access to |value| causes stack-use-after-free,
> > because v8::Persistent<v8::Value>* is pointing to an on-stack pointer. How
can I
> > avoid the issue?
> > > 
> > > One way to avoid this would be to copy the persistent handles into
> > m_wrappersToBeVisited. However persistent handles are not copyable...
> > > 
> > > Jochen: Any idea?
> > 
> > what about unwrapping the handle and collecting ScriptWrappable* ?
> 
> visitDOMWrapper needs a reference to Persitent<Object>, because it needs to
call SetReferenceFromGroup etc which needs a reference to Persistent<Object>.
ScriptWrappable* gives us a wrapper of the main world but doesn't give us
wrappers of isolated worlds... So we need to somehow collect the references to
Persistent<Object>s that are passed into the VisitPersistentHandle.

also collect the world id?

[jochen (gone - plz use gerrit), 2015-06-22 10:55:21]

On 2015/06/22 at 10:54:36, haraken wrote:
> On 2015/06/22 08:45:49, jochen wrote:
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > File Source/bindings/core/v8/V8GCController.cpp (right):
> > 
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > Source/bindings/core/v8/V8GCController.cpp:292:
ASSERT(!value->IsIndependent());
> > On 2015/06/20 at 09:29:49, haraken wrote:
> > > This CL is not correct. The access to |value| causes stack-use-after-free,
> > because v8::Persistent<v8::Value>* is pointing to an on-stack pointer. How
can I
> > avoid the issue?
> > > 
> > > One way to avoid this would be to copy the persistent handles into
> > m_wrappersToBeVisited. However persistent handles are not copyable...
> > > 
> > > Jochen: Any idea?
> > 
> > what about unwrapping the handle and collecting ScriptWrappable* ?
> 
> visitDOMWrapper needs a reference to Persitent<Object>, because it needs to
call SetReferenceFromGroup etc which needs a reference to Persistent<Object>.
ScriptWrappable* gives us a wrapper of the main world but doesn't give us
wrappers of isolated worlds... So we need to somehow collect the references to
Persistent<Object>s that are passed into the VisitPersistentHandle.

also collect the world id?

[haraken, 2015-06-22 11:07:54]

On 2015/06/22 10:55:21, jochen wrote:
> On 2015/06/22 at 10:54:36, haraken wrote:
> > On 2015/06/22 08:45:49, jochen wrote:
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > File Source/bindings/core/v8/V8GCController.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > Source/bindings/core/v8/V8GCController.cpp:292:
> ASSERT(!value->IsIndependent());
> > > On 2015/06/20 at 09:29:49, haraken wrote:
> > > > This CL is not correct. The access to |value| causes
stack-use-after-free,
> > > because v8::Persistent<v8::Value>* is pointing to an on-stack pointer. How
> can I
> > > avoid the issue?
> > > > 
> > > > One way to avoid this would be to copy the persistent handles into
> > > m_wrappersToBeVisited. However persistent handles are not copyable...
> > > > 
> > > > Jochen: Any idea?
> > > 
> > > what about unwrapping the handle and collecting ScriptWrappable* ?
> > 
> > visitDOMWrapper needs a reference to Persitent<Object>, because it needs to
> call SetReferenceFromGroup etc which needs a reference to Persistent<Object>.
> ScriptWrappable* gives us a wrapper of the main world but doesn't give us
> wrappers of isolated worlds... So we need to somehow collect the references to
> Persistent<Object>s that are passed into the VisitPersistentHandle.
> 
> also collect the world id?

It's doable but nasty. For each wrapper, we need to look up:

  wrapper -> creation context -> ScriptState -> DOMWrapperWorld -> id

Maybe what we could do is:

- In VisitPersistentHandle, collect wrappers in only the main world.
- After finishing VisitPersistentHandle, visit the collected wrappers (in the
main world) and all wrappers in the DOMWrapperMap (which contains all wrappers
of isolated worlds).

but this is nasty as well.

Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the V8
object won't be visited -- this is already problematic. It seems that a right
fix is to refactor visitDOMWrapper so that it doesn't allocate any V8 object. It
is doable with Oilpan but it will take a bit more time...

(Still thinking about a short-term solution.)

[jochen (gone - plz use gerrit), 2015-06-23 14:51:36]

On 2015/06/22 at 11:07:54, haraken wrote:
> On 2015/06/22 10:55:21, jochen wrote:
> > On 2015/06/22 at 10:54:36, haraken wrote:
> > > On 2015/06/22 08:45:49, jochen wrote:
> > > >
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > File Source/bindings/core/v8/V8GCController.cpp (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > Source/bindings/core/v8/V8GCController.cpp:292:
> > ASSERT(!value->IsIndependent());
> > > > On 2015/06/20 at 09:29:49, haraken wrote:
> > > > > This CL is not correct. The access to |value| causes
stack-use-after-free,
> > > > because v8::Persistent<v8::Value>* is pointing to an on-stack pointer.
How
> > can I
> > > > avoid the issue?
> > > > > 
> > > > > One way to avoid this would be to copy the persistent handles into
> > > > m_wrappersToBeVisited. However persistent handles are not copyable...
> > > > > 
> > > > > Jochen: Any idea?
> > > > 
> > > > what about unwrapping the handle and collecting ScriptWrappable* ?
> > > 
> > > visitDOMWrapper needs a reference to Persitent<Object>, because it needs
to
> > call SetReferenceFromGroup etc which needs a reference to
Persistent<Object>.
> > ScriptWrappable* gives us a wrapper of the main world but doesn't give us
> > wrappers of isolated worlds... So we need to somehow collect the references
to
> > Persistent<Object>s that are passed into the VisitPersistentHandle.
> > 
> > also collect the world id?
> 
> It's doable but nasty. For each wrapper, we need to look up:
> 
>   wrapper -> creation context -> ScriptState -> DOMWrapperWorld -> id
> 
> Maybe what we could do is:
> 
> - In VisitPersistentHandle, collect wrappers in only the main world.
> - After finishing VisitPersistentHandle, visit the collected wrappers (in the
main world) and all wrappers in the DOMWrapperMap (which contains all wrappers
of isolated worlds).
> 
> but this is nasty as well.
> 
> Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the V8
object won't be visited -- this is already problematic. It seems that a right
fix is to refactor visitDOMWrapper so that it doesn't allocate any V8 object. It
is doable with Oilpan but it will take a bit more time...
> 
> (Still thinking about a short-term solution.)

another solution would be to eagerly create the wrappers that visitDOMWrapper
would otherwise create on demand

[haraken, 2015-06-23 15:09:50]

On 2015/06/23 14:51:36, jochen wrote:
> On 2015/06/22 at 11:07:54, haraken wrote:
> > On 2015/06/22 10:55:21, jochen wrote:
> > > On 2015/06/22 at 10:54:36, haraken wrote:
> > > > On 2015/06/22 08:45:49, jochen wrote:
> > > > >
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > File Source/bindings/core/v8/V8GCController.cpp (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > Source/bindings/core/v8/V8GCController.cpp:292:
> > > ASSERT(!value->IsIndependent());
> > > > > On 2015/06/20 at 09:29:49, haraken wrote:
> > > > > > This CL is not correct. The access to |value| causes
> stack-use-after-free,
> > > > > because v8::Persistent<v8::Value>* is pointing to an on-stack pointer.
> How
> > > can I
> > > > > avoid the issue?
> > > > > > 
> > > > > > One way to avoid this would be to copy the persistent handles into
> > > > > m_wrappersToBeVisited. However persistent handles are not copyable...
> > > > > > 
> > > > > > Jochen: Any idea?
> > > > > 
> > > > > what about unwrapping the handle and collecting ScriptWrappable* ?
> > > > 
> > > > visitDOMWrapper needs a reference to Persitent<Object>, because it needs
> to
> > > call SetReferenceFromGroup etc which needs a reference to
> Persistent<Object>.
> > > ScriptWrappable* gives us a wrapper of the main world but doesn't give us
> > > wrappers of isolated worlds... So we need to somehow collect the
references
> to
> > > Persistent<Object>s that are passed into the VisitPersistentHandle.
> > > 
> > > also collect the world id?
> > 
> > It's doable but nasty. For each wrapper, we need to look up:
> > 
> >   wrapper -> creation context -> ScriptState -> DOMWrapperWorld -> id
> > 
> > Maybe what we could do is:
> > 
> > - In VisitPersistentHandle, collect wrappers in only the main world.
> > - After finishing VisitPersistentHandle, visit the collected wrappers (in
the
> main world) and all wrappers in the DOMWrapperMap (which contains all wrappers
> of isolated worlds).
> > 
> > but this is nasty as well.
> > 
> > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the
V8
> object won't be visited -- this is already problematic. It seems that a right
> fix is to refactor visitDOMWrapper so that it doesn't allocate any V8 object.
It
> is doable with Oilpan but it will take a bit more time...
> > 
> > (Still thinking about a short-term solution.)
> 
> another solution would be to eagerly create the wrappers that visitDOMWrapper
> would otherwise create on demand

We tried it before but it didn't work because the wrapper reachability can
dynamically change in some SVG elements...

> > - In VisitPersistentHandle, collect wrappers in only the main world.
> > - After finishing VisitPersistentHandle, visit the collected wrappers (in
the
> main world) and all wrappers in the DOMWrapperMap (which contains all wrappers
> of isolated worlds).

^^^ I'm now thinking that this will work. I'll prepare a CL when I get a cycle.

> > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the
V8
> object won't be visited -- this is already problematic. It seems that a right
> fix is to refactor visitDOMWrapper so that it doesn't allocate any V8 object.
It
> is doable with Oilpan but it will take a bit more time...

^^^ I noticed that this is not a concern. There is no need to visit the new
wrapper we're now creating. (If we really need to visit it, we can just call
visitDOMWrapper for the wrapper.)


BTW, what happens if (1) we call a visitDOMWrapper at the end of the gcPrologue,
(2) the visitDOMWrapper creates a wrapper, and then (3) it triggers a V8 GC?
Won't it lead to a nested GC?

[haraken, 2015-06-23 15:09:52]

On 2015/06/23 14:51:36, jochen wrote:
> On 2015/06/22 at 11:07:54, haraken wrote:
> > On 2015/06/22 10:55:21, jochen wrote:
> > > On 2015/06/22 at 10:54:36, haraken wrote:
> > > > On 2015/06/22 08:45:49, jochen wrote:
> > > > >
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > File Source/bindings/core/v8/V8GCController.cpp (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > Source/bindings/core/v8/V8GCController.cpp:292:
> > > ASSERT(!value->IsIndependent());
> > > > > On 2015/06/20 at 09:29:49, haraken wrote:
> > > > > > This CL is not correct. The access to |value| causes
> stack-use-after-free,
> > > > > because v8::Persistent<v8::Value>* is pointing to an on-stack pointer.
> How
> > > can I
> > > > > avoid the issue?
> > > > > > 
> > > > > > One way to avoid this would be to copy the persistent handles into
> > > > > m_wrappersToBeVisited. However persistent handles are not copyable...
> > > > > > 
> > > > > > Jochen: Any idea?
> > > > > 
> > > > > what about unwrapping the handle and collecting ScriptWrappable* ?
> > > > 
> > > > visitDOMWrapper needs a reference to Persitent<Object>, because it needs
> to
> > > call SetReferenceFromGroup etc which needs a reference to
> Persistent<Object>.
> > > ScriptWrappable* gives us a wrapper of the main world but doesn't give us
> > > wrappers of isolated worlds... So we need to somehow collect the
references
> to
> > > Persistent<Object>s that are passed into the VisitPersistentHandle.
> > > 
> > > also collect the world id?
> > 
> > It's doable but nasty. For each wrapper, we need to look up:
> > 
> >   wrapper -> creation context -> ScriptState -> DOMWrapperWorld -> id
> > 
> > Maybe what we could do is:
> > 
> > - In VisitPersistentHandle, collect wrappers in only the main world.
> > - After finishing VisitPersistentHandle, visit the collected wrappers (in
the
> main world) and all wrappers in the DOMWrapperMap (which contains all wrappers
> of isolated worlds).
> > 
> > but this is nasty as well.
> > 
> > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the
V8
> object won't be visited -- this is already problematic. It seems that a right
> fix is to refactor visitDOMWrapper so that it doesn't allocate any V8 object.
It
> is doable with Oilpan but it will take a bit more time...
> > 
> > (Still thinking about a short-term solution.)
> 
> another solution would be to eagerly create the wrappers that visitDOMWrapper
> would otherwise create on demand

We tried it before but it didn't work because the wrapper reachability can
dynamically change in some SVG elements...

> > - In VisitPersistentHandle, collect wrappers in only the main world.
> > - After finishing VisitPersistentHandle, visit the collected wrappers (in
the
> main world) and all wrappers in the DOMWrapperMap (which contains all wrappers
> of isolated worlds).

^^^ I'm now thinking that this will work. I'll prepare a CL when I get a cycle.

> > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the
V8
> object won't be visited -- this is already problematic. It seems that a right
> fix is to refactor visitDOMWrapper so that it doesn't allocate any V8 object.
It
> is doable with Oilpan but it will take a bit more time...

^^^ I noticed that this is not a concern. There is no need to visit the new
wrapper we're now creating. (If we really need to visit it, we can just call
visitDOMWrapper for the wrapper.)


BTW, what happens if (1) we call a visitDOMWrapper at the end of the gcPrologue,
(2) the visitDOMWrapper creates a wrapper, and then (3) it triggers a V8 GC?
Won't it lead to a nested GC?

[jochen (gone - plz use gerrit), 2015-06-23 15:11:14]

On 2015/06/23 at 15:09:52, haraken wrote:
> On 2015/06/23 14:51:36, jochen wrote:
> > On 2015/06/22 at 11:07:54, haraken wrote:
> > > On 2015/06/22 10:55:21, jochen wrote:
> > > > On 2015/06/22 at 10:54:36, haraken wrote:
> > > > > On 2015/06/22 08:45:49, jochen wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > > File Source/bindings/core/v8/V8GCController.cpp (right):
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > > Source/bindings/core/v8/V8GCController.cpp:292:
> > > > ASSERT(!value->IsIndependent());
> > > > > > On 2015/06/20 at 09:29:49, haraken wrote:
> > > > > > > This CL is not correct. The access to |value| causes
> > stack-use-after-free,
> > > > > > because v8::Persistent<v8::Value>* is pointing to an on-stack
pointer.
> > How
> > > > can I
> > > > > > avoid the issue?
> > > > > > > 
> > > > > > > One way to avoid this would be to copy the persistent handles into
> > > > > > m_wrappersToBeVisited. However persistent handles are not
copyable...
> > > > > > > 
> > > > > > > Jochen: Any idea?
> > > > > > 
> > > > > > what about unwrapping the handle and collecting ScriptWrappable* ?
> > > > > 
> > > > > visitDOMWrapper needs a reference to Persitent<Object>, because it
needs
> > to
> > > > call SetReferenceFromGroup etc which needs a reference to
> > Persistent<Object>.
> > > > ScriptWrappable* gives us a wrapper of the main world but doesn't give
us
> > > > wrappers of isolated worlds... So we need to somehow collect the
references
> > to
> > > > Persistent<Object>s that are passed into the VisitPersistentHandle.
> > > > 
> > > > also collect the world id?
> > > 
> > > It's doable but nasty. For each wrapper, we need to look up:
> > > 
> > >   wrapper -> creation context -> ScriptState -> DOMWrapperWorld -> id
> > > 
> > > Maybe what we could do is:
> > > 
> > > - In VisitPersistentHandle, collect wrappers in only the main world.
> > > - After finishing VisitPersistentHandle, visit the collected wrappers (in
the
> > main world) and all wrappers in the DOMWrapperMap (which contains all
wrappers
> > of isolated worlds).
> > > 
> > > but this is nasty as well.
> > > 
> > > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the
V8
> > object won't be visited -- this is already problematic. It seems that a
right
> > fix is to refactor visitDOMWrapper so that it doesn't allocate any V8
object. It
> > is doable with Oilpan but it will take a bit more time...
> > > 
> > > (Still thinking about a short-term solution.)
> > 
> > another solution would be to eagerly create the wrappers that
visitDOMWrapper
> > would otherwise create on demand
> 
> We tried it before but it didn't work because the wrapper reachability can
dynamically change in some SVG elements...
> 
> > > - In VisitPersistentHandle, collect wrappers in only the main world.
> > > - After finishing VisitPersistentHandle, visit the collected wrappers (in
the
> > main world) and all wrappers in the DOMWrapperMap (which contains all
wrappers
> > of isolated worlds).
> 
> ^^^ I'm now thinking that this will work. I'll prepare a CL when I get a
cycle.
> 
> > > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object, the
V8
> > object won't be visited -- this is already problematic. It seems that a
right
> > fix is to refactor visitDOMWrapper so that it doesn't allocate any V8
object. It
> > is doable with Oilpan but it will take a bit more time...
> 
> ^^^ I noticed that this is not a concern. There is no need to visit the new
wrapper we're now creating. (If we really need to visit it, we can just call
visitDOMWrapper for the wrapper.)
> 
> 
> BTW, what happens if (1) we call a visitDOMWrapper at the end of the
gcPrologue, (2) the visitDOMWrapper creates a wrapper, and then (3) it triggers
a V8 GC? Won't it lead to a nested GC?

yes, however, nested GCs are supported (kinda)

[haraken, 2015-06-30 04:48:14]

On 2015/06/23 15:11:14, jochen wrote:
> On 2015/06/23 at 15:09:52, haraken wrote:
> > On 2015/06/23 14:51:36, jochen wrote:
> > > On 2015/06/22 at 11:07:54, haraken wrote:
> > > > On 2015/06/22 10:55:21, jochen wrote:
> > > > > On 2015/06/22 at 10:54:36, haraken wrote:
> > > > > > On 2015/06/22 08:45:49, jochen wrote:
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > > > File Source/bindings/core/v8/V8GCController.cpp (right):
> > > > > > > 
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > > > Source/bindings/core/v8/V8GCController.cpp:292:
> > > > > ASSERT(!value->IsIndependent());
> > > > > > > On 2015/06/20 at 09:29:49, haraken wrote:
> > > > > > > > This CL is not correct. The access to |value| causes
> > > stack-use-after-free,
> > > > > > > because v8::Persistent<v8::Value>* is pointing to an on-stack
> pointer.
> > > How
> > > > > can I
> > > > > > > avoid the issue?
> > > > > > > > 
> > > > > > > > One way to avoid this would be to copy the persistent handles
into
> > > > > > > m_wrappersToBeVisited. However persistent handles are not
> copyable...
> > > > > > > > 
> > > > > > > > Jochen: Any idea?
> > > > > > > 
> > > > > > > what about unwrapping the handle and collecting ScriptWrappable* ?
> > > > > > 
> > > > > > visitDOMWrapper needs a reference to Persitent<Object>, because it
> needs
> > > to
> > > > > call SetReferenceFromGroup etc which needs a reference to
> > > Persistent<Object>.
> > > > > ScriptWrappable* gives us a wrapper of the main world but doesn't give
> us
> > > > > wrappers of isolated worlds... So we need to somehow collect the
> references
> > > to
> > > > > Persistent<Object>s that are passed into the VisitPersistentHandle.
> > > > > 
> > > > > also collect the world id?
> > > > 
> > > > It's doable but nasty. For each wrapper, we need to look up:
> > > > 
> > > >   wrapper -> creation context -> ScriptState -> DOMWrapperWorld -> id
> > > > 
> > > > Maybe what we could do is:
> > > > 
> > > > - In VisitPersistentHandle, collect wrappers in only the main world.
> > > > - After finishing VisitPersistentHandle, visit the collected wrappers
(in
> the
> > > main world) and all wrappers in the DOMWrapperMap (which contains all
> wrappers
> > > of isolated worlds).
> > > > 
> > > > but this is nasty as well.
> > > > 
> > > > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object,
the
> V8
> > > object won't be visited -- this is already problematic. It seems that a
> right
> > > fix is to refactor visitDOMWrapper so that it doesn't allocate any V8
> object. It
> > > is doable with Oilpan but it will take a bit more time...
> > > > 
> > > > (Still thinking about a short-term solution.)
> > > 
> > > another solution would be to eagerly create the wrappers that
> visitDOMWrapper
> > > would otherwise create on demand
> > 
> > We tried it before but it didn't work because the wrapper reachability can
> dynamically change in some SVG elements...
> > 
> > > > - In VisitPersistentHandle, collect wrappers in only the main world.
> > > > - After finishing VisitPersistentHandle, visit the collected wrappers
(in
> the
> > > main world) and all wrappers in the DOMWrapperMap (which contains all
> wrappers
> > > of isolated worlds).
> > 
> > ^^^ I'm now thinking that this will work. I'll prepare a CL when I get a
> cycle.

Hmm, I tried to go with the approach but found it hard to implement it in an
efficient way.

Jochen, would it be possible to implement Isolate::SetReferenceToGroup?
Currently we only have Isolate::SetReferenceFromGroup.

My plan is as follows:

(0) Imagine that you have [SetWrapperReferenceTo=Y] in X.idl. What we want to do
here is to add a reference from X's wrapper to Y's wrapper. At this point, it is
not guaranteed that Y has a wrapper.

(1) When visitDOMWrapper(X's wrapper) is called in VisitPersistentHandles, we
just call SetReferenceToGroup(UniqueId(Y), X's wrapper). We don't create Y's
wrapper at this point.

(2) Once we finish VisitPersistentHandles, we create Y's wrapper and call
SetObjectGroupGroupId(Y's wrapper, UniqueId(Y)).

(3) GCPrologue finishes.

[jochen (gone - plz use gerrit), 2015-06-30 08:12:20]

On 2015/06/30 at 04:48:14, haraken wrote:
> On 2015/06/23 15:11:14, jochen wrote:
> > On 2015/06/23 at 15:09:52, haraken wrote:
> > > On 2015/06/23 14:51:36, jochen wrote:
> > > > On 2015/06/22 at 11:07:54, haraken wrote:
> > > > > On 2015/06/22 10:55:21, jochen wrote:
> > > > > > On 2015/06/22 at 10:54:36, haraken wrote:
> > > > > > > On 2015/06/22 08:45:49, jochen wrote:
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > > > > File Source/bindings/core/v8/V8GCController.cpp (right):
> > > > > > > > 
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1200613002/diff/1/Source/bindings/core/v8/V8G...
> > > > > > > > Source/bindings/core/v8/V8GCController.cpp:292:
> > > > > > ASSERT(!value->IsIndependent());
> > > > > > > > On 2015/06/20 at 09:29:49, haraken wrote:
> > > > > > > > > This CL is not correct. The access to |value| causes
> > > > stack-use-after-free,
> > > > > > > > because v8::Persistent<v8::Value>* is pointing to an on-stack
> > pointer.
> > > > How
> > > > > > can I
> > > > > > > > avoid the issue?
> > > > > > > > > 
> > > > > > > > > One way to avoid this would be to copy the persistent handles
into
> > > > > > > > m_wrappersToBeVisited. However persistent handles are not
> > copyable...
> > > > > > > > > 
> > > > > > > > > Jochen: Any idea?
> > > > > > > > 
> > > > > > > > what about unwrapping the handle and collecting ScriptWrappable*
?
> > > > > > > 
> > > > > > > visitDOMWrapper needs a reference to Persitent<Object>, because it
> > needs
> > > > to
> > > > > > call SetReferenceFromGroup etc which needs a reference to
> > > > Persistent<Object>.
> > > > > > ScriptWrappable* gives us a wrapper of the main world but doesn't
give
> > us
> > > > > > wrappers of isolated worlds... So we need to somehow collect the
> > references
> > > > to
> > > > > > Persistent<Object>s that are passed into the VisitPersistentHandle.
> > > > > > 
> > > > > > also collect the world id?
> > > > > 
> > > > > It's doable but nasty. For each wrapper, we need to look up:
> > > > > 
> > > > >   wrapper -> creation context -> ScriptState -> DOMWrapperWorld -> id
> > > > > 
> > > > > Maybe what we could do is:
> > > > > 
> > > > > - In VisitPersistentHandle, collect wrappers in only the main world.
> > > > > - After finishing VisitPersistentHandle, visit the collected wrappers
(in
> > the
> > > > main world) and all wrappers in the DOMWrapperMap (which contains all
> > wrappers
> > > > of isolated worlds).
> > > > > 
> > > > > but this is nasty as well.
> > > > > 
> > > > > Hmm, in the first place, if visitDOMWrapper allocates a new V8 object,
the
> > V8
> > > > object won't be visited -- this is already problematic. It seems that a
> > right
> > > > fix is to refactor visitDOMWrapper so that it doesn't allocate any V8
> > object. It
> > > > is doable with Oilpan but it will take a bit more time...
> > > > > 
> > > > > (Still thinking about a short-term solution.)
> > > > 
> > > > another solution would be to eagerly create the wrappers that
> > visitDOMWrapper
> > > > would otherwise create on demand
> > > 
> > > We tried it before but it didn't work because the wrapper reachability can
> > dynamically change in some SVG elements...
> > > 
> > > > > - In VisitPersistentHandle, collect wrappers in only the main world.
> > > > > - After finishing VisitPersistentHandle, visit the collected wrappers
(in
> > the
> > > > main world) and all wrappers in the DOMWrapperMap (which contains all
> > wrappers
> > > > of isolated worlds).
> > > 
> > > ^^^ I'm now thinking that this will work. I'll prepare a CL when I get a
> > cycle.
> 
> Hmm, I tried to go with the approach but found it hard to implement it in an
efficient way.
> 
> Jochen, would it be possible to implement Isolate::SetReferenceToGroup?
Currently we only have Isolate::SetReferenceFromGroup.
> 
> My plan is as follows:
> 
> (0) Imagine that you have [SetWrapperReferenceTo=Y] in X.idl. What we want to
do here is to add a reference from X's wrapper to Y's wrapper. At this point, it
is not guaranteed that Y has a wrapper.
> 
> (1) When visitDOMWrapper(X's wrapper) is called in VisitPersistentHandles, we
just call SetReferenceToGroup(UniqueId(Y), X's wrapper). We don't create Y's
wrapper at this point.
> 
> (2) Once we finish VisitPersistentHandles, we create Y's wrapper and call
SetObjectGroupGroupId(Y's wrapper, UniqueId(Y)).
> 
> (3) GCPrologue finishes.

sure, that should be possible. i'll give it a try

[haraken, 2015-07-08 02:38:09]

OK, now I understand the issue around here and can answer the question Jochen
asked me offline.

In short, I'm planning to fix the problem by landing
https://codereview.chromium.org/1221193004.

The current implementation is as follows:

(a) [SetWrapperReferenceFrom=Y] on X.idl means that "If any wrapper in the
object group which Y belongs to is reachable, X's wrapper must be kept alive".

(b) [SetWrapperReferenceTo=Y] on X.idl means that "If Y doesn't have a wrapper,
forcibly create Y's wrapper. If X's wrapper is reachable, Y's wrapper must be
kept alive".

The problem is that (b) is doing strange things.

(b) is mostly used to represent a DOM-side RefPtr as a reference in the JS-side
in a case where the RefPtr causes a reference cycle in the DOM side (This issue
will be gone in the Oilpan world). The real intention of adding a hidden
reference from X's wrapper to Y's wrapper is to add a (conceptual) RefPtr from X
to Y. However, then one question arises:

  - Before a V8 GC gets triggered and it calls visitDOMWrapper, Y's wrapper
didn't exist. Then who was keeping Y alive while X is alive before the V8 GC
creates the Y's wrapper?

The answer is that no one kept it alive. In other words, if Blink wants to
represent a DOM-side RefPtr from X to Y as a hidden reference from X's wrapper
to Y's wrapper, Blink must guarantee that Y always has a wrapper. However, this
guarantee is sometimes broken
(https://code.google.com/p/chromium/issues/detail?id=467211). Thus we're using
the V8 GC and forcibly creating Y's wrapper if Y's wrapper is missing.

This approach is wrong. The real problem is that Blink is failing at
guaranteeing that Y always has a wrapper, and we're hiding the problem by asking
the V8 GC to create Y's wrapper. It will just hide the problem in some (common?)
cases. What we should fix is the Blink-side implementation.

So I'm planning to land https://codereview.chromium.org/1221193004. After
landing the CL, we will have the following rule:

(a) [SetWrapperReferenceFrom=Y] on X.idl means that "If any wrapper in the
object group which Y belongs to is reachable, X's wrapper must be kept alive".
(Note: In real IDL use cases, Y is limited to Node.)

(b) [SetWrapperReferenceTo=Y] on X.idl means that "If X's wrapper is reachable,
any wrapper in the object group which Y belongs to is reachable". (Note: In real
IDL use cases, the object group contains only Y's wrapper.)

As mentioned above, this change will expose existing Blink's lifetime bug.
However, given the following reasons, I think it would be reasonable to land the
CL right now.

- The CL passes all layout tests.
- It is more important to fix the crashes in V8 GC.
- The Blink's lifetime bug will be fixed once we ship Oilpan.

[jsbell, 2015-07-08 17:16:45]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-07-08 17:16:45]

Any chance this unblock https://codereview.chromium.org/931543002/ and
https://codereview.chromium.org/924863003/ ?

(I need to read it in more detail, just hoping...)

[haraken, 2015-07-08 23:34:07]

On 2015/07/08 17:16:45, jsbell wrote:
> Any chance this unblock https://codereview.chromium.org/931543002/ and
> https://codereview.chromium.org/924863003/ ?
> 
> (I need to read it in more detail, just hoping...)

I cannot land https://codereview.chromium.org/931543002/ because we hit the
ASSERT when Blink fails at keeping the wrapper alive (this is a bug). Those
Blink bugs should be fixed and the ASSERT should be enabled. My current plan is
to fix the issue by shipping Oilpan.

The fact that https://codereview.chromium.org/924863003/ has been blocked by
https://codereview.chromium.org/931543002/ implies that
https://codereview.chromium.org/924863003/ is also failing at keeping the
wrapper alive. (On the other hand, the current custom binding in
IDBBindingUtilities seems to be doing a correct thing to keep the wrapper
alive.) In that sense, I think a right thing to do here is not land
https://codereview.chromium.org/924863003/.

[SetWrapperReferenceTo] is not a way to "create the target wrapper and keep it
alive". [SetWrapperReferenceTo] is a way to "keep the target wrapper alive if
the target wrapper exists". It is not a responsibility of
[SetWrapperReferenceFrom/To] to create the wrapper :)