Fix DisownOpener and related tests.

Fix DisownOpener and related tests.

BUG=397185
TEST=RenderFrameHostManagerTest.DisownOpener*.
TBR=nasko (just for DisownSameSiteOpener test)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291556

[Charlie Reis, 2014-08-22 19:41:23]

Nasko, can you take a look?  I've included a lot of explanation of the 4 bugs I
found below.

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:459: // Notify all
swapped out hosts, including the pending RVH.
Bug 1: We wanted to notify the pending RVH, but we weren't because the pending
one isn't in proxy_hosts_ anymore.

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:457: // TODO(creis):
Take in RenderFrameHost instead, since frames can have openers.
I thought about tackling this here as well, but I think I'll save that for a
second CL to keep this one focused on bug fixes.

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:470:
render_frame_host_->render_view_host()->DisownOpener();
Bug 2: It's possible for a cross-process navigation to commit just as the
DidDisownOpener message is in flight.  We should still notify the recently
committed RFH, even though it's not swapped out anymore.

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:415: #if
defined(THREAD_SANITIZER)
I don't think I've fixed the ThreadSanitizer issue.  I could re-enable just to
see how it goes, or I could leave it disabled there.

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:467:
EXPECT_FALSE(new_shell->web_contents()->HasOpener());
I'm still not sure why the original test was failing the way it was.  The
IsSwappedOut check could have failed if we tried to notify the pending RVH
(e.g., before we moved to RenderFrameProxyHosts), but I think the ExecuteScript
line will wait for the opener to be cleared before we move on to the back
navigation below.

(Reasoning: the DidDisownOpener IPC will be sent by the renderer before the
DomAutomationController reply.)

I've removed the IsSwappedOut check, but there's a chance the test will fail
this new expectation if there's still a race.

https://codereview.chromium.org/497183003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/497183003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:1356:
DCHECK(IsSwappedOut());
Bug 3: This can happen on pending or recently committed navigations.  (We need
to tell all the RVHs but the one that notified us, regardless of their state.)

https://codereview.chromium.org/497183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/497183003/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3673: // No action is
necessary if the opener has already been cleared.
Bug 4: There was a risk of sending unnecessary notifications if the opener had
already been cleared.

[nasko, 2014-08-22 20:24:34]

It might be worthwhile adding a test case for same-process navigation in the new
window.

LGTM

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/497183003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:467:
EXPECT_FALSE(new_shell->web_contents()->HasOpener());
On 2014/08/22 19:41:23, Charlie Reis wrote:
> I'm still not sure why the original test was failing the way it was.  The
> IsSwappedOut check could have failed if we tried to notify the pending RVH
> (e.g., before we moved to RenderFrameProxyHosts), but I think the
ExecuteScript
> line will wait for the opener to be cleared before we move on to the back
> navigation below.

ExecuteScript waits until the script is executed before returning, due to how it
is implemented (waiting for the DomAutomation to send a message back). I expect
we get a didDisownOpener synchronously as part of the script exectution, hence
the IPC comes before the ExecuteScript has returned, which allows the DidDisown
IPC to be processed.

> (Reasoning: the DidDisownOpener IPC will be sent by the renderer before the
> DomAutomationController reply.)
> 
> I've removed the IsSwappedOut check, but there's a chance the test will fail
> this new expectation if there's still a race.

[Charlie Reis, 2014-08-22 21:03:31]

Thanks!  New test added for the same-site opener case.  As you mentioned, I'll
TBR you for the test.

[Charlie Reis, 2014-08-22 21:04:50]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-08-22 21:05:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/creis@chromium.org/497183003/20001

[commit-bot: I haz the power, 2014-08-22 22:11:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 23:36:59]

Committed patchset #2 (20001) as 291556

[nasko, 2014-08-25 14:17:57]

New test LGTM with one nit.

https://codereview.chromium.org/497183003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/497183003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1193: const
GURL kUrl2("http://www.chromium.org/");
nit: kUrl2 is unused.

[Charlie Reis, 2014-08-25 17:24:00]

https://codereview.chromium.org/497183003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/497183003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1193: const
GURL kUrl2("http://www.chromium.org/");
On 2014/08/25 14:17:57, nasko wrote:
> nit: kUrl2 is unused.

Fixed in patch 3 of https://codereview.chromium.org/505443002/.