Avoid stack allocating StyleSheetCollections.

Avoid stack allocating StyleSheetCollections.

StyleSheetCollection is a GarbageCollected<>-derived object, which ideally
shouldn't be allocated on the stack (== outside the heap), as it leaves
the door open for it being attempted marked (and traced), which it the
allocation doesn't support (the allocation has no header information
preceeding it.)

R=
BUG=

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

[sof, 2016-06-24 14:12:04]

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

[sof, 2016-06-24 14:12:04]

not sure how stringent we should be about avoiding these stack allocations ..
thoughts?

[haraken, 2016-06-24 14:28:28]

LGTM

On 2016/06/24 14:12:04, sof wrote:
> not sure how stringent we should be about avoiding these stack allocations ..
> thoughts?

Yeah, there would be no need to force the rule. If someone tries to store an
on-stack object to a Member, it will anyway hit the assert in
Member::checkPointer().

[sof, 2016-06-24 15:28:28]

On 2016/06/24 14:28:28, haraken wrote:
> LGTM
> 
> On 2016/06/24 14:12:04, sof wrote:
> > not sure how stringent we should be about avoiding these stack allocations
..
> > thoughts?
> 
> Yeah, there would be no need to force the rule. If someone tries to store an
> on-stack object to a Member, it will anyway hit the assert in
> Member::checkPointer().

I'd forgotten about that check completely :)

A cheaper & approximate version would be to invert it and check if the pointer
is within the estimated stack region.

[haraken, 2016-08-19 01:13:54]

Let's land this CL :)

[sof, 2016-08-19 06:13:00]

On 2016/08/19 01:13:54, haraken wrote:
> Let's land this CL :)

ok, let me try.

[sof, 2016-08-19 06:13:08]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 06:13:32]

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

[Hannes Payer (out of office), 2016-08-19 06:39:16]

On 2016/08/19 06:13:00, sof wrote:
> On 2016/08/19 01:13:54, haraken wrote:
> > Let's land this CL :)
> 
> ok, let me try.

Cool, let's land this CL.

In general, you can have GarbageCollected<>-derived objects on the stack. You
only have to proof that they do not escape, i.e. get rooted by objects on the
garbage collected heap. This is basically manual escape analysis which usually
has the effect that very short living objects do not pollute the garbage
collected heap.

[haraken, 2016-08-19 06:43:33]

On 2016/08/19 06:39:16, Hannes Payer wrote:
> On 2016/08/19 06:13:00, sof wrote:
> > On 2016/08/19 01:13:54, haraken wrote:
> > > Let's land this CL :)
> > 
> > ok, let me try.
> 
> Cool, let's land this CL.
> 
> In general, you can have GarbageCollected<>-derived objects on the stack. You
> only have to proof that they do not escape, i.e. get rooted by objects on the
> garbage collected heap. This is basically manual escape analysis which usually
> has the effect that very short living objects do not pollute the garbage
> collected heap.

Yes, GarbageCollected objects on the stack are safe (and allowed in Blink)
unless the objects pointed to by some object (via Member/Persistent, incremental
marking stack etc). Once they're pointed to by some object, it causes a problem
because the on-stack objects don't have a header. Which is exactly what you're
hitting now.

[sof, 2016-08-19 06:46:57]

On 2016/08/19 06:43:33, haraken wrote:
> On 2016/08/19 06:39:16, Hannes Payer wrote:
> > On 2016/08/19 06:13:00, sof wrote:
> > > On 2016/08/19 01:13:54, haraken wrote:
> > > > Let's land this CL :)
> > > 
> > > ok, let me try.
> > 
> > Cool, let's land this CL.
> > 
> > In general, you can have GarbageCollected<>-derived objects on the stack.
You
> > only have to proof that they do not escape, i.e. get rooted by objects on
the
> > garbage collected heap. This is basically manual escape analysis which
usually
> > has the effect that very short living objects do not pollute the garbage
> > collected heap.
> 
> Yes, GarbageCollected objects on the stack are safe (and allowed in Blink)
> unless the objects pointed to by some object (via Member/Persistent,
incremental
> marking stack etc). Once they're pointed to by some object, it causes a
problem
> because the on-stack objects don't have a header. Which is exactly what you're
> hitting now.

(ftr, https://codereview.chromium.org/2090893003/ was what I used to identify
this particular one.)

[Hannes Payer (out of office), 2016-08-19 06:57:27]

On 2016/08/19 06:46:57, sof wrote:
> On 2016/08/19 06:43:33, haraken wrote:
> > On 2016/08/19 06:39:16, Hannes Payer wrote:
> > > On 2016/08/19 06:13:00, sof wrote:
> > > > On 2016/08/19 01:13:54, haraken wrote:
> > > > > Let's land this CL :)
> > > > 
> > > > ok, let me try.
> > > 
> > > Cool, let's land this CL.
> > > 
> > > In general, you can have GarbageCollected<>-derived objects on the stack.
> You
> > > only have to proof that they do not escape, i.e. get rooted by objects on
> the
> > > garbage collected heap. This is basically manual escape analysis which
> usually
> > > has the effect that very short living objects do not pollute the garbage
> > > collected heap.
> > 
> > Yes, GarbageCollected objects on the stack are safe (and allowed in Blink)
> > unless the objects pointed to by some object (via Member/Persistent,
> incremental
> > marking stack etc). Once they're pointed to by some object, it causes a
> problem
> > because the on-stack objects don't have a header. Which is exactly what
you're
> > hitting now.
> 
> (ftr, https://codereview.chromium.org/2090893003/ was what I used to identify
> this particular one.)

Cool!

[commit-bot: I haz the power, 2016-08-19 08:33:18]

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

[commit-bot: I haz the power, 2016-08-19 08:33:19]

Dry run: This issue passed the CQ dry run.

[sof, 2016-08-19 08:49:19]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-08-19 08:49:32]

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-08-19 08:53:13]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-19 08:55:03]

Description was changed from

==========
Avoid stack allocating StyleSheetCollections.

StyleSheetCollection is a GarbageCollected<>-derived object, which ideally
shouldn't be allocated on the stack (== outside the heap), as it leaves
the door open for it being attempted marked (and traced), which it the
allocation doesn't support (the allocation has no header information
preceeding it.)

R=
BUG=
==========

to

==========
Avoid stack allocating StyleSheetCollections.

StyleSheetCollection is a GarbageCollected<>-derived object, which ideally
shouldn't be allocated on the stack (== outside the heap), as it leaves
the door open for it being attempted marked (and traced), which it the
allocation doesn't support (the allocation has no header information
preceeding it.)

R=
BUG=

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

[commit-bot: I haz the power, 2016-08-19 08:55:04]

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