Oilpan: tidy up unsafe heap pointer reference following r361300.

Oilpan: tidy up unsafe heap pointer reference following r361300.

R=dcheng
BUG=

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

[sof, 2015-11-25 20:33:26]

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

[sof, 2015-11-25 20:33:26]

please take a look.

[dcheng, 2015-11-25 20:36:38]

Nice, I missed this. Thanks!

[dcheng, 2015-11-25 20:36:45]

LGTM too =P

[sof, 2015-11-25 20:43:15]

On 2015/11/25 20:36:38, dcheng wrote:
> Nice, I missed this. Thanks!

The GC plugin will soon flag such bare references as errors, so help is on the
way. :)

[sof, 2015-11-25 21:10:10]

Description was changed from

==========
Oilpan: tidy up unsafe heap pointer reference following r361300.

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

to

==========
Oilpan: tidy up unsafe heap pointer reference following r361300.

R=dcheng
BUG=
==========

[sof, 2015-11-25 21:10:14]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-25 21:12:02]

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

[commit-bot: I haz the power, 2015-11-25 22:36:36]

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

[commit-bot: I haz the power, 2015-11-25 22:36:37]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[haraken, 2015-11-25 23:33:56]

LGTM, but I'd prefer using RefPtrWillBePersistent.

RawPtrWillBePersistent implies a possibility that we have to use a RawPtr to
avoid a cycle but can use a Persistent for some reason. We need to consider why
every time.

[sof, 2015-11-26 06:47:34]

On 2015/11/25 23:33:56, haraken wrote:
> LGTM, but I'd prefer using RefPtrWillBePersistent.
> 
> RawPtrWillBePersistent implies a possibility that we have to use a RawPtr to
> avoid a cycle but can use a Persistent for some reason. We need to consider
why
> every time.

This is a back reference from the client to parent view -- wouldn't you have to
consider every time if that doesn't create a cycle non-Oilpan if you use
RefPtr<> ?

[haraken, 2015-11-26 06:55:18]

On 2015/11/26 06:47:34, sof wrote:
> On 2015/11/25 23:33:56, haraken wrote:
> > LGTM, but I'd prefer using RefPtrWillBePersistent.
> > 
> > RawPtrWillBePersistent implies a possibility that we have to use a RawPtr to
> > avoid a cycle but can use a Persistent for some reason. We need to consider
> why
> > every time.
> 
> This is a back reference from the client to parent view -- wouldn't you have
to
> consider every time if that doesn't create a cycle non-Oilpan if you use
> RefPtr<> ?

If this is a back reference (and cannot change it to a RefPtr), why can we use a
Persistent in oilpan?

[sof, 2015-11-26 07:09:49]

On 2015/11/26 06:55:18, haraken wrote:
> On 2015/11/26 06:47:34, sof wrote:
> > On 2015/11/25 23:33:56, haraken wrote:
> > > LGTM, but I'd prefer using RefPtrWillBePersistent.
> > > 
> > > RawPtrWillBePersistent implies a possibility that we have to use a RawPtr
to
> > > avoid a cycle but can use a Persistent for some reason. We need to
consider
> > why
> > > every time.
> > 
> > This is a back reference from the client to parent view -- wouldn't you have
> to
> > consider every time if that doesn't create a cycle non-Oilpan if you use
> > RefPtr<> ?
> 
> If this is a back reference (and cannot change it to a RefPtr), why can we use
a
> Persistent in oilpan?

Oh, TestWebRemoteFramClient::m_frame is effectively an owned (frame) reference,
not a back pointer.

[sof, 2015-11-26 07:32:21]

On 2015/11/26 07:09:49, sof wrote:
> On 2015/11/26 06:55:18, haraken wrote:
> > On 2015/11/26 06:47:34, sof wrote:
> > > On 2015/11/25 23:33:56, haraken wrote:
> > > > LGTM, but I'd prefer using RefPtrWillBePersistent.
> > > > 
> > > > RawPtrWillBePersistent implies a possibility that we have to use a
RawPtr
> to
> > > > avoid a cycle but can use a Persistent for some reason. We need to
> consider
> > > why
> > > > every time.
> > > 
> > > This is a back reference from the client to parent view -- wouldn't you
have
> > to
> > > consider every time if that doesn't create a cycle non-Oilpan if you use
> > > RefPtr<> ?
> > 
> > If this is a back reference (and cannot change it to a RefPtr), why can we
use
> a
> > Persistent in oilpan?
> 
> Oh, TestWebRemoteFramClient::m_frame is effectively an owned (frame)
reference,
> not a back pointer.

RawPtrWillBePersistent<> is the correct type to use -- see
WebRemoteFrameImpl::create().

[haraken, 2015-11-26 09:03:07]

On 2015/11/26 07:32:21, sof wrote:
> On 2015/11/26 07:09:49, sof wrote:
> > On 2015/11/26 06:55:18, haraken wrote:
> > > On 2015/11/26 06:47:34, sof wrote:
> > > > On 2015/11/25 23:33:56, haraken wrote:
> > > > > LGTM, but I'd prefer using RefPtrWillBePersistent.
> > > > > 
> > > > > RawPtrWillBePersistent implies a possibility that we have to use a
> RawPtr
> > to
> > > > > avoid a cycle but can use a Persistent for some reason. We need to
> > consider
> > > > why
> > > > > every time.
> > > > 
> > > > This is a back reference from the client to parent view -- wouldn't you
> have
> > > to
> > > > consider every time if that doesn't create a cycle non-Oilpan if you use
> > > > RefPtr<> ?
> > > 
> > > If this is a back reference (and cannot change it to a RefPtr), why can we
> use
> > a
> > > Persistent in oilpan?
> > 
> > Oh, TestWebRemoteFramClient::m_frame is effectively an owned (frame)
> reference,
> > not a back pointer.
> 
> RawPtrWillBePersistent<> is the correct type to use -- see
> WebRemoteFrameImpl::create().

Thanks, it's nasty that non-oilpan returns a leakRef-ed raw pointer...

Anyway this CL LGTM.

[sof, 2015-11-26 09:04:10]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-26 09:04:19]

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

[commit-bot: I haz the power, 2015-11-26 09:08:39]

Description was changed from

==========
Oilpan: tidy up unsafe heap pointer reference following r361300.

R=dcheng
BUG=
==========

to

==========
Oilpan: tidy up unsafe heap pointer reference following r361300.

R=dcheng
BUG=
==========

[commit-bot: I haz the power, 2015-11-26 09:08:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-11-26 09:10:34]

Description was changed from

==========
Oilpan: tidy up unsafe heap pointer reference following r361300.

R=dcheng
BUG=
==========

to

==========
Oilpan: tidy up unsafe heap pointer reference following r361300.

R=dcheng
BUG=

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

[commit-bot: I haz the power, 2015-11-26 09:10:35]

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