Convert main_render_frame_ to raw pointer in RenderViewImpl.

Convert main_render_frame_ to raw pointer in RenderViewImpl.

The main RenderFrame object for a page is currently owned directly
through a scoped_ptr by RenderViewImpl. Since we want the main
RenderFrame to be deleted when we do a cross-process navigation,
it needs to be cleaned by as part of the regular callbacks from
Blink - WebFrameClient::frameDetached. This requires the frame to not
be owned by RenderViewImpl, so this CL changes it from scoped_ptr
to raw pointer.

The only change in behavior after this CL is that the RenderFrame
will be destroyed prior to RenderView being destroyed as part of
WebView::close(). The destruction of both objects still stays in
the same task on the event loop, though.

BUG=357747
Committed: https://crrev.com/77de231e243d5adb0bd82c630fd6f3382ab3f42e
Cr-Commit-Position: refs/heads/master@{#329337}

[nasko, 2015-05-08 18:41:19]

nasko@chromium.org changed reviewers:
+ lfg@chromium.org

[nasko, 2015-05-08 18:41:20]

Hey Lucas,
Can you review this CL for me? It is another step towards allowing the main
frame to swap from local to remote in cross-process navigations. This CL
detaches the main RenderFrame from RenderView in terms of ownership, so the
RenderView can persist while the frame object is changed.
Thanks in advance!
Nasko

[nasko, 2015-05-08 18:43:15]

nasko@chromium.org changed reviewers:
+ nick@chromium.org

[nasko, 2015-05-08 18:43:16]

Hey Nick,
Can you take a look at this as well? I want to have extra bit of confidence on
this one, as it is a major stepping stone to removing swapped out state.
Thanks in advance!
Nasko

[lfg, 2015-05-08 19:42:27]

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2654: render_view_->main_render_frame_ =
this;
Consider adding a CHECK or DCHECK that there's no previous main_render_frame_.
It shouldn't, since it's a RemoteFrame going to LocalFrame, but might be good
just for sanity.

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
content/renderer/render_view_browsertest.cc:371: TEST_F(RenderViewImplTest,
RenderFrameMessageAfterDetach) {
After the changes the test is not testing for the same thing as before
(receiving a message after closing the view). We should either rename the test
or delete it if it's no longer relevant.

[nasko, 2015-05-11 16:12:33]

A round of fixes.

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2654: render_view_->main_render_frame_ =
this;
On 2015/05/08 19:42:27, lfg wrote:
> Consider adding a CHECK or DCHECK that there's no previous main_render_frame_.
> It shouldn't, since it's a RemoteFrame going to LocalFrame, but might be good
> just for sanity.

Done.

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1130233002/diff/20001/content/renderer/render...
content/renderer/render_view_browsertest.cc:371: TEST_F(RenderViewImplTest,
RenderFrameMessageAfterDetach) {
On 2015/05/08 19:42:27, lfg wrote:
> After the changes the test is not testing for the same thing as before
> (receiving a message after closing the view). We should either rename the test
> or delete it if it's no longer relevant.

I was looking around for a way to rewrite the test to check what the bug was, so
I left it like this initially. Unfortunately, I couldn't find an easy way to do
it, so I renamed the test to indicate the new expectations.

It might be possible to still write this test case, but it requires adding
support for IPC dispatching to the mock thread object in the test. It is
non-trivial change though, if we deem it is still a needed test, I'd rather do
it in a separate CL to keep this one minimalistic.

[ncarter (slow), 2015-05-11 18:27:13]

I submitted patchset #2 to the blink_win bots, and there may be some real
failures there -- please have a lookand make sure they're clean. I think the
asan bots are a good idea for this patch too.

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:743: // must be cleared, otherwise it will
result in use-after-free bugs.
"otherwise it will result in use-after-free bugs" seems a bit meta. It's fine to
say only that we need to eliminate a reference to this object: we're in the
dtor; the motivation is obvious.

A check like the following might make it clearer:

if (render_view->main_render_frame_) {
  CHECK_EQ(render_view->main_render_frame_, this);
  render_view->main_render_frame_ = nullptr;
}

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2654:
CHECK(!render_view_->main_render_frame_);
How does this happen?

Seems like RenderViewImpl::Create always returns a render_view_ with a
main_render_frame_ set by Initialize(). I'm not seeing the codepath that would
let us wind up with a RenderView with a null main_render_frame_?

[nasko, 2015-05-11 22:01:11]

Addressed Nick's review comments. 

Looking into Windows layout test failures, which might or might not bring more
changes to this CL.

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:743: // must be cleared, otherwise it will
result in use-after-free bugs.
On 2015/05/11 18:27:13, ncarter wrote:
> "otherwise it will result in use-after-free bugs" seems a bit meta. It's fine
to
> say only that we need to eliminate a reference to this object: we're in the
> dtor; the motivation is obvious.
> 
> A check like the following might make it clearer:
> 
> if (render_view->main_render_frame_) {
>   CHECK_EQ(render_view->main_render_frame_, this);
>   render_view->main_render_frame_ = nullptr;
> }

Done.

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2654:
CHECK(!render_view_->main_render_frame_);
On 2015/05/11 18:27:13, ncarter wrote:
> How does this happen?
> 
> Seems like RenderViewImpl::Create always returns a render_view_ with a
> main_render_frame_ set by Initialize(). I'm not seeing the codepath that would
> let us wind up with a RenderView with a null main_render_frame_?

Create RenderView and the main RenderFrame. Navigate the frame cross process,
which replaces the RenderFrame with RenderFrameProxy. In the destructor of
RenderFrame, we nullify the main_render_frame_ pointer, while RenderView is
still alive. Later on, we navigate that same frame back to this process,
resulting in replacing the RenderFrameProxy with a new RenderFrame. They all use
the same RenderView, so we need to ensure we update the main_render_frame_
pointer to the new RenderFrame object. It happens in this block of code.

Should I be including more in the comment? It was supposed to described this,
but in less words. It failed though : (.

[ncarter (slow), 2015-05-11 22:44:50]

All my comments have been addressed; I don't see any problems with the CL
(except for the possible layout test issues). lgtm but let me do another pass if
you make more changes.

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1130233002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2654:
CHECK(!render_view_->main_render_frame_);
On 2015/05/11 22:01:11, nasko wrote:
> On 2015/05/11 18:27:13, ncarter wrote:
> > How does this happen?
> > 
> > Seems like RenderViewImpl::Create always returns a render_view_ with a
> > main_render_frame_ set by Initialize(). I'm not seeing the codepath that
would
> > let us wind up with a RenderView with a null main_render_frame_?
> 
> Create RenderView and the main RenderFrame. Navigate the frame cross process,
> which replaces the RenderFrame with RenderFrameProxy. In the destructor of
> RenderFrame, we nullify the main_render_frame_ pointer, while RenderView is
> still alive. Later on, we navigate that same frame back to this process,
> resulting in replacing the RenderFrameProxy with a new RenderFrame. They all
use
> the same RenderView, so we need to ensure we update the main_render_frame_
> pointer to the new RenderFrame object. It happens in this block of code.
> 
> Should I be including more in the comment? It was supposed to described this,
> but in less words. It failed though : (.

No, it's fine and this makes sense. I had forgotten that the render_view_ could
keep on living like that after its original main frame went remote.

[dcheng, 2015-05-12 01:46:51]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-05-12 01:47:56]

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

[commit-bot: I haz the power, 2015-05-12 03:09:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-12 03:10:21]

Patchset 4 (id:??) landed as
https://crrev.com/77de231e243d5adb0bd82c630fd6f3382ab3f42e
Cr-Commit-Position: refs/heads/master@{#329337}