diff options
| author | viettrungluu@chromium.org <viettrungluu@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-04-11 21:48:33 +0000 |
|---|---|---|
| committer | viettrungluu@chromium.org <viettrungluu@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-04-11 21:48:33 +0000 |
| commit | c9b4bffe4ede7df7b580d5691140e84a2b1dc294 (patch) | |
| tree | cf79de28780d7df9bb6bb65c4f04b656ba3a74ae | |
| parent | 2c4d732f7e2618aee00887a1f6cf626230ff2366 (diff) | |
| download | chromium_src-c9b4bffe4ede7df7b580d5691140e84a2b1dc294.zip chromium_src-c9b4bffe4ede7df7b580d5691140e84a2b1dc294.tar.gz chromium_src-c9b4bffe4ede7df7b580d5691140e84a2b1dc294.tar.bz2 | |
Revert 263352 "Introduce RenderFrameProxyHost object and use it ..."
Leaks: Linux LSan:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%282%29/builds/1508
E.g.:
Indirect leak of 1544 byte(s) in 1 object(s) allocated from:
#0 0x4fc78b in operator new(unsigned long) /usr/local/google/home/thakis/src/chrome/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
#1 0x232ebeb in allocate /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/ext/new_allocator.h:92
#2 0x232ebeb in _M_allocate /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_vector.h:150
#3 0x232ebeb in _M_allocate_and_copy<std::move_iterator<__gnu_cxx::_Hashtable_node<std::pair<const std::basic_string<char>, content::SiteInstance *> > **> > /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_vector.h:1052
#4 0x232ebeb in reserve /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/vector.tcc:74
#5 0x232ebeb in _M_initialize_buckets /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/backward/hashtable.h:577
#6 0x232ebeb in __gnu_cxx::hashtable<std::pair<std::string const, content::SiteInstance*>, std::string, __gnu_cxx::hash<std::string>, std::_Select1st<std::pair<std::string const, content::SiteInstance*> >, std::equal_to<std::string>, std::allocator<content::SiteInstance*> >::hashtable(unsigned long, __gnu_cxx::hash<std::string> const&, std::equal_to<std::string> const&, std::allocator<std::pair<std::string const, content::SiteInstance*> > const&) /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/backward/hashtable.h:334
#7 0x232cb58 in RefCounted /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/ext/hash_map:126
#8 0x232cb58 in content::BrowsingInstance::BrowsingInstance(content::BrowserContext*) content/browser/browsing_instance.cc:18
#9 0x28d4131 in content::SiteInstance::Create(content::BrowserContext*) content/browser/site_instance_impl.cc:220
#10 0x2db9c92 in CreateTestWebContents content/public/test/test_renderer_host.cc:138
#11 0x2db9c92 in content::RenderViewHostTestHarness::SetUp() content/public/test/test_renderer_host.cc:182
#12 0xda5e2d in content::RenderFrameHostManagerTest::SetUp() content/browser/frame_host/render_frame_host_manager_unittest.cc:204
#13 0x2eea0f2 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2045
#14 0x2eea0f2 in testing::Test::Run() testing/gtest/src/gtest.cc:2057
#15 0x2eec3a9 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237
#16 0x2eed183 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344
#17 0x2efe463 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065
#18 0x2efda30 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045
#19 0x2efda30 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697
#20 0x2e7712c in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231
#21 0x2e7712c in base::TestSuite::Run() base/test/test_suite.cc:213
#22 0x2e6aebd in Run base/callback.h:401
#23 0x2e6aebd in base::(anonymous namespace)::LaunchUnitTestsInternal(int, char**, base::Callback<int ()> const&, int) base/test/launcher/unit_test_launcher.cc:494
#24 0x1bfec66 in main content/test/run_all_unittests.cc:14
#25 0x7f853fc5876c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
> Introduce RenderFrameProxyHost object and use it in RFHM.
>
> This is the first CL in a series to create RenderFrameProxy(Host) infrastructure. Before the Blink codebase is ready to transform local and remote frames, the proxy objects will keep internally the existing RF/RFH in swapped out state. This CL creates the browser side proxy object and wraps the swapped out RFH.
>
> BUG=357747
>
> Review URL: https://codereview.chromium.org/217163007
TBR=nasko@chromium.org
Review URL: https://codereview.chromium.org/236003002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@263367 0039d316-1c4b-4281-b951-d872f2087c98
| -rw-r--r-- | content/browser/frame_host/render_frame_host_manager.cc | 252 | ||||
| -rw-r--r-- | content/browser/frame_host/render_frame_host_manager.h | 29 | ||||
| -rw-r--r-- | content/browser/frame_host/render_frame_host_manager_unittest.cc | 81 | ||||
| -rw-r--r-- | content/browser/frame_host/render_frame_proxy_host.cc | 19 | ||||
| -rw-r--r-- | content/browser/frame_host/render_frame_proxy_host.h | 88 | ||||
| -rw-r--r-- | content/content_browser.gypi | 2 |
6 files changed, 136 insertions, 335 deletions
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc index 4c32bf3..b9c5be3 100644 --- a/content/browser/frame_host/render_frame_host_manager.cc +++ b/content/browser/frame_host/render_frame_host_manager.cc @@ -20,7 +20,6 @@ #include "content/browser/frame_host/navigator.h" #include "content/browser/frame_host/render_frame_host_factory.h" #include "content/browser/frame_host/render_frame_host_impl.h" -#include "content/browser/frame_host/render_frame_proxy_host.h" #include "content/browser/renderer_host/render_process_host_impl.h" #include "content/browser/renderer_host/render_view_host_factory.h" #include "content/browser/renderer_host/render_view_host_impl.h" @@ -90,9 +89,11 @@ RenderFrameHostManager::~RenderFrameHostManager() { // We should always have a current RenderFrameHost except in some tests. render_frame_host_.reset(); + // TODO(creis): Now that we aren't using Shutdown, make RenderFrameHostMap + // use scoped_ptrs. // Delete any swapped out RenderFrameHosts. - for (RenderFrameProxyHostMap::iterator iter = proxy_hosts_.begin(); - iter != proxy_hosts_.end(); + for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin(); + iter != swapped_out_hosts_.end(); ++iter) { delete iter->second; } @@ -108,11 +109,9 @@ void RenderFrameHostManager::Init(BrowserContext* browser_context, if (!site_instance) site_instance = SiteInstance::Create(browser_context); - render_frame_host_ = CreateRenderFrameHost(site_instance, - view_routing_id, - frame_routing_id, - false, - delegate_->IsHidden()); + render_frame_host_ = make_scoped_ptr( + CreateRenderFrameHost(site_instance, view_routing_id, frame_routing_id, + false, delegate_->IsHidden())); // Keep track of renderer processes as they start to shut down or are // crashed/killed. @@ -448,8 +447,8 @@ void RenderFrameHostManager::DidNavigateFrame( // TODO(creis): Take in RenderFrameHost instead, since frames can have openers. void RenderFrameHostManager::DidDisownOpener(RenderViewHost* render_view_host) { // Notify all swapped out hosts, including the pending RVH. - for (RenderFrameProxyHostMap::iterator iter = proxy_hosts_.begin(); - iter != proxy_hosts_.end(); + for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin(); + iter != swapped_out_hosts_.end(); ++iter) { DCHECK_NE(iter->second->GetSiteInstance(), current_frame_host()->GetSiteInstance()); @@ -463,8 +462,8 @@ void RenderFrameHostManager::RendererProcessClosing( // swap them back in while the process is exiting. Start by finding them, // since there could be more than one. std::list<int> ids_to_remove; - for (RenderFrameProxyHostMap::iterator iter = proxy_hosts_.begin(); - iter != proxy_hosts_.end(); + for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin(); + iter != swapped_out_hosts_.end(); ++iter) { if (iter->second->GetProcess() == render_process_host) ids_to_remove.push_back(iter->first); @@ -472,8 +471,8 @@ void RenderFrameHostManager::RendererProcessClosing( // Now delete them. while (!ids_to_remove.empty()) { - delete proxy_hosts_[ids_to_remove.back()]; - proxy_hosts_.erase(ids_to_remove.back()); + delete swapped_out_hosts_[ids_to_remove.back()]; + swapped_out_hosts_.erase(ids_to_remove.back()); ids_to_remove.pop_back(); } } @@ -549,36 +548,34 @@ void RenderFrameHostManager::Observe( } } -bool RenderFrameHostManager::ClearProxiesInSiteInstance( +bool RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance( int32 site_instance_id, FrameTreeNode* node) { - RenderFrameProxyHostMap::iterator iter = - node->render_manager()->proxy_hosts_.find(site_instance_id); - if (iter != node->render_manager()->proxy_hosts_.end()) { - RenderFrameProxyHost* proxy = iter->second; + RenderFrameHostMap::iterator iter = + node->render_manager()->swapped_out_hosts_.find(site_instance_id); + if (iter != node->render_manager()->swapped_out_hosts_.end()) { + RenderFrameHostImpl* swapped_out_rfh = iter->second; // If the RVH is pending swap out, it needs to switch state to // pending shutdown. Otherwise it is deleted. - if (proxy->render_view_host()->rvh_state() == + if (swapped_out_rfh->render_view_host()->rvh_state() == RenderViewHostImpl::STATE_PENDING_SWAP_OUT) { - scoped_ptr<RenderFrameHostImpl> swapped_out_rfh = proxy->PassFrameHost(); - swapped_out_rfh->SetPendingShutdown(base::Bind( &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance, node->render_manager()->weak_factory_.GetWeakPtr(), site_instance_id, - swapped_out_rfh.get())); + swapped_out_rfh)); RFHPendingDeleteMap::iterator pending_delete_iter = node->render_manager()->pending_delete_hosts_.find(site_instance_id); if (pending_delete_iter == node->render_manager()->pending_delete_hosts_.end() || - pending_delete_iter->second.get() != swapped_out_rfh) { + pending_delete_iter->second.get() != iter->second) { node->render_manager()->pending_delete_hosts_[site_instance_id] = - linked_ptr<RenderFrameHostImpl>(swapped_out_rfh.release()); + linked_ptr<RenderFrameHostImpl>(swapped_out_rfh); } } else { - delete proxy; + delete swapped_out_rfh; } - node->render_manager()->proxy_hosts_.erase(site_instance_id); + node->render_manager()->swapped_out_hosts_.erase(site_instance_id); } return true; @@ -836,7 +833,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry( return current_instance->GetRelatedSiteInstance(dest_url); } -scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost( +RenderFrameHostImpl* RenderFrameHostManager::CreateRenderFrameHost( SiteInstance* site_instance, int view_routing_id, int frame_routing_id, @@ -865,15 +862,16 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost( } } + // TODO(creis): Make render_frame_host a scoped_ptr. // TODO(creis): Pass hidden to RFH. - scoped_ptr<RenderFrameHostImpl> render_frame_host = - make_scoped_ptr(RenderFrameHostFactory::Create(render_view_host, - render_frame_delegate_, - frame_tree, - frame_tree_node_, - frame_routing_id, - swapped_out).release()); - return render_frame_host.Pass(); + RenderFrameHostImpl* render_frame_host = + RenderFrameHostFactory::Create(render_view_host, + render_frame_delegate_, + frame_tree, + frame_tree_node_, + frame_routing_id, + swapped_out).release(); + return render_frame_host; } int RenderFrameHostManager::CreateRenderFrame( @@ -884,9 +882,6 @@ int RenderFrameHostManager::CreateRenderFrame( CHECK(instance); DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden. - scoped_ptr<RenderFrameHostImpl> new_render_frame_host; - int routing_id = MSG_ROUTING_NONE; - // We are creating a pending or swapped out RFH here. We should never create // it in the same SiteInstance as our current RFH. CHECK_NE(render_frame_host_->GetSiteInstance(), instance); @@ -894,22 +889,17 @@ int RenderFrameHostManager::CreateRenderFrame( // Check if we've already created an RFH for this SiteInstance. If so, try // to re-use the existing one, which has already been initialized. We'll // remove it from the list of swapped out hosts if it commits. - RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance); + RenderFrameHostImpl* new_render_frame_host = + GetSwappedOutRenderFrameHost(instance); FrameTreeNode* parent_node = NULL; if (frame_tree_node_) parent_node = frame_tree_node_->parent(); - if (proxy) { - routing_id = proxy->render_view_host()->GetRoutingID(); - // Delete the existing RenderFrameProxyHost, but reuse the RenderFrameHost. + if (new_render_frame_host) { // Prevent the process from exiting while we're trying to use it. if (!swapped_out) { - new_render_frame_host = proxy->PassFrameHost(); new_render_frame_host->GetProcess()->AddPendingView(); - - proxy_hosts_.erase(instance->GetId()); - delete proxy; } else { // Detect if this is a cross-process child frame that is navigating // back to the same SiteInstance as its parent. @@ -922,20 +912,21 @@ int RenderFrameHostManager::CreateRenderFrame( } } else { // Create a new RenderFrameHost if we don't find an existing one. - new_render_frame_host = CreateRenderFrameHost( - instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE, swapped_out, hidden); - RenderViewHostImpl* render_view_host = - new_render_frame_host->render_view_host(); - - // Prevent the process from exiting while we're trying to navigate in it. - // Otherwise, if the new RFH is swapped out already, store it. - if (!swapped_out) { - new_render_frame_host->GetProcess()->AddPendingView(); + // TODO(creis): Make new_render_frame_host a scoped_ptr. + new_render_frame_host = CreateRenderFrameHost(instance, MSG_ROUTING_NONE, + MSG_ROUTING_NONE, swapped_out, + hidden); + + // If the new RFH is swapped out already, store it. Otherwise prevent the + // process from exiting while we're trying to navigate in it. + if (swapped_out) { + swapped_out_hosts_[instance->GetId()] = new_render_frame_host; } else { - proxy_hosts_[instance->GetId()] = new RenderFrameProxyHost( - new_render_frame_host.Pass()); + new_render_frame_host->GetProcess()->AddPendingView(); } + RenderViewHostImpl* render_view_host = + new_render_frame_host->render_view_host(); bool success = InitRenderView(render_view_host, opener_route_id); if (success && frame_tree_node_->IsMainFrame()) { // Don't show the main frame's view until we get a DidNavigate from it. @@ -943,14 +934,13 @@ int RenderFrameHostManager::CreateRenderFrame( } else if (!swapped_out && pending_render_frame_host_) { CancelPending(); } - routing_id = render_view_host->GetRoutingID(); } // Use this as our new pending RFH if it isn't swapped out. if (!swapped_out) - pending_render_frame_host_ = new_render_frame_host.Pass(); + pending_render_frame_host_.reset(new_render_frame_host); - return routing_id; + return new_render_frame_host->render_view_host()->GetRoutingID(); } bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host, @@ -1027,8 +1017,7 @@ void RenderFrameHostManager::CommitPending() { // Swap in the pending frame and make it active. Also ensure the FrameTree // stays in sync. - scoped_ptr<RenderFrameHostImpl> old_render_frame_host = - render_frame_host_.Pass(); + RenderFrameHostImpl* old_render_frame_host = render_frame_host_.release(); render_frame_host_ = pending_render_frame_host_.Pass(); if (is_main_frame) render_frame_host_->render_view_host()->AttachToFrameTree(); @@ -1057,7 +1046,7 @@ void RenderFrameHostManager::CommitPending() { &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance, weak_factory_.GetWeakPtr(), old_site_instance_id, - old_render_frame_host.get())); + old_render_frame_host)); } else { // TODO(creis): We'll need to set this back to false if we navigate back. old_render_frame_host->set_swapped_out(true); @@ -1085,45 +1074,47 @@ void RenderFrameHostManager::CommitPending() { render_frame_host_->render_view_host()); } - // If the old RFH is not live, just return as there is no work to do. - if (!old_render_frame_host->render_view_host()->IsRenderViewLive()) { - return; - } - - // If the old RFH is live, we are swapping it out and should keep track of - // it in case we navigate back to it, or it is waiting for the unload event - // to execute in the background. - // TODO(creis): Swap out the subframe in --site-per-process. - if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) - DCHECK(old_render_frame_host->is_swapped_out() || - !RenderViewHostImpl::IsRVHStateActive( - old_render_frame_host->render_view_host()->rvh_state())); - // Temp fix for http://crbug.com/90867 until we do a better cleanup to make - // sure we don't get different rvh instances for the same site instance - // in the same rvhmgr. - // TODO(creis): Clean this up. - RenderFrameProxyHostMap::iterator iter = - proxy_hosts_.find(old_site_instance_id); - if (iter != proxy_hosts_.end() && - iter->second->render_frame_host() != old_render_frame_host) { - // Delete the proxy that will be replaced in the map to avoid a leak. - delete iter->second; - } - - // If the RenderViewHost backing the RenderFrameHost is pending shutdown, - // the RenderFrameHost should be put in the map of RenderFrameHosts pending - // shutdown. Otherwise, it is stored in the map of proxy hosts. - if (old_render_frame_host->render_view_host()->rvh_state() == - RenderViewHostImpl::STATE_PENDING_SHUTDOWN) { - proxy_hosts_.erase(old_site_instance_id); - RFHPendingDeleteMap::iterator pending_delete_iter = - pending_delete_hosts_.find(old_site_instance_id); - if (pending_delete_iter == pending_delete_hosts_.end() || - pending_delete_iter->second.get() != old_render_frame_host) { - pending_delete_hosts_[old_site_instance_id] = - linked_ptr<RenderFrameHostImpl>(old_render_frame_host.release()); + // If the pending frame was on the swapped out list, we can remove it. + swapped_out_hosts_.erase(render_frame_host_->GetSiteInstance()->GetId()); + + if (old_render_frame_host->render_view_host()->IsRenderViewLive()) { + // If the old RFH is live, we are swapping it out and should keep track of + // it in case we navigate back to it, or it is waiting for the unload event + // to execute in the background. + // TODO(creis): Swap out the subframe in --site-per-process. + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) + DCHECK(old_render_frame_host->is_swapped_out() || + !RenderViewHostImpl::IsRVHStateActive( + old_render_frame_host->render_view_host()->rvh_state())); + // Temp fix for http://crbug.com/90867 until we do a better cleanup to make + // sure we don't get different rvh instances for the same site instance + // in the same rvhmgr. + // TODO(creis): Clean this up. + RenderFrameHostMap::iterator iter = + swapped_out_hosts_.find(old_site_instance_id); + if (iter != swapped_out_hosts_.end() && + iter->second != old_render_frame_host) { + // Delete the RFH that will be replaced in the map to avoid a leak. + delete iter->second; } - } else { + // If the RenderViewHost backing the RenderFrameHost is pending shutdown, + // the RenderFrameHost should be put in the map of RenderFrameHosts pending + // shutdown. Otherwise, it is stored in the map of swapped out + // RenderFrameHosts. + if (old_render_frame_host->render_view_host()->rvh_state() == + RenderViewHostImpl::STATE_PENDING_SHUTDOWN) { + swapped_out_hosts_.erase(old_site_instance_id); + RFHPendingDeleteMap::iterator pending_delete_iter = + pending_delete_hosts_.find(old_site_instance_id); + if (pending_delete_iter == pending_delete_hosts_.end() || + pending_delete_iter->second.get() != old_render_frame_host) { + pending_delete_hosts_[old_site_instance_id] = + linked_ptr<RenderFrameHostImpl>(old_render_frame_host); + } + } else { + swapped_out_hosts_[old_site_instance_id] = old_render_frame_host; + } + // If there are no active views in this SiteInstance, it means that // this RFH was the last active one in the SiteInstance. Now that we // know that all RFHs are swapped out, we can delete all the RFHs and RVHs @@ -1131,23 +1122,23 @@ void RenderFrameHostManager::CommitPending() { // swapped out list to simplify the deletion. if (!static_cast<SiteInstanceImpl*>( old_render_frame_host->GetSiteInstance())->active_view_count()) { - old_render_frame_host.reset(); ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id); - } else { - proxy_hosts_[old_site_instance_id] = new RenderFrameProxyHost( - old_render_frame_host.Pass()); + // This is deleted while cleaning up the SiteInstance's views. + old_render_frame_host = NULL; } + } else { + delete old_render_frame_host; } } void RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance( int32 site_instance_id) { // First remove any swapped out RFH for this SiteInstance from our own list. - ClearProxiesInSiteInstance(site_instance_id, frame_tree_node_); + ClearSwappedOutRFHsInSiteInstance(site_instance_id, frame_tree_node_); // Use the safe RenderWidgetHost iterator for now to find all RenderViewHosts // in the SiteInstance, then tell their respective FrameTrees to remove all - // RenderFrameProxyHosts corresponding to them. + // swapped out RenderFrameHosts corresponding to them. // TODO(creis): Replace this with a RenderFrameHostIterator that protects // against use-after-frees if a later element is deleted before getting to it. scoped_ptr<RenderWidgetHostIterator> widgets( @@ -1162,7 +1153,7 @@ void RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance( // |rvh| to Shutdown. FrameTree* tree = rvh->GetDelegate()->GetFrameTree(); tree->ForEach(base::Bind( - &RenderFrameHostManager::ClearProxiesInSiteInstance, + &RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance, site_instance_id)); } } @@ -1340,11 +1331,11 @@ void RenderFrameHostManager::CancelPending() { // We no longer need to prevent the process from exiting. pending_render_frame_host->GetProcess()->RemovePendingView(); - // If the SiteInstance for the pending RFH is being used by others, don't - // delete the RFH, just swap it out and it can be reused at a later point. - SiteInstanceImpl* site_instance = static_cast<SiteInstanceImpl*>( - pending_render_frame_host->GetSiteInstance()); - if (site_instance->active_view_count() > 1) { + // The pending RFH may already be on the swapped out list if we started to + // swap it back in and then canceled. If so, make sure it gets swapped out + // again. If it's not on the swapped out list (e.g., aborting a pending + // load), then it's safe to shut down. + if (IsOnSwappedOutList(pending_render_frame_host)) { // Any currently suspended navigations are no longer needed. pending_render_frame_host->render_view_host()->CancelSuspendedNavigations(); @@ -1378,11 +1369,11 @@ void RenderFrameHostManager::RenderViewDeleted(RenderViewHost* rvh) { return; // We can't look it up by SiteInstance ID, which may no longer be valid. - for (RenderFrameProxyHostMap::iterator iter = proxy_hosts_.begin(); - iter != proxy_hosts_.end(); + for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin(); + iter != swapped_out_hosts_.end(); ++iter) { if (iter->second->render_view_host() == rvh) { - proxy_hosts_.erase(iter); + swapped_out_hosts_.erase(iter); break; } } @@ -1390,11 +1381,11 @@ void RenderFrameHostManager::RenderViewDeleted(RenderViewHost* rvh) { bool RenderFrameHostManager::IsRVHOnSwappedOutList( RenderViewHostImpl* rvh) const { - RenderFrameProxyHost* proxy = GetRenderFrameProxyHost( + RenderFrameHostImpl* render_frame_host = GetSwappedOutRenderFrameHost( rvh->GetSiteInstance()); - if (!proxy) + if (!render_frame_host) return false; - return IsOnSwappedOutList(proxy->render_frame_host()); + return IsOnSwappedOutList(render_frame_host); } bool RenderFrameHostManager::IsOnSwappedOutList( @@ -1402,27 +1393,28 @@ bool RenderFrameHostManager::IsOnSwappedOutList( if (!rfh->GetSiteInstance()) return false; - RenderFrameProxyHostMap::const_iterator iter = proxy_hosts_.find( + RenderFrameHostMap::const_iterator iter = swapped_out_hosts_.find( rfh->GetSiteInstance()->GetId()); - if (iter == proxy_hosts_.end()) + if (iter == swapped_out_hosts_.end()) return false; - return iter->second->render_frame_host() == rfh; + return iter->second == rfh; } RenderViewHostImpl* RenderFrameHostManager::GetSwappedOutRenderViewHost( SiteInstance* instance) const { - RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance); - if (proxy) - return proxy->render_view_host(); + RenderFrameHostImpl* render_frame_host = + GetSwappedOutRenderFrameHost(instance); + if (render_frame_host) + return render_frame_host->render_view_host(); return NULL; } -RenderFrameProxyHost* RenderFrameHostManager::GetRenderFrameProxyHost( +RenderFrameHostImpl* RenderFrameHostManager::GetSwappedOutRenderFrameHost( SiteInstance* instance) const { - RenderFrameProxyHostMap::const_iterator iter = - proxy_hosts_.find(instance->GetId()); - if (iter != proxy_hosts_.end()) + RenderFrameHostMap::const_iterator iter = + swapped_out_hosts_.find(instance->GetId()); + if (iter != swapped_out_hosts_.end()) return iter->second; return NULL; diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h index c2ed883..b06e2e3 100644 --- a/content/browser/frame_host/render_frame_host_manager.h +++ b/content/browser/frame_host/render_frame_host_manager.h @@ -30,7 +30,6 @@ class NavigationEntryImpl; class RenderFrameHostDelegate; class RenderFrameHostImpl; class RenderFrameHostManagerTest; -class RenderFrameProxyHost; class RenderViewHost; class RenderViewHostImpl; class RenderWidgetHostDelegate; @@ -269,10 +268,9 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { bool IsOnSwappedOutList(RenderFrameHostImpl* rfh) const; // Returns the swapped out RenderViewHost or RenderFrameHost for the given - // SiteInstance, if any. This method is *deprecated* and - // GetRenderFrameProxyHost should be used. + // SiteInstance, if any. RenderViewHostImpl* GetSwappedOutRenderViewHost(SiteInstance* instance) const; - RenderFrameProxyHost* GetRenderFrameProxyHost( + RenderFrameHostImpl* GetSwappedOutRenderFrameHost( SiteInstance* instance) const; // Runs the unload handler in the current page, when we know that a pending @@ -333,10 +331,10 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { bool should_replace_current_entry; }; - // Used with FrameTree::ForEach to erase RenderFrameProxyHosts from a + // Used with FrameTree::ForEach to erase inactive RenderFrameHosts from a // FrameTreeNode's RenderFrameHostManager. - static bool ClearProxiesInSiteInstance(int32 site_instance_id, - FrameTreeNode* node); + static bool ClearSwappedOutRFHsInSiteInstance(int32 site_instance_id, + FrameTreeNode* node); // Returns whether this tab should transition to a new renderer for // cross-site URLs. Enabled unless we see the --process-per-tab command line @@ -369,11 +367,11 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { bool force_browsing_instance_swap); // Creates a RenderFrameHost and corresponding RenderViewHost if necessary. - scoped_ptr<RenderFrameHostImpl> CreateRenderFrameHost(SiteInstance* instance, - int view_routing_id, - int frame_routing_id, - bool swapped_out, - bool hidden); + RenderFrameHostImpl* CreateRenderFrameHost(SiteInstance* instance, + int view_routing_id, + int frame_routing_id, + bool swapped_out, + bool hidden); // Sets up the necessary state for a new RenderViewHost with the given opener, // if necessary. Returns early if the RenderViewHost has already been @@ -449,9 +447,10 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver { scoped_ptr<WebUIImpl> pending_web_ui_; base::WeakPtr<WebUIImpl> pending_and_current_web_ui_; - // A map of site instance ID to RenderFrameProxyHosts. - typedef base::hash_map<int32, RenderFrameProxyHost*> RenderFrameProxyHostMap; - RenderFrameProxyHostMap proxy_hosts_; + // A map of site instance ID to swapped out RenderFrameHosts. This may + // include pending_render_frame_host_ for navigations to existing entries. + typedef base::hash_map<int32, RenderFrameHostImpl*> RenderFrameHostMap; + RenderFrameHostMap swapped_out_hosts_; // A map of RenderFrameHosts pending shutdown. typedef base::hash_map<int32, linked_ptr<RenderFrameHostImpl> > diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc index 72e8b3a..d43c63f 100644 --- a/content/browser/frame_host/render_frame_host_manager_unittest.cc +++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc @@ -125,37 +125,6 @@ class RenderViewHostDeletedObserver : public WebContentsObserver { }; -// This observer keeps track of the last deleted RenderViewHost to avoid -// accessing it and causing use-after-free condition. -class RenderFrameHostDeletedObserver : public WebContentsObserver { - public: - RenderFrameHostDeletedObserver(RenderFrameHost* rfh) - : WebContentsObserver(WebContents::FromRenderFrameHost(rfh)), - process_id_(rfh->GetProcess()->GetID()), - routing_id_(rfh->GetRoutingID()), - deleted_(false) { - } - - virtual void RenderFrameDeleted(RenderFrameHost* render_frame_host) OVERRIDE { - if (render_frame_host->GetProcess()->GetID() == process_id_ && - render_frame_host->GetRoutingID() == routing_id_) { - deleted_ = true; - } - } - - bool deleted() { - return deleted_; - } - - private: - int process_id_; - int routing_id_; - bool deleted_; - - DISALLOW_COPY_AND_ASSIGN(RenderFrameHostDeletedObserver); -}; - - // This observer is used to check whether IPC messages are being filtered for // swapped out RenderFrameHost objects. It observes the plugin crash and favicon // update events, which the FilterMessagesWhileSwappedOut test simulates being @@ -1796,54 +1765,4 @@ TEST_F(RenderFrameHostManagerTest, EXPECT_TRUE(rvh1->IsSwappedOut()); } -// Test that a RenderFrameHost is properly deleted or swapped out when a -// cross-site navigation is cancelled. -TEST_F(RenderFrameHostManagerTest, - CancelPendingProperlyDeletesOrSwaps) { - const GURL kUrl1("http://www.google.com/"); - const GURL kUrl2("http://www.chromium.org/"); - RenderFrameHostImpl* pending_rfh = NULL; - base::TimeTicks now = base::TimeTicks::Now(); - - // Navigate to the first page. - contents()->NavigateAndCommit(kUrl1); - TestRenderViewHost* rvh1 = test_rvh(); - EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh1->rvh_state()); - - // Navigate to a new site, starting a cross-site navigation. - controller().LoadURL(kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string()); - { - pending_rfh = contents()->GetFrameTree()->root()->render_manager() - ->pending_frame_host(); - RenderFrameHostDeletedObserver rvh_deleted_observer(pending_rfh); - - // Cancel the navigation by simulating a declined beforeunload dialog. - main_test_rfh()->OnMessageReceived( - FrameHostMsg_BeforeUnload_ACK(0, false, now, now)); - EXPECT_FALSE(contents()->cross_navigation_pending()); - - // Since the pending RFH is the only one for the new SiteInstance, it should - // be deleted. - EXPECT_TRUE(rvh_deleted_observer.deleted()); - } - - // Start another cross-site navigation. - controller().LoadURL(kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string()); - { - pending_rfh = contents()->GetFrameTree()->root()->render_manager() - ->pending_frame_host(); - RenderFrameHostDeletedObserver rvh_deleted_observer(pending_rfh); - - // Increment the number of active views in the new SiteInstance, which will - // cause the pending RFH to be swapped out instead of deleted. - static_cast<SiteInstanceImpl*>( - pending_rfh->GetSiteInstance())->increment_active_view_count(); - - main_test_rfh()->OnMessageReceived( - FrameHostMsg_BeforeUnload_ACK(0, false, now, now)); - EXPECT_FALSE(contents()->cross_navigation_pending()); - EXPECT_FALSE(rvh_deleted_observer.deleted()); - } -} - } // namespace content diff --git a/content/browser/frame_host/render_frame_proxy_host.cc b/content/browser/frame_host/render_frame_proxy_host.cc deleted file mode 100644 index e965fda..0000000 --- a/content/browser/frame_host/render_frame_proxy_host.cc +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "content/browser/frame_host/render_frame_proxy_host.h" - -#include "content/browser/frame_host/render_frame_host_impl.h" -#include "content/browser/site_instance_impl.h" - -namespace content { - -RenderFrameProxyHost::RenderFrameProxyHost( - scoped_ptr<RenderFrameHostImpl> render_frame_host) - : site_instance_(render_frame_host->GetSiteInstance()), - render_frame_host_(render_frame_host.Pass()) {} - -RenderFrameProxyHost::~RenderFrameProxyHost() {} - -} // namespace content diff --git a/content/browser/frame_host/render_frame_proxy_host.h b/content/browser/frame_host/render_frame_proxy_host.h deleted file mode 100644 index 514f0b2..0000000 --- a/content/browser/frame_host/render_frame_proxy_host.h +++ /dev/null @@ -1,88 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_PROXY_HOST_H_ -#define CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_PROXY_HOST_H_ - -#include "base/memory/scoped_ptr.h" -#include "content/browser/frame_host/render_frame_host_impl.h" -#include "content/browser/site_instance_impl.h" - -class RenderProcessHost; -class RenderFrameHostImpl; -class RenderViewHostImpl; - -namespace content { - -// When a page's frames are rendered by multiple processes, each renderer has a -// full copy of the frame tree. It has full RenderFrames for the frames it is -// responsible for rendering and placeholder objects (i.e., RenderFrameProxies) -// for frames rendered by other processes. -// -// This class is the browser-side host object for the placeholder. Each node in -// the frame tree has a RenderFrameHost for the active SiteInstance and a set -// of RenderFrameProxyHost objects - one for all other SiteInstances with -// references to this frame. The proxies allow us to keep existing window -// references valid over cross-process navigations and route cross-site -// asynchronous JavaScript calls, such as postMessage. -// -// For now, RenderFrameProxyHost is created when a RenderFrameHost is swapped -// out and acts just as a wrapper. If a RenderFrameHost can be deleted, no -// proxy object is created. It is destroyed when the RenderFrameHost is swapped -// back in or is no longer referenced and is therefore deleted. -// -// Long term, RenderFrameProxyHost will be created whenever a cross-site -// navigation occurs and a reference to the frame navigating needs to be kept -// alive. A RenderFrameProxyHost and a RenderFrameHost for the same SiteInstance -// can exist at the same time, but only one will be "active" at a time. -// There are two cases where the two objects will coexist: -// * When navigating cross-process and there is already a RenderFrameProxyHost -// for the new SiteInstance. A pending RenderFrameHost is created, but it is -// not used until it commits. At that point, RenderFrameHostManager transitions -// the pending RenderFrameHost to the active one and deletes the proxy. -// * When navigating cross-process and the existing document has an unload -// event handler. When the new navigation commits, RenderFrameHostManager -// creates a RenderFrameProxyHost for the old SiteInstance and uses it going -// forward. It also instructs the RenderFrameHost to run the unload event -// handler and is kept alive for the duration. Once the event handling is -// complete, the RenderFrameHost is deleted. -class RenderFrameProxyHost { - public: - explicit RenderFrameProxyHost( - scoped_ptr<RenderFrameHostImpl> render_frame_host); - ~RenderFrameProxyHost(); - - RenderProcessHost* GetProcess() { - return render_frame_host_->GetProcess(); - } - - SiteInstance* GetSiteInstance() { - return site_instance_.get(); - } - - // TODO(nasko): The following methods should be removed when swapping out - // of RenderFrameHosts is no longer used. - RenderFrameHostImpl* render_frame_host() { - return render_frame_host_.get(); - } - RenderViewHostImpl* render_view_host() { - return render_frame_host_->render_view_host(); - } - scoped_ptr<RenderFrameHostImpl> PassFrameHost() { - return render_frame_host_.Pass(); - } - - private: - scoped_refptr<SiteInstance> site_instance_; - - // TODO(nasko): For now, hide the RenderFrameHost inside the proxy, but remove - // it once we have all the code support for proper proxy objects. - scoped_ptr<RenderFrameHostImpl> render_frame_host_; - - DISALLOW_COPY_AND_ASSIGN(RenderFrameProxyHost); -}; - -} // namespace - -#endif // CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_PROXY_HOST_H_ diff --git a/content/content_browser.gypi b/content/content_browser.gypi index de5ccd9..dcac8c8 100644 --- a/content/content_browser.gypi +++ b/content/content_browser.gypi @@ -566,8 +566,6 @@ 'browser/frame_host/render_frame_host_manager.h', 'browser/frame_host/render_frame_message_filter.cc', 'browser/frame_host/render_frame_message_filter.h', - 'browser/frame_host/render_frame_proxy_host.cc', - 'browser/frame_host/render_frame_proxy_host.h', 'browser/frame_host/render_widget_host_view_child_frame.cc', 'browser/frame_host/render_widget_host_view_child_frame.h', 'browser/frame_host/render_widget_host_view_guest.cc', |