[Oilpan] Change pointer type to RefPtrWillBePersistent

Change pointer type of WebLocalFrameImpl from raw pointers to RefPtrWillBePersistent
in WebEmbeddedWorkerImpl and WebSharedWorkerImpl.


Those member variables are instantiated with WebLocalFrame::create(),
and it actually creates a WebLocalFrameImpl instance.

On Oilpan build, WebLocalFrameImpl has a self-referential persistent to
keep itself alive, and it will be cleared on call of
WebLocalFrameImpl::close().

In those two classes, m_mainFrame->close() is called in destructors, so
the self referential persistent is cleared in destruction on Oilpan build.
Beside it, on both non-Oilpan build and Oilpan build, m_mainFrame is
also cleared and should be released.


BUG=509911
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201191

[peria, 2015-08-25 03:47:46]

peria@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[peria, 2015-08-25 03:47:46]

PTL

[haraken, 2015-08-25 03:50:17]

Would you elaborate on why the RawPtrWillBePersistent doesn't cause a leak?

[peria, 2015-08-25 04:15:06]

On 2015/08/25 03:50:17, haraken wrote:
> Would you elaborate on why the RawPtrWillBePersistent doesn't cause a leak?

Following explanation is about Oilpan build only.
In those two classes, m_mainFrame is instantiated only in an initialization
and it means m_mainFrame has an instance of WebLocalFrameImpl.
WebLocalFrameImpl has a self-referential persistent to keep itself alive,
and it will be cleared on call of WebLocalFrameImpl::close().
In those two classes, m_mainFrame->close() is called in destructors, so the
new persistent, which is introduced in this CL, will be cleared then.

[haraken, 2015-08-25 05:15:19]

On 2015/08/25 04:15:06, peria wrote:
> On 2015/08/25 03:50:17, haraken wrote:
> > Would you elaborate on why the RawPtrWillBePersistent doesn't cause a leak?
> 
> Following explanation is about Oilpan build only.
> In those two classes, m_mainFrame is instantiated only in an initialization
> and it means m_mainFrame has an instance of WebLocalFrameImpl.
> WebLocalFrameImpl has a self-referential persistent to keep itself alive,
> and it will be cleared on call of WebLocalFrameImpl::close().
> In those two classes, m_mainFrame->close() is called in destructors, so the
> new persistent, which is introduced in this CL, will be cleared then.

Thanks for the clarification.

Then does it mean that we can remove the self-keeping persistent handle? In
other words:

- Remove the self-keeping handle from WebLocalFrameImpl.
- Change the WebLocalFrameImpl* to RefPtrWillBePersistent<WebLocalFrameImpl>. I
think the RefPtrWillBePersistent will be enough to keep the WebLocalFrameImpl
alive.

[peria, 2015-08-25 05:22:43]

On 2015/08/25 05:15:19, haraken wrote:
> On 2015/08/25 04:15:06, peria wrote:
> > On 2015/08/25 03:50:17, haraken wrote:
> > > Would you elaborate on why the RawPtrWillBePersistent doesn't cause a
leak?
> > 
> > Following explanation is about Oilpan build only.
> > In those two classes, m_mainFrame is instantiated only in an initialization
> > and it means m_mainFrame has an instance of WebLocalFrameImpl.
> > WebLocalFrameImpl has a self-referential persistent to keep itself alive,
> > and it will be cleared on call of WebLocalFrameImpl::close().
> > In those two classes, m_mainFrame->close() is called in destructors, so the
> > new persistent, which is introduced in this CL, will be cleared then.
> 
> Thanks for the clarification.
> 
> Then does it mean that we can remove the self-keeping persistent handle? In
> other words:
> 
> - Remove the self-keeping handle from WebLocalFrameImpl.
> - Change the WebLocalFrameImpl* to RefPtrWillBePersistent<WebLocalFrameImpl>.
I
> think the RefPtrWillBePersistent will be enough to keep the WebLocalFrameImpl
> alive.

It looks difficult.
We can do it for those two classes, but there are many other classes which
creates
instances of WebLocalFrameImple.
Now I'm not sure how they manage the pointer for it.

[haraken, 2015-08-25 05:27:25]

On 2015/08/25 05:22:43, peria wrote:
> On 2015/08/25 05:15:19, haraken wrote:
> > On 2015/08/25 04:15:06, peria wrote:
> > > On 2015/08/25 03:50:17, haraken wrote:
> > > > Would you elaborate on why the RawPtrWillBePersistent doesn't cause a
> leak?
> > > 
> > > Following explanation is about Oilpan build only.
> > > In those two classes, m_mainFrame is instantiated only in an
initialization
> > > and it means m_mainFrame has an instance of WebLocalFrameImpl.
> > > WebLocalFrameImpl has a self-referential persistent to keep itself alive,
> > > and it will be cleared on call of WebLocalFrameImpl::close().
> > > In those two classes, m_mainFrame->close() is called in destructors, so
the
> > > new persistent, which is introduced in this CL, will be cleared then.
> > 
> > Thanks for the clarification.
> > 
> > Then does it mean that we can remove the self-keeping persistent handle? In
> > other words:
> > 
> > - Remove the self-keeping handle from WebLocalFrameImpl.
> > - Change the WebLocalFrameImpl* to
RefPtrWillBePersistent<WebLocalFrameImpl>.
> I
> > think the RefPtrWillBePersistent will be enough to keep the
WebLocalFrameImpl
> > alive.
> 
> It looks difficult.
> We can do it for those two classes, but there are many other classes which
> creates
> instances of WebLocalFrameImple.
> Now I'm not sure how they manage the pointer for it.

Makes sense. Then let's just change the WebLocalFrameImpl* to
RefPtrWillBePersistent.

LGTM.

[peria, 2015-08-25 05:32:22]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-08-25 05:32:27]

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

[peria, 2015-08-25 06:14:09]

The CQ bit was unchecked by peria@chromium.org

[peria, 2015-08-25 06:46:58]

On 2015/08/25 05:27:25, haraken wrote:
> On 2015/08/25 05:22:43, peria wrote:
> > On 2015/08/25 05:15:19, haraken wrote:
> > > On 2015/08/25 04:15:06, peria wrote:
> > > > On 2015/08/25 03:50:17, haraken wrote:
> > > > > Would you elaborate on why the RawPtrWillBePersistent doesn't cause a
> > leak?
> > > > 
> > > > Following explanation is about Oilpan build only.
> > > > In those two classes, m_mainFrame is instantiated only in an
> initialization
> > > > and it means m_mainFrame has an instance of WebLocalFrameImpl.
> > > > WebLocalFrameImpl has a self-referential persistent to keep itself
alive,
> > > > and it will be cleared on call of WebLocalFrameImpl::close().
> > > > In those two classes, m_mainFrame->close() is called in destructors, so
> the
> > > > new persistent, which is introduced in this CL, will be cleared then.
> > > 
> > > Thanks for the clarification.
> > > 
> > > Then does it mean that we can remove the self-keeping persistent handle?
In
> > > other words:
> > > 
> > > - Remove the self-keeping handle from WebLocalFrameImpl.
> > > - Change the WebLocalFrameImpl* to
> RefPtrWillBePersistent<WebLocalFrameImpl>.
> > I
> > > think the RefPtrWillBePersistent will be enough to keep the
> WebLocalFrameImpl
> > > alive.
> > 
> > It looks difficult.
> > We can do it for those two classes, but there are many other classes which
> > creates
> > instances of WebLocalFrameImple.
> > Now I'm not sure how they manage the pointer for it.
> 
> Makes sense. Then let's just change the WebLocalFrameImpl* to
> RefPtrWillBePersistent.
> 
> LGTM.

I found that you say RefPtr which PS1 used RawPtr (for non-Oilpan build).
Now I changed it and ran some tests.  I think RefPtr also goes well.

[peria, 2015-08-25 07:55:45]

PTAL for just in case.

[haraken, 2015-08-25 07:57:24]

LGTM

[peria, 2015-08-25 08:03:05]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-08-25 08:03:19]

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

[commit-bot: I haz the power, 2015-08-25 08:55:43]

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

[commit-bot: I haz the power, 2015-08-25 08:55:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[peria, 2015-08-26 02:38:14]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-08-26 02:38:38]

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

[commit-bot: I haz the power, 2015-08-26 03:18:10]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201191