New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 701295 link

Starred by 1 user
Status: Fixed
Owner:
mlippautz@chromium.org
Closed: Mar 2017
Cc:
jochen@chromium.org
jbroman@chromium.org
█ hlopko@chromium.org
haraken@chromium.org
adithyas@chromium.org
lfg@chromium.org
█ hpayer@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Wrapper visitation cannot deal with multiple inheritance anymore

Project Member Reported by mlippautz@chromium.org, Mar 14 2017 
This is a architectural regression introduced in https://codereview.chromium.org/2728203002/

Example:
class A : public ScriptWrappable {/* defined tracing methods */ };
class B : public TraceWrapperBase { /* defined tracing methods */ };
class C : public A, public B {
 public:
  DECLARE_VIRTUAL_TRACE_WRAPPERS() {
    A::traceWrappers(visitor);
    B::traceWrappers(visitor);
  }
}

Before this CL, each participant of wrapper tracing provided a single virtual method to delegate tracing. A diamond was not a problem because each child class provided its own tracing method that specified how to dispatch to parents. (Write barriers worked by using the outermost parent pointer when creating TraceWrapperMember but this is an orthogonal problem.)

After the CL we are left with marking infrastructure methods on SW and TraceWrapperBase, i.e., they implement markAndDispatchTraceWrappers. In case of a diamond, the compilers fail to compile as the children do not specify how markAndDispatchTraceWrappers should look like. Even if we make it virtual, we still need to provide an implementation for each child that specifies which trace method to dispatch to. This is exactly why we should never put garbage collection infrastructure except tracing methods on the payload objects.

To be honest, if we cannot circumvent that restriction I think we should revert that CL for now as I don't see the immediate benefit. Dealing properly with diamonds and mixins blocks e.g. fixing ErrorEvent where we would need to trace through the ExecutionContext (besides tracing through the regular SW).

Comment 1 by mlippautz@chromium.org, Mar 14 2017 

I also think that I think these special cases are unavoidable and probably an actual blocker for moving the visitors. 

The problem is:
- We cannot add GC infrastructure like markAndDispatchTraceWrappers to the objects without introducing overhead (methods and and additional specification overhead for developers).
- We cannot include NodeRareData and friends in the Visitors because they are in a different component.
- We should still have a special dispatch for them.

In the end I think the cleanest solution would be to take the hit of the additional vtable pointer and just get rid of those special cases. If we cannot do that I think we should still revert.

Comment 2 by mlippautz@chromium.org, Mar 14 2017 

One more braindump: We might get away with special cases just for TraceTrait::traceMarkedWrapper. This is just minimizing the problem thought as there would still be special cases. It's just avoiding them on the inheritance hierarchy of ScriptWrappableVisitor and WrapperVisitor.

Comment 3 by mlippautz@chromium.org, Mar 14 2017

Cc: -mlippautz@chromium.org adithyas@chromium.org
Components: Blink>JavaScript>GC Blink>Bindings
Owner: mlippautz@chromium.org
Status: Started (was: Untriaged)
Proposed https://codereview.chromium.org/2748103002/

Comment 4 by adithyas@chromium.org, Mar 14 2017

Cc: jbroman@chromium.org
Thanks for pointing this out, I didn't really consider the diamond problem (I'm assuming there aren't examples of anything like this, or I'd have seen compiler errors).

I'm fine with the solution you proposed in #3, but I agree that making the special classes inherit from TraceWrapperBase is the cleanest solution. I wanted to initially do that, I don't really know how much we're saving currently by not having an extra vtable pointer.
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96fd6f8611075233b0963e699dfd49305bbb1406

commit 96fd6f8611075233b0963e699dfd49305bbb1406
Author: mlippautz <mlippautz@chromium.org>
Date: Tue Mar 14 15:56:59 2017

[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remove CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG= chromium:701295 

Review-Url: https://codereview.chromium.org/2748103002
Cr-Commit-Position: refs/heads/master@{#456714}

[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h
[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md
[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/core/dom/ElementRareData.h
[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/core/dom/NodeListsNodeData.h
[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/core/dom/NodeRareData.h
[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/platform/heap/GarbageCollected.h
[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/platform/heap/TraceTraits.h
[modify] https://crrev.com/96fd6f8611075233b0963e699dfd49305bbb1406/third_party/WebKit/Source/platform/heap/WrapperVisitor.h

Comment 6 by mlippautz@chromium.org, Mar 15 2017

Status: Fixed (was: Started)
►

Sign in to add a comment