[DevTools] Avoid current_ and pending_ being the same host in RenderFrameDevToolsAgentHost.

[DevTools] Avoid current_ and pending_ being the same host in RenderFrameDevToolsAgentHost.

This happens when we resue RenderFrameHost after crash.
The fix is to commit immediately to avoid pending. This is safe because
we don't even have two RenderFrameHosts.

BUG=644963
Committed: https://crrev.com/f07d611e4c7c7e0b38521119c42166b6104bba1b
Cr-Commit-Position: refs/heads/master@{#421697}

[dgozman, 2016-09-23 17:38:23]

dgozman@chromium.org changed reviewers:
+ creis@chromium.org, pfeldman@chromium.org

[dgozman, 2016-09-23 17:38:23]

Could you please take a look?

I have a problem writing test, because scenario requires RFH to be reused after
crash. This does not happen for chrome://kill navigation. Any ideas on how to
trigger crash on different page (not chrome:// scheme)?

Thanks,
Dmitry

[pfeldman, 2016-09-23 17:46:21]

lgtm

[Charlie Reis, 2016-09-23 18:26:16]

On 2016/09/23 17:38:23, dgozman wrote:
> Could you please take a look?

Thanks for the patch!  Seems reasonable, with some minor questions below.

> I have a problem writing test, because scenario requires RFH to be reused
after
> crash. This does not happen for chrome://kill navigation. Any ideas on how to
> trigger crash on different page (not chrome:// scheme)?
> 
> Thanks,
> Dmitry

Nick suggested content::CrashTab(), which doesn't use the chrome:// scheme. 
Would that work?

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:620: if (old_host
== new_host && !current_frame_crashed_)
Interesting.  I guess current_frame_crashed_ isn't true in this case, despite
the sad tab?  At first glance, I would have expected us to return early here.

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:644:
DCHECK(!pending_ || pending_->host() != old_host);
Maybe we can also upgrade this from a DCHECK to a CHECK?  That would have
prevented the use-after-free and made this a simple crash rather than a security
bug.

[dgozman, 2016-09-24 01:44:04]

Please take another look. I've added DCHECKs and test.

I will upgrade some DCHECKs to CHECKs in a follow-up patch.

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:620: if (old_host
== new_host && !current_frame_crashed_)
On 2016/09/23 18:26:16, Charlie Reis (slow) wrote:
> Interesting.  I guess current_frame_crashed_ isn't true in this case, despite
> the sad tab?  At first glance, I would have expected us to return early here.

The logic here is: if we didn't crash there is no need to reattach and
reinitialize the RenderFrameImpl. Since we crashed in this case, we skip early
return.

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:644:
DCHECK(!pending_ || pending_->host() != old_host);
On 2016/09/23 18:26:16, Charlie Reis (slow) wrote:
> Maybe we can also upgrade this from a DCHECK to a CHECK?  That would have
> prevented the use-after-free and made this a simple crash rather than a
security
> bug.

I can land a separate patch which upgrades to CHECK, so that we separate a
bug-fix from something which may potentially break in the wild.

[dgozman, 2016-09-24 01:44:09]

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

[commit-bot: I haz the power, 2016-09-24 01:44:28]

Dry run: 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-09-24 02:19:15]

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

[commit-bot: I haz the power, 2016-09-24 02:19:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2016-09-26 16:49:02]

Thanks!  LGTM, though it looks like the new test (and some existing tests) are
failing in patch 2.

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2366773003/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:644:
DCHECK(!pending_ || pending_->host() != old_host);
On 2016/09/24 01:44:04, dgozman wrote:
> On 2016/09/23 18:26:16, Charlie Reis (slow) wrote:
> > Maybe we can also upgrade this from a DCHECK to a CHECK?  That would have
> > prevented the use-after-free and made this a simple crash rather than a
> security
> > bug.
> 
> I can land a separate patch which upgrades to CHECK, so that we separate a
> bug-fix from something which may potentially break in the wild.

Sounds good, thanks.

[dgozman, 2016-09-27 18:55:13]

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

[commit-bot: I haz the power, 2016-09-27 18:55:55]

Dry run: 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-09-27 20:34:06]

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

[commit-bot: I haz the power, 2016-09-27 20:34:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dgozman, 2016-09-28 23:23:47]

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

[commit-bot: I haz the power, 2016-09-28 23:24:05]

Dry run: 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-09-29 00:35:25]

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

[commit-bot: I haz the power, 2016-09-29 00:35:26]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-09-29 00:42:58]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-09-29 00:42:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2366773003/#ps60001
(title: "rebased, win failure")

[commit-bot: I haz the power, 2016-09-29 00:43:23]

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-09-29 00:48:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-29 00:53:50]

Description was changed from

==========
[DevTools] Avoid current_ and pending_ being the same host in
RenderFrameDevToolsAgentHost.

This happens when we resue RenderFrameHost after crash.
The fix is to commit immediately to avoid pending. This is safe because
we don't even have two RenderFrameHosts.

BUG=644963
==========

to

==========
[DevTools] Avoid current_ and pending_ being the same host in
RenderFrameDevToolsAgentHost.

This happens when we resue RenderFrameHost after crash.
The fix is to commit immediately to avoid pending. This is safe because
we don't even have two RenderFrameHosts.

BUG=644963

Committed: https://crrev.com/f07d611e4c7c7e0b38521119c42166b6104bba1b
Cr-Commit-Position: refs/heads/master@{#421697}
==========

[commit-bot: I haz the power, 2016-09-29 00:53:51]

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