Re-enable SitePerProcessBrowserTest.CrossSiteIframe

Re-enable SitePerProcessBrowserTest.CrossSiteIframe

This CL fixes a few issues in the cross-process subframe navigation and re-enables the CrossSiteIframe test on most platforms. There are still some issues on ChromeOS and Android, which will be investigated and fixed in follow-up CL.

BUG=399775, 357747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291437

[nasko, 2014-08-19 20:56:31]

Charlie,
This CL is moving ViewMsg_Stop to be per frame and refactoring a bit around that
functionality. It still is timing out on a few tests, but it is a rough start.
Thanks for taking a look!
Nasko

[Charlie Reis, 2014-08-19 21:32:33]

Cool!  I'm excited to get rid of RenderWidgetHost's Stop method, since it
doesn't really belong there.

Two questions and a few nits below.

https://codereview.chromium.org/479403004/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/479403004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1550:
render_frame_host_->render_view_host()->Send(new ViewMsg_Stop(
Can you remind me why this is unsafe to remove?  If the old renderer had a
navigation in progress and we start a cross-process navigation, is there another
point where we tell the old one to stop loading?  Or are they racing to commit?

https://codereview.chromium.org/479403004/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/479403004/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:374: IPC_MESSAGE_ROUTED0(FrameMsg_Stop)
Let's add a comment here.

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:42: // When a RenderFrame will be
replaced by a RenderProxy, the WebRemoteFrame
nit: will be -> is

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:45: blink::WebRemoteFrame* web_frame =
blink::WebRemoteFrame::create(proxy.get());
I don't fully understand why the old code was unnecessary.  If that case isn't
reachable, is there a DCHECK we could add?  Or was it just wrong before?

[nasko, 2014-08-20 14:15:17]

https://codereview.chromium.org/479403004/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/479403004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1550:
render_frame_host_->render_view_host()->Send(new ViewMsg_Stop(
On 2014/08/19 21:32:32, Charlie Reis wrote:
> Can you remind me why this is unsafe to remove?  If the old renderer had a
> navigation in progress and we start a cross-process navigation, is there
another
> point where we tell the old one to stop loading?  Or are they racing to
commit?

RF::OnSwapOut does the stop, but I realized that this is much earlier, so
probably a good idea to keep. Restoring it.

https://codereview.chromium.org/479403004/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/479403004/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:374: IPC_MESSAGE_ROUTED0(FrameMsg_Stop)
On 2014/08/19 21:32:32, Charlie Reis wrote:
> Let's add a comment here.

Done.

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:42: // When a RenderFrame will be
replaced by a RenderProxy, the WebRemoteFrame
On 2014/08/19 21:32:32, Charlie Reis wrote:
> nit: will be -> is

Done.

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:45: blink::WebRemoteFrame* web_frame =
blink::WebRemoteFrame::create(proxy.get());
On 2014/08/19 21:32:32, Charlie Reis wrote:
> I don't fully understand why the old code was unnecessary.  If that case isn't
> reachable, is there a DCHECK we could add?  Or was it just wrong before?

It was just wrong before. It was hitting an ASSERT in WebFrame::swap, which
expects a WebRemoteFrame to be created through ::create instead of
::createRemoteChild.

[Charlie Reis, 2014-08-20 17:09:37]

LGTM.  You might need to rebase past r290828 (the SwapOut CL) to make sure it
still works.  Thanks!

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/479403004/diff/80001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:45: blink::WebRemoteFrame* web_frame =
blink::WebRemoteFrame::create(proxy.get());
On 2014/08/20 14:15:17, nasko wrote:
> On 2014/08/19 21:32:32, Charlie Reis wrote:
> > I don't fully understand why the old code was unnecessary.  If that case
isn't
> > reachable, is there a DCHECK we could add?  Or was it just wrong before?
> 
> It was just wrong before. It was hitting an ASSERT in WebFrame::swap, which
> expects a WebRemoteFrame to be created through ::create instead of
> ::createRemoteChild.

Acknowledged.

[nasko, 2014-08-20 17:13:00]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-08-20 17:14:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/479403004/120001

[nasko, 2014-08-20 17:25:07]

Avi, can you review the OSX change?
Thanks!

[Avi (use Gerrit), 2014-08-20 17:36:13]

Mac change LGTM

Whoever wrote that must have been copy/pasting from somewhere else...

[commit-bot: I haz the power, 2014-08-20 19:13:53]

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...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 19:44:17]

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

[commit-bot: I haz the power, 2014-08-20 19:44:18]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2014-08-21 21:19:45]

Charlie, can you take another look? The latest patch adds the fix you had for
OnSwapOut.

[Charlie Reis, 2014-08-21 21:24:10]

The diff from patchset 7 to patchset 8 LGTM.  Thanks!

[nasko, 2014-08-22 16:17:02]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-08-22 16:18:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/479403004/180001

[commit-bot: I haz the power, 2014-08-22 17:13:37]

Committed patchset #10 (180001) as 291437

[benwells, 2014-08-28 07:02:31]

A revert of this CL (patchset #10) has been created in
https://codereview.chromium.org/515073002/ by benwells@chromium.org.

The reason for reverting is: This is causing unaddressable memory errors on the
DrMemory full bot. Build where the error was introduced:

http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%20...

(error hash: 25DFC5E3C30FC6B7)

Sample log output:

UNADDRESSABLE ACCESS of freed memory: reading 0x034348e0-0x034348e4 4 byte(s)
# 0 blink_web.dll!blink::toCoreFrame                                     
[third_party\webkit\source\web\webframe.cpp:24]
# 1 blink_web.dll!blink::RemoteFrameClient::firstChild                   
[third_party\webkit\source\web\remoteframeclient.cpp:49]
# 2 blink_web.dll!blink::WebRemoteFrameImpl::~WebRemoteFrameImpl         
[third_party\webkit\source\web\webremoteframeimpl.cpp:106]
# 3 blink_web.dll!blink::WebRemoteFrameImpl::close                       
[third_party\webkit\source\web\webremoteframeimpl.cpp:131]
# 4 content.dll!content::RenderFrameProxy::~RenderFrameProxy             
[content\renderer\render_frame_proxy.cc:120]
# 5 content.dll!content::RenderFrameProxy::`vector deleting destructor'
# 6 content.dll!content::RenderFrameProxy::OnDeleteProxy                 
[content\renderer\render_frame_proxy.cc:180]
# 7 content.dll!content::MessageRouter::RouteMessage                     
[content\common\message_router.cc:54]
# 8 content.dll!content::MessageRouter::OnMessageReceived                
[content\common\message_router.cc:46]
# 9 content.dll!content::ChildThread::OnMessageReceived                  
[content\child\child_thread.cc:494]
#10 ipc.dll!IPC::ChannelProxy::Context::OnDispatchMessage                
[ipc\ipc_channel_proxy.cc:273]
#11 ipc.dll!base::internal::Invoker<>::Run                               
[base\bind_internal.h:1253]
#12 base.dll!base::debug::TaskAnnotator::RunTask                         
[base\debug\task_annotator.cc:62]
#13 base.dll!base::MessageLoop::RunTask                                  
[base\message_loop\message_loop.cc:436]
#14 base.dll!base::MessageLoop::DeferOrRunPendingTask                    
[base\message_loop\message_loop.cc:445]
#15 base.dll!base::MessageLoop::DoWork                                   
[base\message_loop\message_loop.cc:552]
#16 base.dll!base::MessagePumpDefault::Run                               
[base\message_loop\message_pump_default.cc:32]
#17 base.dll!base::MessageLoop::RunHandler                               
[base\message_loop\message_loop.cc:408]
#18 content.dll!content::RendererMain                                    
[content\renderer\renderer_main.cc:227]
#19 content.dll!content::RunNamedProcessTypeMain                         
[content\app\content_main_runner.cc:415]
#20 content.dll!content::ContentMainRunnerImpl::Run                      
[content\app\content_main_runner.cc:764]
#21 content.dll!content::ContentMain                                     
[content\app\content_main.cc:19]
#22 content::LaunchTests                                                 
[content\public\test\test_launcher.cc:475]
#23 main                                                                 
[content\test\content_test_launcher.cc:123]


BTW IMHO the CL summary for this change could be better, this change did way
more than re-enalbe a test. Ideally the summary would capture the entirety of
the change..