Oilpan: Temporary fix for leaks through mapping from element to attribute node list.

Oilpan: Temporary fix for leaks through mapping from element to attribute node list.

I think we need to bite the bullet and change the semantics of our
weak collections. These leaks are really hard to notice when you are
programming. See crbug.com/381629 which is the bug report I have
filed for tracking that.

R=erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com
BUG=381629
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175670

[sof, 2014-06-06 12:22:29]

lgtm

Even if temporary, this isn't likely to break much.

[haraken, 2014-06-06 12:23:16]

LGTM

[Mads Ager (chromium), 2014-06-06 12:23:51]

On 2014/06/06 12:22:29, sof wrote:
> lgtm
> 
> Even if temporary, this isn't likely to break much.

Right, in particular not since this is a raw pointer in non-oilpan builds anyway
and there are null checks before all usages. Thanks for the review Sigbjørn!

[Mads Ager (chromium), 2014-06-06 12:24:01]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-06-06 12:24:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/317203005/1

[sof, 2014-06-06 12:27:15]

On 2014/06/06 12:23:51, Mads Ager (chromium) wrote:
> On 2014/06/06 12:22:29, sof wrote:
> > lgtm
> > 
> > Even if temporary, this isn't likely to break much.
> 
> Right, in particular not since this is a raw pointer in non-oilpan builds
anyway
> and there are null checks before all usages. Thanks for the review Sigbjørn!

That too :) I was referring to the m_element disappearing if it becomes
otherwise detached & the Attr losing its value then.

[Mads Ager (chromium), 2014-06-06 12:33:49]

On 2014/06/06 12:27:15, sof wrote:
> On 2014/06/06 12:23:51, Mads Ager (chromium) wrote:
> > On 2014/06/06 12:22:29, sof wrote:
> > > lgtm
> > > 
> > > Even if temporary, this isn't likely to break much.
> > 
> > Right, in particular not since this is a raw pointer in non-oilpan builds
> anyway
> > and there are null checks before all usages. Thanks for the review Sigbjørn!
> 
> That too :) I was referring to the m_element disappearing if it becomes
> otherwise detached & the Attr losing its value then.

Ah, yes, the behavior of that will actually be as it is non-oilpan now. When an
element dies in the non-oilpan build it detaches all of the Attrs which sets
m_element to 0. So that should be fine.

[Mads Ager (chromium), 2014-06-06 12:54:47]

On 2014/06/06 12:33:49, Mads Ager (chromium) wrote:
> On 2014/06/06 12:27:15, sof wrote:
> > On 2014/06/06 12:23:51, Mads Ager (chromium) wrote:
> > > On 2014/06/06 12:22:29, sof wrote:
> > > > lgtm
> > > > 
> > > > Even if temporary, this isn't likely to break much.
> > > 
> > > Right, in particular not since this is a raw pointer in non-oilpan builds
> > anyway
> > > and there are null checks before all usages. Thanks for the review
Sigbjørn!
> > 
> > That too :) I was referring to the m_element disappearing if it becomes
> > otherwise detached & the Attr losing its value then.
> 
> Ah, yes, the behavior of that will actually be as it is non-oilpan now. When
an
> element dies in the non-oilpan build it detaches all of the Attrs which sets
> m_element to 0. So that should be fine.

Oh, I finally get what you mean Sigbjorn. Yes, when the element dies early in
the non-oilpan build we copy over the value to the attribute locally. That would
require custom weak processing. We can do that it this breaks stuff...

[Mads Ager (chromium), 2014-06-06 13:01:36]

The CQ bit was unchecked by ager@chromium.org

[Mads Ager (chromium), 2014-06-06 13:03:02]

I just checked. There is a test of the behavior after the element has been made
unreachable. The reason why it doesn't fail at this point is that we end up
finding the element via conservative stack scanning. I'll use custom weak
processing here and fix the test so that it gets precise GCs with Oilpan. Thanks
for pointing this out!

[Mads Ager (chromium), 2014-06-06 13:23:05]

Updated. PTAL

[sof, 2014-06-06 13:27:47]

nice, lgtm.

[Mads Ager (chromium), 2014-06-06 13:33:27]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-06-06 13:34:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/317203005/20001

[commit-bot: I haz the power, 2014-06-06 14:58:41]

Change committed as 175670