When a proxy is detached, immediately delete its associated provisional frame.

When a proxy is detached, immediately delete the associated provisional frame.

This provides an alternate, more robust fix for race conditions in
issues 526304 and 568676, avoiding early returns in OnNavigate and
didCommitProvisionalLoad when a provisional frame's proxy is detached.
Instead, the RenderFrameProxy now tracks a provisional frame that
would replace it if it commits, and cleans it up immediately if it
gets detached.  Likewise, if the provisional frame is destroyed before
commit (e.g., if the pending navigation is canceled), it unassigns
itself as the proxy's provisional frame.

RenderFrameProxy previously maintained frame_routing_id_, which is
currently unused, so the new provisional_frame_routing_id_ replaces
it.

BUG=487872, 526304, 568676, 660622
Review-Url: https://codereview.chromium.org/2628133002
Cr-Commit-Position: refs/heads/master@{#445515}
Committed: https://chromium.googlesource.com/chromium/src/+/f076d91e660f7289f0ee4bbda0cd1f89563760f3

[alexmos, 2017-01-12 02:29:00]

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

[commit-bot: I haz the power, 2017-01-12 02:29:39]

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, 2017-01-12 02:39:53]

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

[commit-bot: I haz the power, 2017-01-12 02:39:55]

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

[alexmos, 2017-01-12 18:05:42]

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

[commit-bot: I haz the power, 2017-01-12 18:06:39]

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, 2017-01-12 20:22:41]

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

[commit-bot: I haz the power, 2017-01-12 20:22:41]

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_...)

[alexmos, 2017-01-12 22:06:20]

Description was changed from

==========
When a proxy is detached, immediately delete its associated provisional frame.

BUG=
==========

to

==========
When a proxy is detached, immediately delete its associated provisional frame.

This gives an alternate, more robust fix for race conditions in issues
526304 and 568676, avoiding early returns in OnNavigate and
didCommitProvisionalLoad when a provisional frame's proxy is detached.

BUG=487872,526304,568676,660622
==========

[alexmos, 2017-01-12 23:47:06]

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

[commit-bot: I haz the power, 2017-01-12 23:48:16]

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

[alexmos, 2017-01-13 00:02:01]

Description was changed from

==========
When a proxy is detached, immediately delete its associated provisional frame.

This gives an alternate, more robust fix for race conditions in issues
526304 and 568676, avoiding early returns in OnNavigate and
didCommitProvisionalLoad when a provisional frame's proxy is detached.

BUG=487872,526304,568676,660622
==========

to

==========
When a proxy is detached, immediately delete the associated provisional frame.

This provides an alternate, more robust fix for race conditions in
issues 526304 and 568676, avoiding early returns in OnNavigate and
didCommitProvisionalLoad when a provisional frame's proxy is detached.
Instead, the RenderFrameProxy now tracks a provisional frame that
would replace it if it commits, and cleans it up immediately if it
gets detached.  Likewise, if the provisional frame is destroyed before
commit (e.g., if the pending navigation is canceled), it unassigns
itself as the proxy's provisional frame.

RenderFrameProxy previously maintained frame_routing_id_, which is
currently unused, so the new provisional_frame_routing_id_ replaces
it.

BUG=487872,526304,568676,660622
==========

[commit-bot: I haz the power, 2017-01-13 00:50:53]

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

[commit-bot: I haz the power, 2017-01-13 00:50:54]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-01-13 01:50:30]

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

[commit-bot: I haz the power, 2017-01-13 01:50:57]

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

[alexmos, 2017-01-13 02:26:53]

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

[alexmos, 2017-01-13 02:26:54]

Hey Charlie, this is the followup we discussed in
https://codereview.chromium.org/2619123002.  Let me know what you think,
especially whether the new CHECKs make sense.

https://codereview.chromium.org/2628133002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2628133002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6225:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, NavigateAboutBlankAndDetach) {
This test was disabled in https://crbug.com/660622, but it's passing for me
locally, so I don't know if something fixed it since then.  I'd like to try and
re-enable it here since it also tackles the same case of removing the proxy in a
remote-to-local navigation.  I made it a little more robust by waiting for the
subframe to be deleted, rather than for its navigation to succeed, which might
have caused some raciness (not sure).  We can take another look at it if it
starts failing again.

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

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:1001: if (!proxy)
This one is still necessary: if the proxy was detached after the provisional
frame's FrameMsg_CreateFrame was sent but before it arrived, we still need to
abort the frame creation.

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.h:188: int provisional_frame_routing_id_;
frame_routing_id_ was unused, so I just took it out in favor of
provisional_frame_routing_id_.

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (left):

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:893:
provisional_frame->Navigate(common_params, StartNavigationParams(),
I didn't find a way to test this Navigate() call anymore, given that the frame
is gone, so I just repurposed the test to check that the frame was deleted. 
(The only thing we could still test would be to see if OnNavigate would be
dropped for a non-existent frame, but I don't know if that's possible to do in a
RenderViewTest)

[commit-bot: I haz the power, 2017-01-13 03:57:55]

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

[commit-bot: I haz the power, 2017-01-13 03:57:56]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-18 00:18:42]

Thanks, and sorry for the delay!  LGTM.

https://codereview.chromium.org/2628133002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2628133002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6225:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, NavigateAboutBlankAndDetach) {
On 2017/01/13 02:26:54, alexmos wrote:
> This test was disabled in https://crbug.com/660622, but it's passing for me
> locally, so I don't know if something fixed it since then.  I'd like to try
and
> re-enable it here since it also tackles the same case of removing the proxy in
a
> remote-to-local navigation.  I made it a little more robust by waiting for the
> subframe to be deleted, rather than for its navigation to succeed, which might
> have caused some raciness (not sure).  We can take another look at it if it
> starts failing again.

Acknowledged.

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

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:1001: if (!proxy)
On 2017/01/13 02:26:54, alexmos wrote:
> This one is still necessary: if the proxy was detached after the provisional
> frame's FrameMsg_CreateFrame was sent but before it arrived, we still need to
> abort the frame creation.

Yes, since this is a static method.  Agreed.

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3139:
CHECK_EQ(proxy->provisional_frame_routing_id(), routing_id_);
I'm ok with these checks-- I agree with the reasoning.

nit: routing_id_ seems like the expected value to me, so maybe swap the order of
the parameters to CHECK_EQ?

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.h:188: int provisional_frame_routing_id_;
On 2017/01/13 02:26:54, alexmos wrote:
> frame_routing_id_ was unused, so I just took it out in favor of
> provisional_frame_routing_id_.

Acknowledged.

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (left):

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:893:
provisional_frame->Navigate(common_params, StartNavigationParams(),
On 2017/01/13 02:26:54, alexmos wrote:
> I didn't find a way to test this Navigate() call anymore, given that the frame
> is gone, so I just repurposed the test to check that the frame was deleted. 
> (The only thing we could still test would be to see if OnNavigate would be
> dropped for a non-existent frame, but I don't know if that's possible to do in
a
> RenderViewTest)

Acknowledged.

[alexmos, 2017-01-23 19:14:15]

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

[commit-bot: I haz the power, 2017-01-23 19:14:49]

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, 2017-01-23 20:11:50]

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

[commit-bot: I haz the power, 2017-01-23 20:11:51]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-01-23 22:20:59]

Thanks, I'm going to land this now that we've branched.

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

https://codereview.chromium.org/2628133002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3139:
CHECK_EQ(proxy->provisional_frame_routing_id(), routing_id_);
On 2017/01/18 00:18:41, Charlie Reis wrote:
> I'm ok with these checks-- I agree with the reasoning.
> 
> nit: routing_id_ seems like the expected value to me, so maybe swap the order
of
> the parameters to CHECK_EQ?

Done.

[alexmos, 2017-01-23 22:21:23]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2017-01-23 22:21:24]

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/2628133002/#ps80001
(title: "Charlie's nit")

[commit-bot: I haz the power, 2017-01-23 22:22:13]

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, 2017-01-23 22:28:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485210083880150,
"parent_rev": "2ec369bd8d2542a0e163c2b5efe647a54ef69ae6", "commit_rev":
"f076d91e660f7289f0ee4bbda0cd1f89563760f3"}

[commit-bot: I haz the power, 2017-01-23 22:28:35]

Description was changed from

==========
When a proxy is detached, immediately delete the associated provisional frame.

This provides an alternate, more robust fix for race conditions in
issues 526304 and 568676, avoiding early returns in OnNavigate and
didCommitProvisionalLoad when a provisional frame's proxy is detached.
Instead, the RenderFrameProxy now tracks a provisional frame that
would replace it if it commits, and cleans it up immediately if it
gets detached.  Likewise, if the provisional frame is destroyed before
commit (e.g., if the pending navigation is canceled), it unassigns
itself as the proxy's provisional frame.

RenderFrameProxy previously maintained frame_routing_id_, which is
currently unused, so the new provisional_frame_routing_id_ replaces
it.

BUG=487872,526304,568676,660622
==========

to

==========
When a proxy is detached, immediately delete the associated provisional frame.

This provides an alternate, more robust fix for race conditions in
issues 526304 and 568676, avoiding early returns in OnNavigate and
didCommitProvisionalLoad when a provisional frame's proxy is detached.
Instead, the RenderFrameProxy now tracks a provisional frame that
would replace it if it commits, and cleans it up immediately if it
gets detached.  Likewise, if the provisional frame is destroyed before
commit (e.g., if the pending navigation is canceled), it unassigns
itself as the proxy's provisional frame.

RenderFrameProxy previously maintained frame_routing_id_, which is
currently unused, so the new provisional_frame_routing_id_ replaces
it.

BUG=487872,526304,568676,660622

Review-Url: https://codereview.chromium.org/2628133002
Cr-Commit-Position: refs/heads/master@{#445515}
Committed:
https://chromium.googlesource.com/chromium/src/+/f076d91e660f7289f0ee4bbda0cd...
==========

[commit-bot: I haz the power, 2017-01-23 22:28:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f076d91e660f7289f0ee4bbda0cd...