Ensure window reference is valid when navigating cross-process and back.

Ensure window reference is valid when navigating cross-process and back.

BUG=357747, 537657, 514766, 537689
Committed: https://crrev.com/120b59d16ddc290fa37fd441ec9e5d22a06a520d
Cr-Commit-Position: refs/heads/master@{#351725}

[nasko, 2015-09-30 20:12:35]

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

[nasko, 2015-09-30 20:12:36]

Hey Charlie,
Can you review this CL for me? It fixes the crash you reported when running with
swapped out disabled. The problem is that we always deleted the RenderViewHost,
regardless if its ref-count. This causes the back navigation to create a new
RenderViewHost/RenderFrameHost and in Blink we don't swap the old RemoteFrame
with a LocalFrame.
Thanks in advance!
Nasko

[Charlie Reis, 2015-09-30 21:09:55]

Oh, sorry, same nits from https://codereview.chromium.org/1381443003/.  (Looks
like I reviewed the wrong CL.)

[nasko, 2015-09-30 21:44:06]

https://chromiumcodereview.appspot.com/1383513005/diff/1/content/browser/fram...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/1383513005/diff/1/content/browser/fram...
content/browser/frame_host/render_frame_host_manager.cc:917: // reuse it.
On 2015/09/30 21:08:03, Charlie Reis wrote:
> I'm trying to understand how this relates to the crash.  Were we deleting the
> RVH while the main frame's RFPH had a reference to it?

Yes.
 
> Does this also somehow fix the "not calling WebFrame::Swap" issue Alex
> mentioned?

The proxy that references the RVH needs to be replaced with a frame, which
happens by ::swap. In order for swap to happen though, we need to create new
frame that is associated with the same RenderView. Since we've put the RVH on
the pending_deletion_list_, we don't find it when we need it and create a new
one. This new one creates its own RenderFrame for the top level and we continue
on.

We didn't catch this before, because all of the tests we have do a brand new
window.open('', 'foo'), which ends up finding the new RenderFrame, while we have
a lingering reference to the old RenderFrameProxy. The moment we use that
reference, things explode. This is why my new test takes exactly this approach,
to ensure the actual JS reference remains valid.

https://chromiumcodereview.appspot.com/1383513005/diff/1/content/test/data/cl...
File content/test/data/click-noreferrer-links.html (right):

https://chromiumcodereview.appspot.com/1383513005/diff/1/content/test/data/cl...
content/test/data/click-noreferrer-links.html:93: function
getExitingWindowLocation() {
On 2015/09/30 21:08:04, Charlie Reis wrote:
> nit: Exiting -> Existing
> 
> And actually, that name doesn't really convey what it's doing.  Maybe
> GetLastOpenedWindowLocation?

Done.

[Charlie Reis, 2015-09-30 21:49:07]

Great.  LGTM.

https://chromiumcodereview.appspot.com/1383513005/diff/20001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/1383513005/diff/20001/content/browser/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2066: //
back, can be still accessed through the original window reference in
nit: it can

[nasko, 2015-09-30 21:54:56]

https://chromiumcodereview.appspot.com/1383513005/diff/20001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/1383513005/diff/20001/content/browser/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2066: //
back, can be still accessed through the original window reference in
On 2015/09/30 21:49:07, Charlie Reis wrote:
> nit: it can

Done.

[nasko, 2015-09-30 21:55:01]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-09-30 21:55:01]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1383513005/#ps40001
(title: "Fixing a nit.")

[commit-bot: I haz the power, 2015-09-30 21:55:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1383513005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1383513005/40001

[commit-bot: I haz the power, 2015-10-01 01:21:47]

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

[commit-bot: I haz the power, 2015-10-01 01:21:48]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[nasko, 2015-10-01 04:18:32]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-10-01 04:18:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1383513005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1383513005/40001

[commit-bot: I haz the power, 2015-10-01 05:12:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-01 05:13:02]

Patchset 3 (id:??) landed as
https://crrev.com/120b59d16ddc290fa37fd441ec9e5d22a06a520d
Cr-Commit-Position: refs/heads/master@{#351725}