BlinkGCPlugin: require GC mixin instances to declare local trace().

BlinkGCPlugin: require GC mixin instances to declare local trace().

For classes that derive from GarbageCollectedMixin and declare both
adjustAndMark() and isHeapObjectAlive() overrides (typically using
the USING_GARBAGE_COLLECTED_MIXIN() macro), check that the class also
declares a local trace() method override.

This is done so as to require that GC mixin instances do have trace()
in their leftmost vtbl.

R=haraken
BUG=444565
Committed: https://crrev.com/35290f1ba94f90e89492b51567826270c02a81da
Cr-Commit-Position: refs/heads/master@{#310272}

[sof, 2015-01-06 22:23:37]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-01-06 22:23:38]

Please take a look.

I _believe_ this is the check we want&have to provide for mixins, but not 100.0%
sure.

[haraken, 2015-01-07 01:37:51]

LGTM, thanks for improving the plugin!

> I _believe_ this is the check we want&have to provide for mixins, but not
100.0%
> sure.

What part are you suspecting?

[sof, 2015-01-07 08:08:31]

On 2015/01/07 01:37:51, haraken wrote:
> LGTM, thanks for improving the plugin!
> 
> > I _believe_ this is the check we want&have to provide for mixins, but not
> 100.0%
> > sure.
> 
> What part are you suspecting?

Mostly verification that a leftmost trace() is really needed for mixins, given
that we already rely on adjustAndMark() being invoked.

This also doesn't require that mixin trace()s delegate to the empty
GarbageCollectedMixin::trace(). It seems overly fussy to require this.

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

On 2015/01/07 08:08:31, sof wrote:
> On 2015/01/07 01:37:51, haraken wrote:
> > LGTM, thanks for improving the plugin!
> > 
> > > I _believe_ this is the check we want&have to provide for mixins, but not
> > 100.0%
> > > sure.
> > 
> > What part are you suspecting?
> 
> Mostly verification that a leftmost trace() is really needed for mixins, given
> that we already rely on adjustAndMark() being invoked.
> 
> This also doesn't require that mixin trace()s delegate to the empty
> GarbageCollectedMixin::trace(). It seems overly fussy to require this.

Thanks for the clarification; your approach sounds reasonable to me.

[zerny-google, 2015-01-07 08:55:06]

IIRC there is no need for a mixin derived class to declare/override a trace
method (apart for the existing rules) up until the point where the mixin is
combined with an actual GC base. The non-GC allocated mixins can be
independently instantiated so there is no need to require anything of the vtable
wrt the trace method. Once the mixin is combined with an actual GC-derived class
I would think the existing rules would issue an error if there is not a trace
method in the left-most base from that point. At least that was our reasoning
IIRC. Do you have a counter example?

[zerny-google, 2015-01-07 08:56:17]

On 2015/01/07 08:55:06, zerny-google wrote:
> IIRC there is no need for a mixin derived class to declare/override a trace
> method (apart for the existing rules) up until the point where the mixin is
> combined with an actual GC base. The non-GC allocated mixins can be

That should have been: *cannot* be

> independently instantiated so there is no need to require anything of the
vtable
> wrt the trace method. Once the mixin is combined with an actual GC-derived
class
> I would think the existing rules would issue an error if there is not a trace
> method in the left-most base from that point. At least that was our reasoning
> IIRC. Do you have a counter example?

[sof, 2015-01-07 08:57:05]

On 2015/01/07 08:56:17, zerny-google wrote:
> On 2015/01/07 08:55:06, zerny-google wrote:
> > IIRC there is no need for a mixin derived class to declare/override a trace
> > method (apart for the existing rules) up until the point where the mixin is
> > combined with an actual GC base. The non-GC allocated mixins can be
> 
> That should have been: *cannot* be
> 
> > independently instantiated so there is no need to require anything of the
> vtable
> > wrt the trace method. Once the mixin is combined with an actual GC-derived
> class
> > I would think the existing rules would issue an error if there is not a
trace
> > method in the left-most base from that point. At least that was our
reasoning
> > IIRC. Do you have a counter example?

I guess https://codereview.chromium.org/835363003/ are counter examples where
errors weren't issued.

[zerny-google, 2015-01-07 09:51:48]

On 2015/01/07 08:57:05, sof wrote:
> On 2015/01/07 08:56:17, zerny-google wrote:
> > On 2015/01/07 08:55:06, zerny-google wrote:
> > > IIRC there is no need for a mixin derived class to declare/override a
trace
> > > method (apart for the existing rules) up until the point where the mixin
is
> > > combined with an actual GC base. The non-GC allocated mixins can be
> > 
> > That should have been: *cannot* be
> > 
> > > independently instantiated so there is no need to require anything of the
> > vtable
> > > wrt the trace method. Once the mixin is combined with an actual GC-derived
> > class
> > > I would think the existing rules would issue an error if there is not a
> trace
> > > method in the left-most base from that point. At least that was our
> reasoning
> > > IIRC. Do you have a counter example?
> 
> I guess https://codereview.chromium.org/835363003/ are counter examples where
> errors weren't issued.

Hmmm. Those cases should be caught by CheckPolymorphicClass in
BlinkGCPlugin.cpp. I think this is a bug due to the TODO on line:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/clang/blink_...

For the AudioTrack case, trace will be null and "Condition (1)" on line 1090
does not trigger because trace is null instead of being the inherited trace from
TrackBase. Implementing a GetInheritedTraceMethod should fix the issue.

That said, the AudioTrack case is actually safe since the vtbl of AudioTrack
will start as the vtbl of TrackBase. Regardless, we should emit an error here I
think.

[sof, 2015-01-07 12:11:57]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-01-07 12:12:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/834373003/40001

[commit-bot: I haz the power, 2015-01-07 12:53:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-07 12:54:40]

Patchset 3 (id:??) landed as
https://crrev.com/35290f1ba94f90e89492b51567826270c02a81da
Cr-Commit-Position: refs/heads/master@{#310272}