Delete entry in WebFrame->RenderFrameProxy map at WebFrame::close time.

Delete entry in WebFrame->RenderFrameProxy map at WebFrame::close time.

The entry in the WebFrame->RenderFrameProxy map is removed in the
RenderFrameProxy destructor, but the destructor runs after the frame
has been destroyed. Remove the entry at the time of WebFrame::close().

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

[nasko, 2015-05-07 21:25:29]

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

[nasko, 2015-05-07 21:25:30]

Hey Nick,
This is a small, independent CL to ensure we invalidate map entries as close to
the key being invalidated. RenderFrameImpl already handles it this way, so this
brings them in alignment.
Thanks in advance!
Nasko

[ncarter (slow), 2015-05-07 21:35:52]

lgtm with paranoid suggestion

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

https://codereview.chromium.org/1136593003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.cc:133: 
Is it worth adding a CHECK here to ensure sure that |web_frame_| is nullptr
(i.e. that we're being destroyed via frameDetached?) On the grounds that if it
isn't, we'd leak a reference in the g_frame_map, which could cause trouble
later.

[nasko, 2015-05-07 21:40:52]

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

https://codereview.chromium.org/1136593003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.cc:133: 
On 2015/05/07 21:35:52, ncarter wrote:
> Is it worth adding a CHECK here to ensure sure that |web_frame_| is nullptr
> (i.e. that we're being destroyed via frameDetached?) On the grounds that if it
> isn't, we'd leak a reference in the g_frame_map, which could cause trouble
> later.

Yeah, I thought of doing that and forgot to add it. Thanks!

[nasko, 2015-05-07 21:41:22]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-05-07 21:41:22]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1136593003/#ps20001
(title: "Add a CHECK")

[commit-bot: I haz the power, 2015-05-07 21:44:42]

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

[nasko, 2015-05-07 22:47:13]

Nick, the new CHECK did fire, so the latest patchset fixes the issue.

[ncarter (slow), 2015-05-07 23:33:36]

lgtm with comment suggestions

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

https://codereview.chromium.org/1136593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:735: if (!is_subframe_ &&
render_frame_proxy_)
I was initially worried that this frameDetached call would trigger a new Detach
IPC, but it seems it's safe: the proxies detached this way are never subframes,
so they'll never have parents, so we won't reach the detach IPC.

So I think we're okay.

https://codereview.chromium.org/1136593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:736: render_frame_proxy_->frameDetached();
Gosh it's kinda gross that the rfph dtor calls back into this class
(render_frame->set_render_frame_proxy(null)) before returning. But, I suppose
hard to avoid when you have circular references.

Might be worth adding a comment like this:

render_frame_proxy_->frameDetached();  // deletes/clears |render_frame_proxy_|

[nasko, 2015-05-07 23:44:36]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-05-07 23:44:36]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1136593003/#ps60001
(title: "Add a comment for the frameDetached call.")

[commit-bot: I haz the power, 2015-05-07 23:44:57]

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

[nasko, 2015-05-07 23:46:06]

Thanks!

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

https://codereview.chromium.org/1136593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:735: if (!is_subframe_ &&
render_frame_proxy_)
On 2015/05/07 23:33:36, ncarter wrote:
> I was initially worried that this frameDetached call would trigger a new
Detach
> IPC, but it seems it's safe: the proxies detached this way are never
subframes,
> so they'll never have parents, so we won't reach the detach IPC.
> 
> So I think we're okay.

I did at the subframe check exactly to guard against that. It also shouldn't
have ever been called for subframes.

https://codereview.chromium.org/1136593003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:736: render_frame_proxy_->frameDetached();
On 2015/05/07 23:33:36, ncarter wrote:
> Gosh it's kinda gross that the rfph dtor calls back into this class
> (render_frame->set_render_frame_proxy(null)) before returning. But, I suppose
> hard to avoid when you have circular references.
> 
> Might be worth adding a comment like this:
> 
> render_frame_proxy_->frameDetached();  // deletes/clears |render_frame_proxy_|
> 

I'm hopeful that the CL I'm working on is going to allow us to get rid of this.
I've added a comment in the meantime :(.

[commit-bot: I haz the power, 2015-05-08 01:17:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-08 01:18:34]

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