Blink GC plugin: Gracefully handle cyclic part objects.

Blink GC plugin: Gracefully handle cyclic part objects.

When checking a class if it or its part objects don't define GC roots,
avoid looping on part objects that are cyclic. i.e., the part object
containing a collection that refers back to itself.

R=haraken
BUG=424962
Committed: https://crrev.com/fa4843d2396ab7540007d5efaf18c767cf92ce5d
Cr-Commit-Position: refs/heads/master@{#300256}

[sof, 2014-10-17 16:53:29]

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

[sof, 2014-10-17 16:53:30]

Please take a look.

Running into this with ContextMenuItem.

(While here, I also updated the expected test output for the crash_on_invalid
test.)

[haraken, 2014-10-17 17:18:10]

LGTM

[sof, 2014-10-17 18:41:11]

Cc: +thakis, hans

fyi - this crasher doesn't currently show with Blink's ToT (*), but it'd be good
to have the fix be considered for the next clang roll.

* - GC_PLUGIN_IGNORE() annotations can be used to work around the bug, should a
recursive "part object" start being used from a GCed class. One place where such
a thing will show up soon is for context menu objects and their use of the
recursive ContextMenuItem.

[hans, 2014-10-17 18:48:54]

On 2014/10/17 18:41:11, sof wrote:
> Cc: +thakis, hans
> 
> fyi - this crasher doesn't currently show with Blink's ToT (*), but it'd be
good
> to have the fix be considered for the next clang roll.
> 
> * - GC_PLUGIN_IGNORE() annotations can be used to work around the bug, should
a
> recursive "part object" start being used from a GCed class. One place where
such
> a thing will show up soon is for context menu objects and their use of the
> recursive ContextMenuItem.

Thanks! I just landed a Clang roll this week, and I'll make sure this patch is
in when I build packages for the next roll.

[sof, 2014-10-20 12:34:38]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-20 12:35:04]

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

[commit-bot: I haz the power, 2014-10-20 12:59:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-20 13:00:18]

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

[sof, 2014-11-19 10:59:23]

On 2014/10/17 18:48:54, hans wrote:
> On 2014/10/17 18:41:11, sof wrote:
> > Cc: +thakis, hans
> > 
> > fyi - this crasher doesn't currently show with Blink's ToT (*), but it'd be
> good
> > to have the fix be considered for the next clang roll.
> > 
> > * - GC_PLUGIN_IGNORE() annotations can be used to work around the bug,
should
> a
> > recursive "part object" start being used from a GCed class. One place where
> such
> > a thing will show up soon is for context menu objects and their use of the
> > recursive ContextMenuItem.
> 
> Thanks! I just landed a Clang roll this week, and I'll make sure this patch is
> in when I build packages for the next roll.

Looks like the last clang roll (2014-11-12) didn't include this?

[Nico, 2014-11-19 17:42:25]

On Wed, Nov 19, 2014 at 2:59 AM, <sigbjornf@opera.com> wrote:

> On 2014/10/17 18:48:54, hans wrote:
>
>> On 2014/10/17 18:41:11, sof wrote:
>> > Cc: +thakis, hans
>> >
>> > fyi - this crasher doesn't currently show with Blink's ToT (*), but
>> it'd be
>> good
>> > to have the fix be considered for the next clang roll.
>> >
>> > * - GC_PLUGIN_IGNORE() annotations can be used to work around the bug,
>>
> should
>
>> a
>> > recursive "part object" start being used from a GCed class. One place
>> where
>> such
>> > a thing will show up soon is for context menu objects and their use of
>> the
>> > recursive ContextMenuItem.
>>
>
>  Thanks! I just landed a Clang roll this week, and I'll make sure this
>> patch is
>> in when I build packages for the next roll.
>>
>
> Looks like the last clang roll (2014-11-12) didn't include this?
>

That roll got reverted in #304109


>
> https://codereview.chromium.org/666473003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof, 2014-12-01 19:39:29]

On 2014/11/19 17:42:25, Nico wrote:
> On Wed, Nov 19, 2014 at 2:59 AM, <mailto:sigbjornf@opera.com> wrote:
> 
> > On 2014/10/17 18:48:54, hans wrote:
> >
> >> On 2014/10/17 18:41:11, sof wrote:
> >> > Cc: +thakis, hans
> >> >
> >> > fyi - this crasher doesn't currently show with Blink's ToT (*), but
> >> it'd be
> >> good
> >> > to have the fix be considered for the next clang roll.
> >> >
> >> > * - GC_PLUGIN_IGNORE() annotations can be used to work around the bug,
> >>
> > should
> >
> >> a
> >> > recursive "part object" start being used from a GCed class. One place
> >> where
> >> such
> >> > a thing will show up soon is for context menu objects and their use of
> >> the
> >> > recursive ContextMenuItem.
> >>
> >
> >  Thanks! I just landed a Clang roll this week, and I'll make sure this
> >> patch is
> >> in when I build packages for the next roll.
> >>
> >
> > Looks like the last clang roll (2014-11-12) didn't include this?
> >
> 
> That roll got reverted in #304109
> 

oh; I'm just indirectly tracking via
http://commondatastorage.googleapis.com/chromium-browser-clang/index.html

Are there better alternatives/bugs to follow to stay up-to-date on status?

[Nico, 2014-12-01 19:55:56]

On Mon, Dec 1, 2014 at 11:39 AM, <sigbjornf@opera.com> wrote:

> On 2014/11/19 17:42:25, Nico wrote:
>
>> On Wed, Nov 19, 2014 at 2:59 AM, <mailto:sigbjornf@opera.com> wrote:
>>
>
>  > On 2014/10/17 18:48:54, hans wrote:
>> >
>> >> On 2014/10/17 18:41:11, sof wrote:
>> >> > Cc: +thakis, hans
>> >> >
>> >> > fyi - this crasher doesn't currently show with Blink's ToT (*), but
>> >> it'd be
>> >> good
>> >> > to have the fix be considered for the next clang roll.
>> >> >
>> >> > * - GC_PLUGIN_IGNORE() annotations can be used to work around the
>> bug,
>> >>
>> > should
>> >
>> >> a
>> >> > recursive "part object" start being used from a GCed class. One place
>> >> where
>> >> such
>> >> > a thing will show up soon is for context menu objects and their use
>> of
>> >> the
>> >> > recursive ContextMenuItem.
>> >>
>> >
>> >  Thanks! I just landed a Clang roll this week, and I'll make sure this
>> >> patch is
>> >> in when I build packages for the next roll.
>> >>
>> >
>> > Looks like the last clang roll (2014-11-12) didn't include this?
>> >
>>
>
>  That roll got reverted in #304109
>>
>
>
> oh; I'm just indirectly tracking via
> http://commondatastorage.googleapis.com/chromium-browser-clang/index.html
>
> Are there better alternatives/bugs to follow to stay up-to-date on status?
>

The bots look at CLANG_REVISION in tools/clang/scripts/update.sh


>
>
> https://codereview.chromium.org/666473003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.