[wrapper-tracing] Support for incrementally tracing ScopedPersistent

[wrapper-tracing] Support for incrementally tracing ScopedPersistent

This CL essentially adds the infrastructure to incrementally trace
ScopedPersistent, including automatic write barriers. The type that should be
used is TraceWrapperScopedPersistent.

Once the dust settles, this type should no longer inherit from ScopedPersistent
and contain a handle that is only weak when traced, i.e., follow regular tracing
semantics.

Drive-by-fixes:
- Only implement tracing of Persistent<v8::Value> as we can cast to this type
  from any child.
- Harden assertions about tracing types.

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

Committed: https://crrev.com/aa50c29e2487ffaaf8ab0ee98f1f84f0efc89016
Cr-Commit-Position: refs/heads/master@{#429617}

[Michael Lippautz, 2016-11-02 16:00:03]

PTAL: This is essentially a polished version of
https://codereview.chromium.org/2458773008/

(TODO: run-bindings-tests --reset-results)

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:125: class
TraceWrapperScopedPersistent : public ScopedPersistent<T> {
The name doesn't really fit. I am all open for any other suggestions.

[Marcel Hlopko, 2016-11-02 16:44:51]

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:125: class
TraceWrapperScopedPersistent : public ScopedPersistent<T> {
On 2016/11/02 at 16:00:03, Michael Lippautz wrote:
> The name doesn't really fit. I am all open for any other suggestions.

TracedV8Reference? It's neither scoped nor persistent, so I'd at least get rid
of those words :)

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:221: if
(!srcObject || !dstObject) {
|| !dstObject->handle?

Or dcheck in TraceWrapperScopedPersistent.set?

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:225: //
that this implicitly disabled the write barrier when  wrapper tracing
...this *is* implicitly...

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:252: //  
DCHECK(m_reporter);
delete commented out code?

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:23: class
raceWrapperScopedPersistent;
*T*raceWrapperScopedPersistent?
It's probably not needed if this compiles fine :)

[Michael Lippautz, 2016-11-02 17:07:56]

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:125: class
TraceWrapperScopedPersistent : public ScopedPersistent<T> {
On 2016/11/02 16:44:51, Marcel Hlopko wrote:
> On 2016/11/02 at 16:00:03, Michael Lippautz wrote:
> > The name doesn't really fit. I am all open for any other suggestions.
> 
> TracedV8Reference? It's neither scoped nor persistent, so I'd at least get rid
> of those words :)

+1 for TracedV8Reference. haraken?

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
(right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:221: if
(!srcObject || !dstObject) {
On 2016/11/02 16:44:51, Marcel Hlopko wrote:
> || !dstObject->handle?
> 
> Or dcheck in TraceWrapperScopedPersistent.set?

Done, here, since we have the full type definition.

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:225: //
that this implicitly disabled the write barrier when  wrapper tracing
On 2016/11/02 16:44:51, Marcel Hlopko wrote:
> ...this *is* implicitly...

Rewrote as ... this implicitly disables the write barrier...

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:252: //  
DCHECK(m_reporter);
On 2016/11/02 16:44:51, Marcel Hlopko wrote:
> delete commented out code?

Already gone.

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:23: class
raceWrapperScopedPersistent;
On 2016/11/02 16:44:51, Marcel Hlopko wrote:
> *T*raceWrapperScopedPersistent?
> It's probably not needed if this compiles fine :)

Was already fixed in latest upload. I swear everything compiles :)

[Marcel Hlopko, 2016-11-02 17:14:39]

lgtm

[haraken, 2016-11-03 05:55:25]

Can we add a test?

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:120: * appropriate
handle type than v8::Persistent. The handle should have regular

I'm just curious but how hard would it be to implement TraceWrapperV8Reference
correctly? It's nasty that we use v8::Persistent, make it weak, but trace it to
keep it alive... It will work, but we're doing a very strange thing.

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:125: class
TraceWrapperScopedPersistent : public ScopedPersistent<T> {
On 2016/11/02 17:07:55, Michael Lippautz wrote:
> On 2016/11/02 16:44:51, Marcel Hlopko wrote:
> > On 2016/11/02 at 16:00:03, Michael Lippautz wrote:
> > > The name doesn't really fit. I am all open for any other suggestions.
> > 
> > TracedV8Reference? It's neither scoped nor persistent, so I'd at least get
rid
> > of those words :)
> 
> +1 for TracedV8Reference. haraken?

I want to keep a "TraceWrapper" prefix at least. TraceWrapperV8Reference?

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:122: * trough
tracing.

through

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:23: class
raceWrapperScopedPersistent;

TraceWrapperScopedPersistent

[Michael Lippautz, 2016-11-03 08:59:54]

Addressed comments. Working on tests now.

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:120: * appropriate
handle type than v8::Persistent. The handle should have regular
On 2016/11/03 05:55:25, haraken wrote:
> 
> I'm just curious but how hard would it be to implement TraceWrapperV8Reference
> correctly? It's nasty that we use v8::Persistent, make it weak, but trace it
to
> keep it alive... It will work, but we're doing a very strange thing.
> 

What I have in mind is a new handle type on the V8 side that can be used by
TraceWrapperV8Reference or maybe even directly. Briefly discussed with jochen@
and he also agrees on adding a new handle type.

In essence, this is already on the radar and will be done in near future.

https://codereview.chromium.org/2474693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:125: class
TraceWrapperScopedPersistent : public ScopedPersistent<T> {
On 2016/11/03 05:55:25, haraken wrote:
> On 2016/11/02 17:07:55, Michael Lippautz wrote:
> > On 2016/11/02 16:44:51, Marcel Hlopko wrote:
> > > On 2016/11/02 at 16:00:03, Michael Lippautz wrote:
> > > > The name doesn't really fit. I am all open for any other suggestions.
> > > 
> > > TracedV8Reference? It's neither scoped nor persistent, so I'd at least get
> rid
> > > of those words :)
> > 
> > +1 for TracedV8Reference. haraken?
> 
> I want to keep a "TraceWrapper" prefix at least. TraceWrapperV8Reference?

Done.

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:122: * trough
tracing.
On 2016/11/03 05:55:25, haraken wrote:
> 
> through

Done.

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2474693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:23: class
raceWrapperScopedPersistent;
On 2016/11/03 05:55:25, haraken wrote:
> 
> TraceWrapperScopedPersistent

Done.

[Michael Lippautz, 2016-11-03 12:55:55]

added tests, ptal

[Michael Lippautz, 2016-11-03 14:03:03]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 14:03:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 14:10:50]

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

[commit-bot: I haz the power, 2016-11-03 14:10:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Michael Lippautz, 2016-11-03 14:15:36]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 14:16:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-11-03 14:25:04]

LGTM

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:127:
TraceWrapperV8Reference(void* parent)

Add explicit.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:131: v8::Isolate*
isolate,

Put v8::Isolate* as the first parameter.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:137:
TraceWrapperV8Reference(void* parent,

I'd remove this constructor unless it's really needed.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:138: v8::Isolate*
isolate,

Ditto.

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

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:226: // is
not active as object will not be marked in this case.

Remove 'Note that...'

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:125: // is
not active as object will not be marked in this case.

Remove the part after 'Note that...'. If the wrapper tracing is disabled, we
will have already returned at line 118.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:95: void
traceWrappers(const T* traceable) const {

Would you help me understand who uses this method? I'm afraid that this method
can be used for 'catch-all' of traceWrappers().

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:99: "only garbage
collected objects can be");

Only garbage collected objects can be used in traceWrappers().

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:142: const
TraceWrapperV8Reference<v8::Value>*) const = 0;

It's inconsistent that traceWrappers(const TraceWrapperMember<T>& t) takes a
reference but traceWrappers(const TraceWrapperV8Reference<v8::Value>*) takes a
pointer.

[Michael Lippautz, 2016-11-03 14:53:41]

Thanks for the detailed review!

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h (right):

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:127:
TraceWrapperV8Reference(void* parent)
On 2016/11/03 14:25:04, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:131: v8::Isolate*
isolate,
On 2016/11/03 14:25:04, haraken wrote:
> 
> Put v8::Isolate* as the first parameter.

Done.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:137:
TraceWrapperV8Reference(void* parent,
On 2016/11/03 14:25:04, haraken wrote:
> 
> I'd remove this constructor unless it's really needed.

Done.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:138: v8::Isolate*
isolate,
On 2016/11/03 14:25:04, haraken wrote:
> 
> Ditto.

Done.

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

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp:226: // is
not active as object will not be marked in this case.
On 2016/11/03 14:25:04, haraken wrote:
> 
> Remove 'Note that...'

Same as above.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h
(right):

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h:125: // is
not active as object will not be marked in this case.
On 2016/11/03 14:25:04, haraken wrote:
> 
> Remove the part after 'Note that...'. If the wrapper tracing is disabled, we
> will have already returned at line 118.

Should've said "... when the _GC_ is not active..."

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:95: void
traceWrappers(const T* traceable) const {
On 2016/11/03 14:25:04, haraken wrote:
> 
> Would you help me understand who uses this method? I'm afraid that this method
> can be used for 'catch-all' of traceWrappers().
> 

Yes, one has to be careful with traceWrappers overloads. It's used for anything
that inherits from TraceWrapperBase. Once we are in the same component we can
improve this signature.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:99: "only garbage
collected objects can be");
On 2016/11/03 14:25:04, haraken wrote:
> 
> Only garbage collected objects can be used in traceWrappers().

Done.

https://codereview.chromium.org/2474693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:142: const
TraceWrapperV8Reference<v8::Value>*) const = 0;
On 2016/11/03 14:25:04, haraken wrote:
> 
> It's inconsistent that traceWrappers(const TraceWrapperMember<T>& t) takes a
> reference but traceWrappers(const TraceWrapperV8Reference<v8::Value>*) takes a
> pointer.

Done.

[Michael Lippautz, 2016-11-03 14:56:43]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-11-03 14:56:43]

The patchset sent to the CQ was uploaded after l-g-t-m from hlopko@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2474693002/#ps180001
(title: "Added bailout for tests when the flag is off")

[commit-bot: I haz the power, 2016-11-03 14:57:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 16:44:22]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-03 17:12:59]

Description was changed from

==========
[wrapper-tracing] Support for incrementally tracing ScopedPersistent

This CL essentially adds the infrastructure to incrementally trace
ScopedPersistent, including automatic write barriers. The type that should be
used is TraceWrapperScopedPersistent.

Once the dust settles, this type should no longer inherit from ScopedPersistent
and contain a handle that is only weak when traced, i.e., follow regular tracing
semantics.

Drive-by-fixes:
- Only implement tracing of Persistent<v8::Value> as we can cast to this type
  from any child.
- Harden assertions about tracing types.

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

to

==========
[wrapper-tracing] Support for incrementally tracing ScopedPersistent

This CL essentially adds the infrastructure to incrementally trace
ScopedPersistent, including automatic write barriers. The type that should be
used is TraceWrapperScopedPersistent.

Once the dust settles, this type should no longer inherit from ScopedPersistent
and contain a handle that is only weak when traced, i.e., follow regular tracing
semantics.

Drive-by-fixes:
- Only implement tracing of Persistent<v8::Value> as we can cast to this type
  from any child.
- Harden assertions about tracing types.

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

Committed: https://crrev.com/aa50c29e2487ffaaf8ab0ee98f1f84f0efc89016
Cr-Commit-Position: refs/heads/master@{#429617}
==========

[commit-bot: I haz the power, 2016-11-03 17:13:00]

Patchset 10 (id:??) landed as
https://crrev.com/aa50c29e2487ffaaf8ab0ee98f1f84f0efc89016
Cr-Commit-Position: refs/heads/master@{#429617}