[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
Committed: https://crrev.com/589b820cd653d34fb6bd3da77f147292b480533e
Cr-Commit-Position: refs/heads/master@{#430295}

[Michael Lippautz, 2016-11-07 10:35:49]

Description was changed from

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untie TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
==========

to

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
==========

[Michael Lippautz, 2016-11-07 10:36:12]

mlippautz@chromium.org changed reviewers:
+ haraken@chromium.org, hlopko@chromium.org

[Michael Lippautz, 2016-11-07 10:36:13]

ptal

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

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h:107: const
ScopedPersistent<S>& cast() const {
Dead code.

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

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:66: const
TraceWrapperV8Reference<S>& cast() const {
This cast is not safe when we have vtable + templates (as we would just
reinterpret the vtable pointer).

However, if we want to avoid this cast, we need 3 dispatchers for markWrapper
(v8::Object, v8::Value, v8::Function), and potentially even more in future.

Since we anyways planned to untangle TraceWrapperV8Reference from
ScopedPersistent I think this is the better way to go.

wdyt?

[Michael Lippautz, 2016-11-07 10:54:11]

Description was changed from

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
==========

to

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303

TODO: run-bindings-tests --reset-results
==========

[Marcel Hlopko, 2016-11-07 14:16:09]

lgtm

[haraken, 2016-11-07 14:27:58]

LGTM with comments.

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

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:15: *
unless traced.

If the wrapper tracing is disabled, the reference is a weak ScopedPersistent.
Otherwise, the reference is (strongly) traced by the wrapper tracing.

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:23: class
TraceWrapperV8Reference /*: public ScopedPersistent<T> */ {

Remove the comment.

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:30: :
m_handle(isolate, handle), m_parent(parent) {

It would be better to use internalSet. Then you can remove the below write
barrier.

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:31:
m_handle.SetWeak();

Can we use SetPhantom?

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:39:
m_handle.SetWeak();

Can we use SetPhantom?

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:62:
isolate->SetReference(parent, m_handle);

This should not be called if the wrapper tracing is enabled. Can we add an
DCHECK about it?

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:66: const
TraceWrapperV8Reference<S>& cast() const {
On 2016/11/07 10:36:13, Michael Lippautz wrote:
> This cast is not safe when we have vtable + templates (as we would just
> reinterpret the vtable pointer).
> 
> However, if we want to avoid this cast, we need 3 dispatchers for markWrapper
> (v8::Object, v8::Value, v8::Function), and potentially even more in future.
> 
> Since we anyways planned to untangle TraceWrapperV8Reference from
> ScopedPersistent I think this is the better way to go.
> 
> wdyt?

Sounds reasonable to me.

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

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:113: //
m_listener.setWeak(this, &wrapperCleared);

Remove this?

[Michael Lippautz, 2016-11-07 15:07:15]

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

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:15: *
unless traced.
On 2016/11/07 14:27:58, haraken wrote:
> 
> If the wrapper tracing is disabled, the reference is a weak ScopedPersistent.
> Otherwise, the reference is (strongly) traced by the wrapper tracing.

Done.

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:23: class
TraceWrapperV8Reference /*: public ScopedPersistent<T> */ {
On 2016/11/07 14:27:58, haraken wrote:
> 
> Remove the comment.

Done.

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:30: :
m_handle(isolate, handle), m_parent(parent) {
On 2016/11/07 14:27:58, haraken wrote:
> 
> It would be better to use internalSet. Then you can remove the below write
> barrier.
> 

Done.

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:31:
m_handle.SetWeak();
On 2016/11/07 14:27:58, haraken wrote:
> 
> Can we use SetPhantom?

SetWeak() without callbacks implements the phantom semantics. 

(It's exactly what ScopedPersistent::setPhantom() does.)

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:39:
m_handle.SetWeak();
On 2016/11/07 14:27:58, haraken wrote:
> 
> Can we use SetPhantom?

See above.

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperV8Reference.h:62:
isolate->SetReference(parent, m_handle);
On 2016/11/07 14:27:58, haraken wrote:
> 
> This should not be called if the wrapper tracing is enabled. Can we add an
> DCHECK about it?

Done.

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

https://codereview.chromium.org/2479133002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:113: //
m_listener.setWeak(this, &wrapperCleared);
On 2016/11/07 14:27:58, haraken wrote:
> 
> Remove this?

Done.

[Michael Lippautz, 2016-11-07 15:08:04]

Description was changed from

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303

TODO: run-bindings-tests --reset-results
==========

to

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
==========

[Michael Lippautz, 2016-11-07 15:08:12]

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

[commit-bot: I haz the power, 2016-11-07 15:08:26]

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-07 16:43:49]

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

[commit-bot: I haz the power, 2016-11-07 16:43:50]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2016-11-07 16:46:11]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-11-07 16:46:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, hlopko@chromium.org
Link to the patchset: https://codereview.chromium.org/2479133002/#ps60001
(title: "Regenerate bindings results")

[commit-bot: I haz the power, 2016-11-07 16:46:33]

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-07 16:51:28]

Description was changed from

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
==========

to

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
==========

[commit-bot: I haz the power, 2016-11-07 16:51:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-07 17:13:32]

Description was changed from

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303
==========

to

==========
[wrapper-tracing] Fix dispatching of TraceWrapperv8Reference

Untangle TraceWrapperV8Reference from ScopedPersistent. This is required as the
combination of vtable and templates makes it impossible to properly cast to the
needed type.

BUG=chromium:662303

Committed: https://crrev.com/589b820cd653d34fb6bd3da77f147292b480533e
Cr-Commit-Position: refs/heads/master@{#430295}
==========

[commit-bot: I haz the power, 2016-11-07 17:13:33]

Patchset 4 (id:??) landed as
https://crrev.com/589b820cd653d34fb6bd3da77f147292b480533e
Cr-Commit-Position: refs/heads/master@{#430295}