Don't forward load event when a RenderFrameHost is pending deletion.

Don't forward load event when a RenderFrameHost is pending deletion.

It was possible that a RFH became pending deletion in parallel to it
dispatching a load event, which could trigger a renderer kill in
RenderFrameHostImpl::OnDispatchLoad due to not finding a proxy in
the parent's SiteInstance.  Instead, ignore load events dispatched
when a RFH is pending deletion.  A test for this will be landed
separately; see https://crbug.com/636513#10 for repro steps.

BUG=636513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5a8d0a6e170969e26a53a809421c359b36d44e47
Cr-Commit-Position: refs/heads/master@{#414570}

[alexmos, 2016-08-25 21:40:07]

Description was changed from

==========
Don't forward load event when a RenderFrameHost is pending deletion.

It was possible that a RFH became pending deletion in parallel to it
dispatching a load event, which could trigger a renderer kill in
RenderFrameHostImpl::OnDispatchLoad due to not finding a proxy in
the parent's SiteInstance.  Instead, ignore load events dispatched
when a RFH is pending deletion.  A test for this will be landed
separately; see https://crbug.com/636513#10 for repro steps.

BUG=636513
==========

to

==========
Don't forward load event when a RenderFrameHost is pending deletion.

It was possible that a RFH became pending deletion in parallel to it
dispatching a load event, which could trigger a renderer kill in
RenderFrameHostImpl::OnDispatchLoad due to not finding a proxy in
the parent's SiteInstance.  Instead, ignore load events dispatched
when a RFH is pending deletion.  A test for this will be landed
separately; see https://crbug.com/636513#10 for repro steps.

BUG=636513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-08-25 21:40:09]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 21:41:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2016-08-25 21:43:40]

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

[alexmos, 2016-08-25 21:43:40]

Charlie, can you please take a quick look?  See my analysis and description of
how to repro this in https://crbug.com/636513#10.  I think this might be good to
land before the branch cut, and since I'm not sure I can unflake and clean up my
test draft before then, I'm going ahead with just the fix.

[Charlie Reis, 2016-08-25 21:54:02]

Thanks for figuring this out!  LGTM with nit.  I'm ok landing the test
separately, but let's be sure to get that as well.

(Side note: We should look for more cases where this problem might occur, like
other IPCs that cause us to replicate state to other renderers.  I imagine this
isn't the only place.)

https://codereview.chromium.org/2280783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2280783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1784: if (this !=
frame_tree_node()->render_manager()->current_frame_host())
I think !is_active() might be a clearer way to check if this RFH is pending
deletion?

[alexmos, 2016-08-25 21:59:06]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 21:59:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2016-08-25 22:03:46]

Thanks!

On 2016/08/25 21:54:02, Charlie Reis (OOO til 8-30) wrote:
> Thanks for figuring this out!  LGTM with nit.  I'm ok landing the test
> separately, but let's be sure to get that as well.

Will do.

> (Side note: We should look for more cases where this problem might occur, like
> other IPCs that cause us to replicate state to other renderers.  I imagine
this
> isn't the only place.)

Agreed.  Looking briefly, many IPC actually already check for is_active()
(CreateChildFrame, accessibility, DidStartLoading, etc.), but it'd be good to
systematically check that we aren't missing anything.

https://codereview.chromium.org/2280783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2280783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1784: if (this !=
frame_tree_node()->render_manager()->current_frame_host())
On 2016/08/25 21:54:01, Charlie Reis (OOO til 8-30) wrote:
> I think !is_active() might be a clearer way to check if this RFH is pending
> deletion?

Indeed, done.  There are a couple more instances of the old check in this file
that we can clean up in the future.

[alexmos, 2016-08-25 22:06:28]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2016-08-25 22:06:31]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-08-25 22:06:31]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2280783002/#ps20001
(title: "Use is_active() to check pending deletion")

[commit-bot: I haz the power, 2016-08-25 22:06:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 22:52:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-25 22:54:52]

Description was changed from

==========
Don't forward load event when a RenderFrameHost is pending deletion.

It was possible that a RFH became pending deletion in parallel to it
dispatching a load event, which could trigger a renderer kill in
RenderFrameHostImpl::OnDispatchLoad due to not finding a proxy in
the parent's SiteInstance.  Instead, ignore load events dispatched
when a RFH is pending deletion.  A test for this will be landed
separately; see https://crbug.com/636513#10 for repro steps.

BUG=636513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't forward load event when a RenderFrameHost is pending deletion.

It was possible that a RFH became pending deletion in parallel to it
dispatching a load event, which could trigger a renderer kill in
RenderFrameHostImpl::OnDispatchLoad due to not finding a proxy in
the parent's SiteInstance.  Instead, ignore load events dispatched
when a RFH is pending deletion.  A test for this will be landed
separately; see https://crbug.com/636513#10 for repro steps.

BUG=636513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5a8d0a6e170969e26a53a809421c359b36d44e47
Cr-Commit-Position: refs/heads/master@{#414570}
==========

[commit-bot: I haz the power, 2016-08-25 22:54:53]

Patchset 2 (id:??) landed as
https://crrev.com/5a8d0a6e170969e26a53a809421c359b36d44e47
Cr-Commit-Position: refs/heads/master@{#414570}