[v8 gc] Introduce a base class for all objects that can have pending activity

[v8 gc] Introduce a base class for all objects that can have pending activity

When we trace the C++ heap from V8, we have to have a way to find all
objects that can keep themselves alive. Since most hasPendingActivity()
implementations don't have an easy way to detect when the activity state
changes, we keep a list that is scanned during the atomic pause.

BUG=468240
R=hpayer@chromium.org,haraken@chromium.org,hlopko@chromium.org

Committed: https://crrev.com/6bc1ec987cdb07ded7b766632eeef3eb8a6bec7e
Cr-Commit-Position: refs/heads/master@{#382254}

[jochen (gone - plz use gerrit), 2016-03-16 08:43:24]

Description was changed from

==========
Introduce a base class for all objects that can have pending activity

When we trace the C++ heap from V8, we have to have a way to find all
objects that can keep themselves alive. Since most hasPendingActivity()
implementations don't have an easy way to detect when the activity state
changes, we keep a list that is scanned during the atomic pause.

BUG=468240
R=hpayer@chromium.org,haraken@chromium.org,hlopko@chromium.org
==========

to

==========
[v8 gc] Introduce a base class for all objects that can have pending activity

When we trace the C++ heap from V8, we have to have a way to find all
objects that can keep themselves alive. Since most hasPendingActivity()
implementations don't have an easy way to detect when the activity state
changes, we keep a list that is scanned during the atomic pause.

BUG=468240
R=hpayer@chromium.org,haraken@chromium.org,hlopko@chromium.org
==========

[jochen (gone - plz use gerrit), 2016-03-16 08:46:38]

ptal

I know that Hannes doesn't like the name - better proposals welcome :)

https://codereview.chromium.org/1805843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCRoot.cpp (right):

https://codereview.chromium.org/1805843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCRoot.cpp:25: gcRoots().add(this);
we might have to add a ScriptWrappable* parameter to the ctor and store that as
well, as we'll need the ScriptWrappable, and there's no way to cast from
V8GCRoot to ScriptWrappable otherwise

[Marcel Hlopko, 2016-03-16 09:07:40]

https://codereview.chromium.org/1805843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCRoot.cpp (right):

https://codereview.chromium.org/1805843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCRoot.cpp:25: gcRoots().add(this);
I will certainly need a way to get to the ScriptWrappable somehow, ctor
parameter sounds good to me. Any disadvantages that I don't see?

[Yuki, 2016-03-16 10:38:53]

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

[Yuki, 2016-03-16 10:38:54]

How were the classes listed in this CL selected?  Maybe it's the classes that
have its own hasPendingActivity()?

I'm considering how we can guarantee that the target classes correctly inherit
from V8GCRoot, i.e. no one forgot to make it inherit from V8GCRoot when defining
a new class.

If the target classes are those which have [DependentLifetime] in the IDL files,
I think we can generate compile-time test code in V8TargetClass.cpp in order to
ensure that it inherits from V8GCRoot.

[haraken, 2016-03-16 11:54:55]

My suggestion would be:

class ScriptWrappable {
  ...; // Don't have hasPendingActivity.
};

class ActiveScriptWrappable : public ScriptWrappable {
  virtual hasPendingActivity() { return false; }
};

class HTMLMediaElement : public ActiveScriptWrappable {
  virtual hasPendingActivity() { return ...; }
};

Also we might want to rename hasPendingActivity to keepWrapperAlive
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/UwRGJgLBElo/mFL68oKs...).

[jochen (gone - plz use gerrit), 2016-03-16 11:59:50]

On 2016/03/16 at 11:54:55, haraken wrote:
> My suggestion would be:
> 
> class ScriptWrappable {
>   ...; // Don't have hasPendingActivity.
> };
> 
> class ActiveScriptWrappable : public ScriptWrappable {
>   virtual hasPendingActivity() { return false; }
> };
> 
> class HTMLMediaElement : public ActiveScriptWrappable {
>   virtual hasPendingActivity() { return ...; }
> };
> 
> Also we might want to rename hasPendingActivity to keepWrapperAlive
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/UwRGJgLBElo/mFL68oKs...).

that requires reintroducing toActiveScriptWrappable (formerly toActiveDOMObject)

[jochen (gone - plz use gerrit), 2016-03-16 12:00:05]

which i guess is fine

[haraken, 2016-03-16 13:43:05]

On 2016/03/16 11:59:50, jochen wrote:
> On 2016/03/16 at 11:54:55, haraken wrote:
> > My suggestion would be:
> > 
> > class ScriptWrappable {
> >   ...; // Don't have hasPendingActivity.
> > };
> > 
> > class ActiveScriptWrappable : public ScriptWrappable {
> >   virtual hasPendingActivity() { return false; }
> > };
> > 
> > class HTMLMediaElement : public ActiveScriptWrappable {
> >   virtual hasPendingActivity() { return ...; }
> > };
> > 
> > Also we might want to rename hasPendingActivity to keepWrapperAlive
>
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/UwRGJgLBElo/mFL68oKs...).
> 
> that requires reintroducing toActiveScriptWrappable (formerly
toActiveDOMObject)

Ah, got it. Yeah, reintroducing toActiveScriptWrappable would be an option.

Another option would be:

class ScriptWrappable {
  virtual bool hasPendingActivity() { return false; }
};

class ActiveScriptWrappable : public ScriptWrappable {

};

class HTMLMediaElement : public ActiveScriptWrappable {
  virtual bool hasPendingActivity() { ...; }
};

and, in V8GCController (or somewhere), add
ASSERT(wrapper->isActiveScriptWrappable() || !wrapper->hasPendingActivity()).
That way we can dynamically detect an object that doesn't inherit from
ActiveScriptWrappable but uses hasPendingActivity.

[Hannes Payer (out of office), 2016-03-16 13:52:30]

On 2016/03/16 13:43:05, haraken wrote:
> On 2016/03/16 11:59:50, jochen wrote:
> > On 2016/03/16 at 11:54:55, haraken wrote:
> > > My suggestion would be:
> > > 
> > > class ScriptWrappable {
> > >   ...; // Don't have hasPendingActivity.
> > > };
> > > 
> > > class ActiveScriptWrappable : public ScriptWrappable {
> > >   virtual hasPendingActivity() { return false; }
> > > };
> > > 
> > > class HTMLMediaElement : public ActiveScriptWrappable {
> > >   virtual hasPendingActivity() { return ...; }
> > > };
> > > 
> > > Also we might want to rename hasPendingActivity to keepWrapperAlive
> >
>
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/UwRGJgLBElo/mFL68oKs...).
> > 
> > that requires reintroducing toActiveScriptWrappable (formerly
> toActiveDOMObject)
> 
> Ah, got it. Yeah, reintroducing toActiveScriptWrappable would be an option.
> 
> Another option would be:
> 
> class ScriptWrappable {
>   virtual bool hasPendingActivity() { return false; }
> };
> 
> class ActiveScriptWrappable : public ScriptWrappable {
> 
> };
> 
> class HTMLMediaElement : public ActiveScriptWrappable {
>   virtual bool hasPendingActivity() { ...; }
> };
> 
> and, in V8GCController (or somewhere), add
> ASSERT(wrapper->isActiveScriptWrappable() || !wrapper->hasPendingActivity()).
> That way we can dynamically detect an object that doesn't inherit from
> ActiveScriptWrappable but uses hasPendingActivity.

As discussed offline: V8GCRoot may be misleading since this list does not
contain all roots for the GC. It sounds like that it is mostly used in the
context of finalization, i.e. we iterate over the list and revive dead objects.
I would change the name to V8GCFinalizers or something like that to be more
precise.

[haraken, 2016-03-16 14:05:16]

On 2016/03/16 13:52:30, Hannes Payer - slow wrote:
> On 2016/03/16 13:43:05, haraken wrote:
> > On 2016/03/16 11:59:50, jochen wrote:
> > > On 2016/03/16 at 11:54:55, haraken wrote:
> > > > My suggestion would be:
> > > > 
> > > > class ScriptWrappable {
> > > >   ...; // Don't have hasPendingActivity.
> > > > };
> > > > 
> > > > class ActiveScriptWrappable : public ScriptWrappable {
> > > >   virtual hasPendingActivity() { return false; }
> > > > };
> > > > 
> > > > class HTMLMediaElement : public ActiveScriptWrappable {
> > > >   virtual hasPendingActivity() { return ...; }
> > > > };
> > > > 
> > > > Also we might want to rename hasPendingActivity to keepWrapperAlive
> > >
> >
>
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/UwRGJgLBElo/mFL68oKs...).
> > > 
> > > that requires reintroducing toActiveScriptWrappable (formerly
> > toActiveDOMObject)
> > 
> > Ah, got it. Yeah, reintroducing toActiveScriptWrappable would be an option.
> > 
> > Another option would be:
> > 
> > class ScriptWrappable {
> >   virtual bool hasPendingActivity() { return false; }
> > };
> > 
> > class ActiveScriptWrappable : public ScriptWrappable {
> > 
> > };
> > 
> > class HTMLMediaElement : public ActiveScriptWrappable {
> >   virtual bool hasPendingActivity() { ...; }
> > };
> > 
> > and, in V8GCController (or somewhere), add
> > ASSERT(wrapper->isActiveScriptWrappable() ||
!wrapper->hasPendingActivity()).
> > That way we can dynamically detect an object that doesn't inherit from
> > ActiveScriptWrappable but uses hasPendingActivity.
> 
> As discussed offline: V8GCRoot may be misleading since this list does not
> contain all roots for the GC. It sounds like that it is mostly used in the
> context of finalization, i.e. we iterate over the list and revive dead
objects.
> I would change the name to V8GCFinalizers or something like that to be more
> precise.

I don't have a strong opinion about the name, but I don't want to decouple
hasPendingActivity() from the V8GCRoot class. hasPendingActivity() should be a
method of the V8GCRoot class.

[jochen (gone - plz use gerrit), 2016-03-17 16:55:35]

ptal

There's now an ActiveScriptWrappable. it doesn't inherit from ScriptWrappable,
as sometimes base classes are ScriptWrappable (like EventTarget) but only some
of the derived classes are active.

I also replaced a bunch of calls to ScriptWrappable::hasPendingActivity() with
false. Because that's what it'll always return.

[Yuki, 2016-03-18 06:59:40]

LGTM.

However, I'm a little afraid that this CL may increase the binary size of Blink
because we have a thousand instances of WrapperTypeInfo.  Adding 8-byte pointer
in WrapperTypeInfo may cause a regression.  (IIRC, I hit it before.)

This is just FYI.  I'm okay to go with this CL.

Just in case, options in my mind are:
1) put hasPendingActivity() back into ScriptWrappable so that we don't need
WrapperTypeInfo::toActiveScriptWrappable.
2) combine refObject and derefObject in WrapperTypeInfo such like
  refOrDerefObject(wrapper, refOrDerefFlag)

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:123: if (type !=
npObjectTypeInfo() && toActiveScriptWrappable(wrapper) &&
toActiveScriptWrappable(wrapper)->hasPendingActivity()) {
I think you can do:
    type != npObjectTypeInfo() &&
    type->toActiveScriptWrappable(wrapper) &&
    type->toActiveScriptWrappable(wrapper)->hasPendingActivity()
and then, you don't need the new global function:
    toActiveScriptWrappable(wrapper)

In addition, I think we'd like to have hasPendingActivity() in WrapperTypeInfo.
    type != npObjectTypeInfo() &&
    type->hasPendingActivity(wrapper)
then, we no longer need to call toActiveScriptWrappable() twice.  It should be
faster.

[haraken, 2016-03-18 08:07:28]

LGTM

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1199: ###
[RequiresFinalizer] _(i)_

"RequiresFinalizer" sounds a bit confusing, since it implies that the DOM object
needs a finalizer or something. I'd rename it to "ActiveScriptWrappable".

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:56:
RequiresFinalizer

Alphabetical order.

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
(right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:19:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<FinalizerHashMap>, finalizerMap,
new ThreadSpecific<FinalizerHashMap>());

I'm just curious but why do you call this a "finalizer" list?

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:35:
ScriptWrappable* ActiveScriptWrappable::toScriptWrappable() const

For performance reasons, would it be better to make ActiveScriptWrappable hold a
raw pointer to ScriptWrappable?

Then the HashMap could be a HashSet.

[Marcel Hlopko, 2016-03-18 08:34:40]

lgtm, but maybe we could discuss the naming a bit?

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1222: If a given DOM
object needs to be kept alive as long as the DOM object has pending activitiesi,
yoiu need to specify `[RequiresFinalizer]` and `[DependentLifetime]`. For
example, `[RequiresFinalizer]` can be used when the DOM object is expecting
events ot be raised in the future.
typo activitiesi, yoiu

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1222: If a given DOM
object needs to be kept alive as long as the DOM object has pending activitiesi,
yoiu need to specify `[RequiresFinalizer]` and `[DependentLifetime]`. For
example, `[RequiresFinalizer]` can be used when the DOM object is expecting
events ot be raised in the future.
typo ot

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:487: bool
V8GCController::hasPendingActivity(v8::Isolate* isolate, ExecutionContext*
executionContext)
Pardon my lack of knowledge of the system, but I'm wondering whether we need
isolate as a parameter here, can't we just take the "current" one? Can one
isolate ask if other isolate hasPendingActivity? Feel free to educate me offline
if it's not worth typing :)

[Marcel Hlopko, 2016-03-18 10:12:38]

So we discussed with Hannes and agreed that both root and finalization are
misleading. But the only better name we could came up with was some variation
with the 'active(root|object|wrappable|thingy)'. Would you say that in blink
codebase the word 'active' carries the meaning of 'object which controls its
liveness? Do you have a better name for the concept?

[Yuki, 2016-03-18 10:38:22]

On 2016/03/18 10:12:38, hlopko wrote:
> So we discussed with Hannes and agreed that both root and finalization are
> misleading. But the only better name we could came up with was some variation
> with the 'active(root|object|wrappable|thingy)'. Would you say that in blink
> codebase the word 'active' carries the meaning of 'object which controls its
> liveness? Do you have a better name for the concept?

Among the list, I'd prefer ActiveWrappable.  Haraken suggested
ActiveScriptWrappable in the previous reply and I'm okay with it.

Given that they're "platform objects" in the spec's words, ActivePlatformObject
would be another option, I think.  I feel that "ActiveObject" is too much vague
about what "Object" means.

[Marcel Hlopko, 2016-03-18 10:43:11]

+1 to everything Yuki said

[jochen (gone - plz use gerrit), 2016-03-18 10:46:58]

ActiveScriptWrappable it is then - that way, you don't have to search around in
the source so much to figure out what it means.

[Yuki, 2016-03-18 10:48:49]

On 2016/03/18 10:46:58, jochen wrote:
> ActiveScriptWrappable it is then - that way, you don't have to search around
in
> the source so much to figure out what it means.

+1.  Thanks for renaming it again and again.

[Hannes Payer (out of office), 2016-03-18 12:27:01]

lgtm

[haraken, 2016-03-18 12:51:29]

ActiveScriptWrappable SGTM.

[jochen (gone - plz use gerrit), 2016-03-18 13:18:02]

all comments addressed, assuming the bots like me better this time, i'll commit

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLExtendedAttributes.md (right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLExtendedAttributes.md:1222: If a given DOM
object needs to be kept alive as long as the DOM object has pending activitiesi,
yoiu need to specify `[RequiresFinalizer]` and `[DependentLifetime]`. For
example, `[RequiresFinalizer]` can be used when the DOM object is expecting
events ot be raised in the future.
On 2016/03/18 at 08:34:40, hlopko wrote:
> typo activitiesi, yoiu

done

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
(right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:19:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<FinalizerHashMap>, finalizerMap,
new ThreadSpecific<FinalizerHashMap>());
On 2016/03/18 at 08:07:28, haraken wrote:
> I'm just curious but why do you call this a "finalizer" list?

rename to activeScriptWrappables

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:35:
ScriptWrappable* ActiveScriptWrappable::toScriptWrappable() const
On 2016/03/18 at 08:07:28, haraken wrote:
> For performance reasons, would it be better to make ActiveScriptWrappable hold
a raw pointer to ScriptWrappable?
> 
> Then the HashMap could be a HashSet.

done

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:123: if (type !=
npObjectTypeInfo() && toActiveScriptWrappable(wrapper) &&
toActiveScriptWrappable(wrapper)->hasPendingActivity()) {
On 2016/03/18 at 06:59:40, Yuki wrote:
> I think you can do:
>     type != npObjectTypeInfo() &&
>     type->toActiveScriptWrappable(wrapper) &&
>     type->toActiveScriptWrappable(wrapper)->hasPendingActivity()
> and then, you don't need the new global function:
>     toActiveScriptWrappable(wrapper)
> 
> In addition, I think we'd like to have hasPendingActivity() in
WrapperTypeInfo.
>     type != npObjectTypeInfo() &&
>     type->hasPendingActivity(wrapper)
> then, we no longer need to call toActiveScriptWrappable() twice.  It should be
faster.

added hasPendingActivity()

https://codereview.chromium.org/1805843002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:487: bool
V8GCController::hasPendingActivity(v8::Isolate* isolate, ExecutionContext*
executionContext)
On 2016/03/18 at 08:34:40, hlopko wrote:
> Pardon my lack of knowledge of the system, but I'm wondering whether we need
isolate as a parameter here, can't we just take the "current" one? Can one
isolate ask if other isolate hasPendingActivity? Feel free to educate me offline
if it's not worth typing :)

the embedder should always know which isolate it wants to access

[jochen (gone - plz use gerrit), 2016-03-20 20:18:33]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-03-20 20:18:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, hpayer@chromium.org, hlopko@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1805843002/#ps120001
(title: "updates")

[commit-bot: I haz the power, 2016-03-20 20:18:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1805843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1805843002/120001

[commit-bot: I haz the power, 2016-03-20 22:24:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-20 22:24:56]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[jochen (gone - plz use gerrit), 2016-03-21 05:32:06]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2016-03-21 05:32:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1805843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1805843002/120001

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

yukishiino@, thanks for the warnings about binary size. I decided to go with the
additional field anyways, as it just reintroduces the toActiveDOMObject that
haraken@ recently removed. It's sad that the size savings are undone, but I
expect that the result will be acceptable as it was before.

[commit-bot: I haz the power, 2016-03-21 08:08:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-21 08:10:12]

Description was changed from

==========
[v8 gc] Introduce a base class for all objects that can have pending activity

When we trace the C++ heap from V8, we have to have a way to find all
objects that can keep themselves alive. Since most hasPendingActivity()
implementations don't have an easy way to detect when the activity state
changes, we keep a list that is scanned during the atomic pause.

BUG=468240
R=hpayer@chromium.org,haraken@chromium.org,hlopko@chromium.org
==========

to

==========
[v8 gc] Introduce a base class for all objects that can have pending activity

When we trace the C++ heap from V8, we have to have a way to find all
objects that can keep themselves alive. Since most hasPendingActivity()
implementations don't have an easy way to detect when the activity state
changes, we keep a list that is scanned during the atomic pause.

BUG=468240
R=hpayer@chromium.org,haraken@chromium.org,hlopko@chromium.org

Committed: https://crrev.com/6bc1ec987cdb07ded7b766632eeef3eb8a6bec7e
Cr-Commit-Position: refs/heads/master@{#382254}
==========

[commit-bot: I haz the power, 2016-03-21 08:10:15]

Patchset 7 (id:??) landed as
https://crrev.com/6bc1ec987cdb07ded7b766632eeef3eb8a6bec7e
Cr-Commit-Position: refs/heads/master@{#382254}

[sof, 2016-05-10 05:22:24]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-05-10 05:22:26]

https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
(right):

https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:31:
ActiveScriptWrappable::~ActiveScriptWrappable()
haraken@: is it guaranteed that this global set of bare pointers never contain
anything that's about to be (lazily) swept, and will not be visited when tracing
ScriptWrappables?

[jochen (gone - plz use gerrit), 2016-05-10 05:26:06]

On 2016/05/10 at 05:22:26, sigbjornf wrote:
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
(right):
> 
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:31:
ActiveScriptWrappable::~ActiveScriptWrappable()
> haraken@: is it guaranteed that this global set of bare pointers never contain
anything that's about to be (lazily) swept, and will not be visited when tracing
ScriptWrappables?

Why do you need this guarantee?

[sof, 2016-05-10 05:37:01]

On 2016/05/10 05:26:06, jochen wrote:
> On 2016/05/10 at 05:22:26, sigbjornf wrote:
> >
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:31:
> ActiveScriptWrappable::~ActiveScriptWrappable()
> > haraken@: is it guaranteed that this global set of bare pointers never
contain
> anything that's about to be (lazily) swept, and will not be visited when
tracing
> ScriptWrappables?
> 
> Why do you need this guarantee?

Touching dying objects risks touching dead ones, so at least knowing that this
unmanaged set of heap pointers is safe, would be good. I couldn't find the
argument behind that here.

[sof, 2016-05-10 06:40:44]

On 2016/05/10 05:37:01, sof wrote:
> On 2016/05/10 05:26:06, jochen wrote:
> > On 2016/05/10 at 05:22:26, sigbjornf wrote:
> > >
> >
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:31:
> > ActiveScriptWrappable::~ActiveScriptWrappable()
> > > haraken@: is it guaranteed that this global set of bare pointers never
> contain
> > anything that's about to be (lazily) swept, and will not be visited when
> tracing
> > ScriptWrappables?
> > 
> > Why do you need this guarantee?
> 
> Touching dying objects risks touching dead ones, so at least knowing that this
> unmanaged set of heap pointers is safe, would be good. I couldn't find the
> argument behind that here.

Letting the Oilpan GC handle the accurate liveness of ActiveScriptWrappables is
an alternative, see https://codereview.chromium.org/1962113003/

[jochen (gone - plz use gerrit), 2016-05-10 06:43:18]

On 2016/05/10 at 06:40:44, sigbjornf wrote:
> On 2016/05/10 05:37:01, sof wrote:
> > On 2016/05/10 05:26:06, jochen wrote:
> > > On 2016/05/10 at 05:22:26, sigbjornf wrote:
> > > >
> > >
> >
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > > > File
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
> > > (right):
> > > > 
> > > >
> > >
> >
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:31:
> > > ActiveScriptWrappable::~ActiveScriptWrappable()
> > > > haraken@: is it guaranteed that this global set of bare pointers never
> > contain
> > > anything that's about to be (lazily) swept, and will not be visited when
> > tracing
> > > ScriptWrappables?
> > > 
> > > Why do you need this guarantee?
> > 
> > Touching dying objects risks touching dead ones, so at least knowing that
this
> > unmanaged set of heap pointers is safe, would be good. I couldn't find the
> > argument behind that here.
> 
> Letting the Oilpan GC handle the accurate liveness of ActiveScriptWrappables
is an alternative, see https://codereview.chromium.org/1962113003/

sgtm

[haraken, 2016-05-10 07:08:27]

On 2016/05/10 06:43:18, jochen wrote:
> On 2016/05/10 at 06:40:44, sigbjornf wrote:
> > On 2016/05/10 05:37:01, sof wrote:
> > > On 2016/05/10 05:26:06, jochen wrote:
> > > > On 2016/05/10 at 05:22:26, sigbjornf wrote:
> > > > >
> > > >
> > >
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > > > > File
> third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp
> > > > (right):
> > > > > 
> > > > >
> > > >
> > >
>
https://codereview.chromium.org/1805843002/diff/120001/third_party/WebKit/Sou...
> > > > >
third_party/WebKit/Source/bindings/core/v8/ActiveScriptWrappable.cpp:31:
> > > > ActiveScriptWrappable::~ActiveScriptWrappable()
> > > > > haraken@: is it guaranteed that this global set of bare pointers never
> > > contain
> > > > anything that's about to be (lazily) swept, and will not be visited when
> > > tracing
> > > > ScriptWrappables?
> > > > 
> > > > Why do you need this guarantee?
> > > 
> > > Touching dying objects risks touching dead ones, so at least knowing that
> this
> > > unmanaged set of heap pointers is safe, would be good. I couldn't find the
> > > argument behind that here.
> > 
> > Letting the Oilpan GC handle the accurate liveness of ActiveScriptWrappables
> is an alternative, see https://codereview.chromium.org/1962113003/
> 
> sgtm

Sounds good.