Detach RemoteFrame from the tree when deleting RenderFrameProxy.

Detach RemoteFrame from the tree when deleting RenderFrameProxy.

BUG=414675
Committed: https://crrev.com/6e5f5510621232a24338c6036ed083d496213aa9
Cr-Commit-Position: refs/heads/master@{#300057}

[nasko, 2014-10-16 20:52:57]

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

[nasko, 2014-10-16 20:52:58]

Hey Daniel,
Can you look over the usage of detaching proxies/remote frames?
Thanks!
Nasko

[dcheng, 2014-10-16 20:54:47]

https://codereview.chromium.org/660093003/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/660093003/diff/1/content/renderer/render_fram...
content/renderer/render_frame_proxy.cc:182: web_frame_->detach();
Remind me what OnDeleteProxy is used for? I guess this is browser-driven?

[nasko, 2014-10-16 20:56:34]

https://codereview.chromium.org/660093003/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/660093003/diff/1/content/renderer/render_fram...
content/renderer/render_frame_proxy.cc:182: web_frame_->detach();
On 2014/10/16 20:54:47, dcheng wrote:
> Remind me what OnDeleteProxy is used for? I guess this is browser-driven?

Correct, this is an IPC coming from the browser. When we remove the real frame
in the process hosting it, we need to remove all the corresponding proxies from
all other processes involved. This is the handler for that IPC.

[dcheng, 2014-10-16 20:59:49]

lgtm

[nasko, 2014-10-16 22:43:42]

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

[nasko, 2014-10-16 22:43:43]

Hey Charlie,
Can you review the changes in ownership/deletion of RenderFrameProxy?
Since the top-level proxy is special and created by RV, along with the RF, it
should be deleted in that manner as well. Until we get rid of swapped out, this
seems like the safest thing, even though it is very ugly.
Thanks,
Nasko

[Charlie Reis, 2014-10-16 23:17:56]

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

https://codereview.chromium.org/660093003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:602: delete render_frame_proxy_;
Worth a DCHECK that we're the main frame, or is that covered elsewhere?

https://codereview.chromium.org/660093003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/660093003/diff/20001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:179: if (render_frame &&
web_frame_->parent())
Seems like render_frame wouldn't exist if this were a subframe.  Perhaps we want
to remove this whole block, including set_render_frame_proxy(NULL)?

[nasko, 2014-10-16 23:21:09]

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

https://codereview.chromium.org/660093003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:602: delete render_frame_proxy_;
On 2014/10/16 23:17:56, Charlie Reis wrote:
> Worth a DCHECK that we're the main frame, or is that covered elsewhere?

The web_frame_, which knows where in the frame tree we are, is already gone by
this time. I don't see a clean way to check unless we introduce a new member,
which is not worthwhile IMHO.

https://codereview.chromium.org/660093003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/660093003/diff/20001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:179: if (render_frame &&
web_frame_->parent())
On 2014/10/16 23:17:56, Charlie Reis wrote:
> Seems like render_frame wouldn't exist if this were a subframe.  Perhaps we
want
> to remove this whole block, including set_render_frame_proxy(NULL)?

Done.

[Charlie Reis, 2014-10-16 23:48:29]

LGTM

https://codereview.chromium.org/660093003/diff/40001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/660093003/diff/40001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:174: RenderFrameImpl* render_frame =
Don't need this anymore.

[nasko, 2014-10-17 02:29:49]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-10-17 02:30:18]

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

[commit-bot: I haz the power, 2014-10-17 02:43:31]

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

[commit-bot: I haz the power, 2014-10-17 02:43:32]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[nasko, 2014-10-17 04:04:44]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-10-17 04:05:32]

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

[commit-bot: I haz the power, 2014-10-17 04:06:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-17 04:06:54]

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