diff options
12 files changed, 274 insertions, 114 deletions
diff --git a/content/browser/frame_host/frame_tree.cc b/content/browser/frame_host/frame_tree.cc index 0dff87b..cb0f453 100644 --- a/content/browser/frame_host/frame_tree.cc +++ b/content/browser/frame_host/frame_tree.cc @@ -196,7 +196,9 @@ RenderViewHostImpl* FrameTree::GetRenderViewHostForSubFrame( SiteInstance* site_instance) { RenderViewHostMap::iterator iter = render_view_host_map_.find(site_instance->GetId()); - CHECK(iter != render_view_host_map_.end()); + // TODO(creis): Mirror the frame tree so this check can't fail. + if (iter == render_view_host_map_.end()) + return NULL; RenderViewHostRefCount rvh_refcount = iter->second; return rvh_refcount.first; } diff --git a/content/browser/frame_host/frame_tree.h b/content/browser/frame_host/frame_tree.h index 2ee1cdd..8aa85bb 100644 --- a/content/browser/frame_host/frame_tree.h +++ b/content/browser/frame_host/frame_tree.h @@ -124,15 +124,15 @@ class CONTENT_EXPORT FrameTree { void RegisterRenderFrameHost(RenderFrameHostImpl* render_frame_host); void UnregisterRenderFrameHost(RenderFrameHostImpl* render_frame_host); + // Returns the FrameTreeNode with the given renderer-specific |frame_id|. + // TODO(creis): Make this private and replace this with a version that takes + // in a routing ID. + FrameTreeNode* FindByFrameID(int64 frame_id); + private: typedef std::pair<RenderViewHostImpl*, int> RenderViewHostRefCount; typedef base::hash_map<int, RenderViewHostRefCount> RenderViewHostMap; - // Returns the FrameTreeNode with the given renderer-specific |frame_id|. - // For internal use only. - // TODO(creis): Replace this with a version that takes in a routing ID. - FrameTreeNode* FindByFrameID(int64 frame_id); - // These delegates are installed into all the RenderViewHosts and // RenderFrameHosts that we create. RenderFrameHostDelegate* render_frame_delegate_; diff --git a/content/browser/frame_host/navigation_controller_delegate.h b/content/browser/frame_host/navigation_controller_delegate.h index 886b3f9..404e927 100644 --- a/content/browser/frame_host/navigation_controller_delegate.h +++ b/content/browser/frame_host/navigation_controller_delegate.h @@ -36,7 +36,6 @@ class NavigationControllerDelegate { virtual const std::string& GetContentsMimeType() const = 0; virtual void NotifyNavigationStateChanged(unsigned changed_flags) = 0; virtual void Stop() = 0; - virtual SiteInstance* GetSiteInstance() const = 0; virtual SiteInstance* GetPendingSiteInstance() const = 0; virtual int32 GetMaxPageID() = 0; virtual int32 GetMaxPageIDForSiteInstance(SiteInstance* site_instance) = 0; diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc index f7bfa5b..d326f76 100644 --- a/content/browser/frame_host/navigation_controller_impl.cc +++ b/content/browser/frame_host/navigation_controller_impl.cc @@ -724,6 +724,7 @@ void NavigationControllerImpl::LoadURLWithParams(const LoadURLParams& params) { } bool NavigationControllerImpl::RendererDidNavigate( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params, LoadCommittedDetails* details) { is_initial_navigation_ = false; @@ -751,7 +752,7 @@ bool NavigationControllerImpl::RendererDidNavigate( pending_entry_ && pending_entry_->should_replace_entry(); // Do navigation-type specific actions. These will make and commit an entry. - details->type = ClassifyNavigation(params); + details->type = ClassifyNavigation(rvh, params); // is_in_page must be computed before the entry gets committed. details->is_in_page = IsURLInPageNavigation( @@ -759,22 +760,22 @@ bool NavigationControllerImpl::RendererDidNavigate( switch (details->type) { case NAVIGATION_TYPE_NEW_PAGE: - RendererDidNavigateToNewPage(params, details->did_replace_entry); + RendererDidNavigateToNewPage(rvh, params, details->did_replace_entry); break; case NAVIGATION_TYPE_EXISTING_PAGE: - RendererDidNavigateToExistingPage(params); + RendererDidNavigateToExistingPage(rvh, params); break; case NAVIGATION_TYPE_SAME_PAGE: - RendererDidNavigateToSamePage(params); + RendererDidNavigateToSamePage(rvh, params); break; case NAVIGATION_TYPE_IN_PAGE: - RendererDidNavigateInPage(params, &details->did_replace_entry); + RendererDidNavigateInPage(rvh, params, &details->did_replace_entry); break; case NAVIGATION_TYPE_NEW_SUBFRAME: - RendererDidNavigateNewSubframe(params); + RendererDidNavigateNewSubframe(rvh, params); break; case NAVIGATION_TYPE_AUTO_SUBFRAME: - if (!RendererDidNavigateAutoSubframe(params)) + if (!RendererDidNavigateAutoSubframe(rvh, params)) return false; break; case NAVIGATION_TYPE_NAV_IGNORE: @@ -819,12 +820,14 @@ bool NavigationControllerImpl::RendererDidNavigate( active_entry->ResetForCommit(); // The active entry's SiteInstance should match our SiteInstance. - CHECK(active_entry->site_instance() == delegate_->GetSiteInstance()); + // TODO(creis): This check won't pass for subframes until we create entries + // for subframe navigations. + if (PageTransitionIsMainFrame(params.transition)) + CHECK(active_entry->site_instance() == rvh->GetSiteInstance()); // Remember the bindings the renderer process has at this point, so that // we do not grant this entry additional bindings if we come back to it. - active_entry->SetBindings( - delegate_->GetRenderViewHost()->GetEnabledBindings()); + active_entry->SetBindings(rvh->GetEnabledBindings()); // Now prep the rest of the details for the notification and broadcast. details->entry = active_entry; @@ -838,6 +841,7 @@ bool NavigationControllerImpl::RendererDidNavigate( } NavigationType NavigationControllerImpl::ClassifyNavigation( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params) const { if (params.page_id == -1) { // The renderer generates the page IDs, and so if it gives us the invalid @@ -861,7 +865,8 @@ NavigationType NavigationControllerImpl::ClassifyNavigation( return NAVIGATION_TYPE_NAV_IGNORE; } - if (params.page_id > delegate_->GetMaxPageID()) { + if (params.page_id > delegate_->GetMaxPageIDForSiteInstance( + rvh->GetSiteInstance())) { // Greater page IDs than we've ever seen before are new pages. We may or may // not have a pending entry for the page, and this may or may not be the // main frame. @@ -885,7 +890,7 @@ NavigationType NavigationControllerImpl::ClassifyNavigation( // Now we know that the notification is for an existing page. Find that entry. int existing_entry_index = GetEntryIndexWithPageID( - delegate_->GetSiteInstance(), + rvh->GetSiteInstance(), params.page_id); if (existing_entry_index == -1) { // The page was not found. It could have been pruned because of the limit on @@ -919,14 +924,13 @@ NavigationType NavigationControllerImpl::ClassifyNavigation( temp.append(base::IntToString(entries_[i]->site_instance()->GetId())); else temp.append("N"); - if (entries_[i]->site_instance() != delegate_->GetSiteInstance()) + if (entries_[i]->site_instance() != rvh->GetSiteInstance()) temp.append("x"); temp.append(","); } GURL url(temp); - static_cast<RenderViewHostImpl*>( - delegate_->GetRenderViewHost())->Send( - new ViewMsg_TempCrashWithData(url)); + static_cast<RenderViewHostImpl*>(rvh)->Send( + new ViewMsg_TempCrashWithData(url)); return NAVIGATION_TYPE_NAV_IGNORE; } NavigationEntryImpl* existing_entry = entries_[existing_entry_index].get(); @@ -972,7 +976,9 @@ NavigationType NavigationControllerImpl::ClassifyNavigation( } void NavigationControllerImpl::RendererDidNavigateToNewPage( - const ViewHostMsg_FrameNavigate_Params& params, bool replace_entry) { + RenderViewHost* rvh, + const ViewHostMsg_FrameNavigate_Params& params, + bool replace_entry) { NavigationEntryImpl* new_entry; bool update_virtual_url; // Only make a copy of the pending entry if it is appropriate for the new page @@ -980,7 +986,7 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage( // the SiteInstance hasn't been assigned to something else. if (pending_entry_ && (!pending_entry_->site_instance() || - pending_entry_->site_instance() == delegate_->GetSiteInstance())) { + pending_entry_->site_instance() == rvh->GetSiteInstance())) { new_entry = new NavigationEntryImpl(*pending_entry_); // Don't use the page type from the pending entry. Some interstitial page @@ -1014,7 +1020,7 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage( new_entry->SetPageID(params.page_id); new_entry->SetTransitionType(params.transition); new_entry->set_site_instance( - static_cast<SiteInstanceImpl*>(delegate_->GetSiteInstance())); + static_cast<SiteInstanceImpl*>(rvh->GetSiteInstance())); new_entry->SetHasPostData(params.is_post); new_entry->SetPostID(params.post_id); new_entry->SetOriginalRequestURL(params.original_request_url); @@ -1034,6 +1040,7 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage( } void NavigationControllerImpl::RendererDidNavigateToExistingPage( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params) { // We should only get here for main frame navigations. DCHECK(PageTransitionIsMainFrame(params.transition)); @@ -1041,7 +1048,7 @@ void NavigationControllerImpl::RendererDidNavigateToExistingPage( // This is a back/forward navigation. The existing page for the ID is // guaranteed to exist by ClassifyNavigation, and we just need to update it // with new information from the renderer. - int entry_index = GetEntryIndexWithPageID(delegate_->GetSiteInstance(), + int entry_index = GetEntryIndexWithPageID(rvh->GetSiteInstance(), params.page_id); DCHECK(entry_index >= 0 && entry_index < static_cast<int>(entries_.size())); @@ -1060,9 +1067,9 @@ void NavigationControllerImpl::RendererDidNavigateToExistingPage( // The site instance will normally be the same except during session restore, // when no site instance will be assigned. DCHECK(entry->site_instance() == NULL || - entry->site_instance() == delegate_->GetSiteInstance()); + entry->site_instance() == rvh->GetSiteInstance()); entry->set_site_instance( - static_cast<SiteInstanceImpl*>(delegate_->GetSiteInstance())); + static_cast<SiteInstanceImpl*>(rvh->GetSiteInstance())); entry->SetHasPostData(params.is_post); entry->SetPostID(params.post_id); @@ -1081,16 +1088,17 @@ void NavigationControllerImpl::RendererDidNavigateToExistingPage( // If a transient entry was removed, the indices might have changed, so we // have to query the entry index again. last_committed_entry_index_ = - GetEntryIndexWithPageID(delegate_->GetSiteInstance(), params.page_id); + GetEntryIndexWithPageID(rvh->GetSiteInstance(), params.page_id); } void NavigationControllerImpl::RendererDidNavigateToSamePage( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params) { // This mode implies we have a pending entry that's the same as an existing // entry for this page ID. This entry is guaranteed to exist by // ClassifyNavigation. All we need to do is update the existing entry. NavigationEntryImpl* existing_entry = GetEntryWithPageID( - delegate_->GetSiteInstance(), params.page_id); + rvh->GetSiteInstance(), params.page_id); // We assign the entry's unique ID to be that of the new one. Since this is // always the result of a user action, we want to dismiss infobars, etc. like @@ -1110,12 +1118,14 @@ void NavigationControllerImpl::RendererDidNavigateToSamePage( } void NavigationControllerImpl::RendererDidNavigateInPage( - const ViewHostMsg_FrameNavigate_Params& params, bool* did_replace_entry) { + RenderViewHost* rvh, + const ViewHostMsg_FrameNavigate_Params& params, + bool* did_replace_entry) { DCHECK(PageTransitionIsMainFrame(params.transition)) << "WebKit should only tell us about in-page navs for the main frame."; // We're guaranteed to have an entry for this one. NavigationEntryImpl* existing_entry = GetEntryWithPageID( - delegate_->GetSiteInstance(), params.page_id); + rvh->GetSiteInstance(), params.page_id); // Reference fragment navigation. We're guaranteed to have the last_committed // entry and it will be the same page as the new navigation (minus the @@ -1133,10 +1143,11 @@ void NavigationControllerImpl::RendererDidNavigateInPage( // If a transient entry was removed, the indices might have changed, so we // have to query the entry index again. last_committed_entry_index_ = - GetEntryIndexWithPageID(delegate_->GetSiteInstance(), params.page_id); + GetEntryIndexWithPageID(rvh->GetSiteInstance(), params.page_id); } void NavigationControllerImpl::RendererDidNavigateNewSubframe( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params) { if (PageTransitionCoreTypeIs(params.transition, PAGE_TRANSITION_AUTO_SUBFRAME)) { @@ -1158,6 +1169,7 @@ void NavigationControllerImpl::RendererDidNavigateNewSubframe( } bool NavigationControllerImpl::RendererDidNavigateAutoSubframe( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params) { // We're guaranteed to have a previously committed entry, and we now need to // handle navigation inside of a subframe in it without creating a new entry. @@ -1167,7 +1179,7 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe( // navigation entry. This is case "2." in NAV_AUTO_SUBFRAME comment in the // header file. In case "1." this will be a NOP. int entry_index = GetEntryIndexWithPageID( - delegate_->GetSiteInstance(), + rvh->GetSiteInstance(), params.page_id); if (entry_index < 0 || entry_index >= static_cast<int>(entries_.size())) { diff --git a/content/browser/frame_host/navigation_controller_impl.h b/content/browser/frame_host/navigation_controller_impl.h index 99fc36a..e9206b4 100644 --- a/content/browser/frame_host/navigation_controller_impl.h +++ b/content/browser/frame_host/navigation_controller_impl.h @@ -134,7 +134,10 @@ class CONTENT_EXPORT NavigationControllerImpl // // In the case that nothing has changed, the details structure is undefined // and it will return false. - bool RendererDidNavigate(const ViewHostMsg_FrameNavigate_Params& params, + // + // TODO(creis): Change RenderViewHost to RenderFrameHost. + bool RendererDidNavigate(RenderViewHost* rvh, + const ViewHostMsg_FrameNavigate_Params& params, LoadCommittedDetails* details); // Notifies us that we just became active. This is used by the WebContentsImpl @@ -231,6 +234,7 @@ class CONTENT_EXPORT NavigationControllerImpl // Classifies the given renderer navigation (see the NavigationType enum). NavigationType ClassifyNavigation( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params) const; // Causes the controller to load the specified entry. The function assumes @@ -250,17 +254,27 @@ class CONTENT_EXPORT NavigationControllerImpl // The functions taking |did_replace_entry| will fill into the given variable // whether the last entry has been replaced or not. // See LoadCommittedDetails.did_replace_entry. + // + // TODO(creis): Change RenderViewHost to RenderFrameHost. void RendererDidNavigateToNewPage( - const ViewHostMsg_FrameNavigate_Params& params, bool replace_entry); + RenderViewHost* rvh, + const ViewHostMsg_FrameNavigate_Params& params, + bool replace_entry); void RendererDidNavigateToExistingPage( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params); void RendererDidNavigateToSamePage( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params); void RendererDidNavigateInPage( - const ViewHostMsg_FrameNavigate_Params& params, bool* did_replace_entry); + RenderViewHost* rvh, + const ViewHostMsg_FrameNavigate_Params& params, + bool* did_replace_entry); void RendererDidNavigateNewSubframe( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params); bool RendererDidNavigateAutoSubframe( + RenderViewHost* rvh, const ViewHostMsg_FrameNavigate_Params& params); // Helper function for code shared between Reload() and ReloadIgnoringCache(). diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc index e13573e..aaecd1e 100644 --- a/content/browser/frame_host/navigation_controller_impl_unittest.cc +++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc @@ -1673,7 +1673,7 @@ TEST_F(NavigationControllerTest, Redirect) { LoadCommittedDetails details; EXPECT_EQ(0U, notifications.size()); - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; @@ -1730,7 +1730,7 @@ TEST_F(NavigationControllerTest, PostThenRedirect) { LoadCommittedDetails details; EXPECT_EQ(0U, notifications.size()); - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; @@ -1777,7 +1777,7 @@ TEST_F(NavigationControllerTest, ImmediateRedirect) { LoadCommittedDetails details; EXPECT_EQ(0U, notifications.size()); - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; @@ -1816,7 +1816,7 @@ TEST_F(NavigationControllerTest, NewSubframe) { params.page_state = PageState::CreateFromURL(url2); LoadCommittedDetails details; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_EQ(url1, details.previous_url); @@ -1852,7 +1852,7 @@ TEST_F(NavigationControllerTest, SubframeOnEmptyPage) { params.page_state = PageState::CreateFromURL(url); LoadCommittedDetails details; - EXPECT_FALSE(controller.RendererDidNavigate(params, &details)); + EXPECT_FALSE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(0U, notifications.size()); } @@ -1880,7 +1880,7 @@ TEST_F(NavigationControllerTest, AutoSubframe) { // Navigating should do nothing. LoadCommittedDetails details; - EXPECT_FALSE(controller.RendererDidNavigate(params, &details)); + EXPECT_FALSE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(0U, notifications.size()); // There should still be only one entry. @@ -1912,7 +1912,7 @@ TEST_F(NavigationControllerTest, BackSubframe) { // This should generate a new entry. LoadCommittedDetails details; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_EQ(2, controller.GetEntryCount()); @@ -1921,7 +1921,7 @@ TEST_F(NavigationControllerTest, BackSubframe) { const GURL url3("http://foo3"); params.page_id = 2; params.url = url3; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_EQ(3, controller.GetEntryCount()); @@ -1931,7 +1931,7 @@ TEST_F(NavigationControllerTest, BackSubframe) { controller.GoBack(); params.url = url2; params.page_id = 1; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_EQ(3, controller.GetEntryCount()); @@ -1943,7 +1943,7 @@ TEST_F(NavigationControllerTest, BackSubframe) { controller.GoBack(); params.url = url1; params.page_id = 0; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_EQ(3, controller.GetEntryCount()); @@ -2002,7 +2002,8 @@ TEST_F(NavigationControllerTest, InPage) { self_params.was_within_same_page = true; LoadCommittedDetails details; - EXPECT_TRUE(controller.RendererDidNavigate(self_params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), self_params, + &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_TRUE(details.is_in_page); @@ -2022,7 +2023,7 @@ TEST_F(NavigationControllerTest, InPage) { params.was_within_same_page = true; // This should generate a new entry. - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_TRUE(details.is_in_page); @@ -2034,7 +2035,8 @@ TEST_F(NavigationControllerTest, InPage) { controller.GoBack(); back_params.url = url1; back_params.page_id = 0; - EXPECT_TRUE(controller.RendererDidNavigate(back_params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), back_params, + &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_TRUE(details.is_in_page); @@ -2047,7 +2049,8 @@ TEST_F(NavigationControllerTest, InPage) { controller.GoForward(); forward_params.url = url2; forward_params.page_id = 1; - EXPECT_TRUE(controller.RendererDidNavigate(forward_params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), forward_params, + &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_TRUE(details.is_in_page); @@ -2061,9 +2064,11 @@ TEST_F(NavigationControllerTest, InPage) { // one identified by an existing page ID. This would result in the second URL // losing the reference fragment when you navigate away from it and then back. controller.GoBack(); - EXPECT_TRUE(controller.RendererDidNavigate(back_params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), back_params, + &details)); controller.GoForward(); - EXPECT_TRUE(controller.RendererDidNavigate(forward_params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), forward_params, + &details)); EXPECT_EQ(forward_params.url, controller.GetVisibleEntry()->GetURL()); @@ -2072,7 +2077,7 @@ TEST_F(NavigationControllerTest, InPage) { params.page_id = 2; params.url = url3; navigation_entry_committed_counter_ = 0; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_FALSE(details.is_in_page); @@ -2104,7 +2109,7 @@ TEST_F(NavigationControllerTest, InPage_Replace) { // This should NOT generate a new entry, nor prune the list. LoadCommittedDetails details; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_TRUE(details.is_in_page); @@ -2154,7 +2159,7 @@ TEST_F(NavigationControllerTest, ClientRedirectAfterInPageNavigation) { // This should NOT generate a new entry, nor prune the list. LoadCommittedDetails details; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_TRUE(details.is_in_page); @@ -2178,7 +2183,7 @@ TEST_F(NavigationControllerTest, ClientRedirectAfterInPageNavigation) { // This SHOULD generate a new entry. LoadCommittedDetails details; - EXPECT_TRUE(controller.RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller.RendererDidNavigate(test_rvh(), params, &details)); EXPECT_EQ(1U, navigation_entry_committed_counter_); navigation_entry_committed_counter_ = 0; EXPECT_FALSE(details.is_in_page); @@ -2341,7 +2346,8 @@ TEST_F(NavigationControllerTest, RestoreNavigate) { params.is_post = false; params.page_state = PageState::CreateFromURL(url); LoadCommittedDetails details; - our_controller.RendererDidNavigate(params, &details); + our_controller.RendererDidNavigate(our_contents->GetRenderViewHost(), params, + &details); // There should be no longer any pending entry and one committed one. This // means that we were able to locate the entry, assign its site instance, and @@ -2427,7 +2433,7 @@ TEST_F(NavigationControllerTest, RestoreNavigateAfterFailure) { params.is_post = false; params.page_state = PageState::CreateFromURL(url); LoadCommittedDetails details; - our_controller.RendererDidNavigate(params, &details); + our_controller.RendererDidNavigate(rvh, params, &details); // There should be no pending entry and one committed one. EXPECT_EQ(1, our_controller.GetEntryCount()); @@ -2952,7 +2958,7 @@ TEST_F(NavigationControllerTest, SameSubframe) { params.is_post = false; params.page_state = PageState::CreateFromURL(subframe); LoadCommittedDetails details; - EXPECT_FALSE(controller.RendererDidNavigate(params, &details)); + EXPECT_FALSE(controller.RendererDidNavigate(test_rvh(), params, &details)); // Nothing should have changed. EXPECT_EQ(controller.GetEntryCount(), 1); @@ -3069,7 +3075,7 @@ TEST_F(NavigationControllerTest, SubframeWhilePending) { LoadCommittedDetails details; // This should return false meaning that nothing was actually updated. - EXPECT_FALSE(controller.RendererDidNavigate(params, &details)); + EXPECT_FALSE(controller.RendererDidNavigate(test_rvh(), params, &details)); // The notification should have updated the last committed one, and not // the pending load. diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc index 6cc9dad..f1ee1c9 100644 --- a/content/browser/frame_host/navigator_impl.cc +++ b/content/browser/frame_host/navigator_impl.cc @@ -4,6 +4,7 @@ #include "content/browser/frame_host/navigator_impl.h" +#include "base/command_line.h" #include "content/browser/frame_host/frame_tree_node.h" #include "content/browser/frame_host/navigation_controller_impl.h" #include "content/browser/frame_host/navigation_entry_impl.h" @@ -15,6 +16,7 @@ #include "content/public/browser/invalidate_type.h" #include "content/public/browser/navigation_controller.h" #include "content/public/browser/render_view_host.h" +#include "content/public/common/content_switches.h" #include "content/public/common/url_constants.h" namespace content { @@ -38,15 +40,22 @@ void NavigatorImpl::DidStartProvisionalLoad( RenderProcessHost* render_process_host = render_frame_host->GetProcess(); RenderViewHost::FilterURL(render_process_host, false, &validated_url); + // TODO(creis): This is a hack for now, until we mirror the frame tree and do + // cross-process subframe navigations in actual subframes. As a result, we + // can currently only support a single cross-process subframe per RVH. + NavigationEntryImpl* pending_entry = + NavigationEntryImpl::FromNavigationEntry(controller_->GetPendingEntry()); + if (pending_entry && + pending_entry->frame_tree_node_id() != -1 && + CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) + is_main_frame = false; + if (is_main_frame) { // If there is no browser-initiated pending entry for this navigation and it // is not for the error URL, create a pending entry using the current // SiteInstance, and ensure the address bar updates accordingly. We don't // know the referrer or extra headers at this point, but the referrer will // be set properly upon commit. - NavigationEntryImpl* pending_entry = - NavigationEntryImpl::FromNavigationEntry( - controller_->GetPendingEntry()); bool has_browser_initiated_pending_entry = pending_entry && !pending_entry->is_renderer_initiated(); if (!has_browser_initiated_pending_entry && !is_error_page) { diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc index c64aee3..81bbe51 100644 --- a/content/browser/frame_host/render_frame_host_manager.cc +++ b/content/browser/frame_host/render_frame_host_manager.cc @@ -191,9 +191,10 @@ RenderViewHostImpl* RenderFrameHostManager::Navigate( if (dest_render_frame_host != render_frame_host_ && dest_render_frame_host->render_view_host()->GetView()) { dest_render_frame_host->render_view_host()->GetView()->Hide(); - } else { + } else if (frame_tree_node_->IsMainFrame()) { // This is our primary renderer, notify here as we won't be calling - // CommitPending (which does the notify). + // CommitPending (which does the notify). We only do this for top-level + // frames. delegate_->NotifySwappedFromRenderManager( NULL, render_frame_host_->render_view_host()); } @@ -482,8 +483,12 @@ void RenderFrameHostManager::SwapOutOldPage() { // message to the ResourceDispatcherHost, allowing the pending RVH's response // to resume. // TODO(creis): We should do this on the RFH or else we'll swap out the - // top-level page when subframes navigate. - render_frame_host_->render_view_host()->SwapOut(); + // top-level page when subframes navigate. Until then, we skip swapping out + // for subframes. + if (frame_tree_node_->IsMainFrame()) + render_frame_host_->render_view_host()->SwapOut(); + else + SwappedOut(render_frame_host_->render_view_host()); // ResourceDispatcherHost has told us to run the onunload handler, which // means it is not a download or unsafe page, and we are going to perform the @@ -777,6 +782,16 @@ RenderFrameHostImpl* RenderFrameHostManager::CreateRenderFrameHost( site_instance, view_routing_id, frame_routing_id, swapped_out, hidden); } else { render_view_host = frame_tree->GetRenderViewHostForSubFrame(site_instance); + + // If we haven't found a RVH for a subframe RFH, it's because we currently + // do not create top-level RFHs for pending subframe navigations. Create + // the RVH here for now. + // TODO(creis): Mirror the frame tree so this check isn't necessary. + if (!render_view_host) { + render_view_host = frame_tree->CreateRenderViewHostForMainFrame( + site_instance, view_routing_id, frame_routing_id, swapped_out, + hidden); + } } // TODO(creis): Make render_frame_host a scoped_ptr. @@ -833,7 +848,7 @@ int RenderFrameHostManager::CreateRenderFrame( if (success && frame_tree_node_->IsMainFrame()) { // Don't show the main frame's view until we get a DidNavigate from it. render_view_host->GetView()->Hide(); - } else if (!swapped_out) { + } else if (!swapped_out && pending_render_frame_host_) { CancelPending(); } } @@ -909,20 +924,21 @@ void RenderFrameHostManager::CommitPending() { render_frame_host_->render_view_host()->GetView() && render_frame_host_->render_view_host()->GetView()->HasFocus(); + // TODO(creis): As long as show/hide are on RVH, we don't want to do them for + // subframe navigations or they'll interfere with the top-level page. + bool is_main_frame = frame_tree_node_->IsMainFrame(); + // Swap in the pending frame and make it active. Also ensure the FrameTree // stays in sync. RenderFrameHostImpl* old_render_frame_host = render_frame_host_; render_frame_host_ = pending_render_frame_host_; pending_render_frame_host_ = NULL; - render_frame_host_->render_view_host()->AttachToFrameTree(); + if (is_main_frame) + render_frame_host_->render_view_host()->AttachToFrameTree(); // The process will no longer try to exit, so we can decrement the count. render_frame_host_->GetProcess()->RemovePendingView(); - // TODO(creis): As long as show/hide are on RVH, we don't want to do them for - // subframe navigations or they'll interfere with the top-level page. - bool is_main_frame = frame_tree_node_->IsMainFrame(); - // If the view is gone, then this RenderViewHost died while it was hidden. // We ignored the RenderProcessGone call at the time, so we should send it now // to make sure the sad tab shows up, etc. @@ -933,10 +949,16 @@ void RenderFrameHostManager::CommitPending() { render_frame_host_->render_view_host()->GetView()->Show(); } - // Hide the old view now that the new one is visible. + // If the old view is live and top-level, hide it now that the new one is + // visible. if (old_render_frame_host->render_view_host()->GetView()) { - old_render_frame_host->render_view_host()->GetView()->Hide(); - old_render_frame_host->render_view_host()->WasSwappedOut(); + if (is_main_frame) { + old_render_frame_host->render_view_host()->GetView()->Hide(); + old_render_frame_host->render_view_host()->WasSwappedOut(); + } else { + // TODO(creis): Swap out the subframe. We'll need to swap it back in when + // navigating back. + } } // Make sure the size is up to date. (Fix for bug 1079768.) @@ -967,7 +989,9 @@ void RenderFrameHostManager::CommitPending() { 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. - DCHECK(old_render_frame_host->render_view_host()->is_swapped_out()); + // TODO(creis): Swap out the subframe in --site-per-process. + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) + DCHECK(old_render_frame_host->render_view_host()->is_swapped_out()); // 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. diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index a393053..fcdf375 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc @@ -702,13 +702,19 @@ void RenderViewHostImpl::OnCrossSiteResponse( PageTransition page_transition, int64 frame_id, bool should_replace_current_entry) { - RenderViewHostDelegate::RendererManagement* manager = - delegate_->GetRendererManagementDelegate(); - if (manager) { - manager->OnCrossSiteResponse(this, global_request_id, is_transfer, - transfer_url_chain, referrer, page_transition, - frame_id, should_replace_current_entry); + FrameTreeNode* node = NULL; + if (frame_id != -1 && + CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) { + node = delegate_->GetFrameTree()->FindByFrameID(frame_id); } + + // TODO(creis): We should always be able to get the RFHM for a frame_id, + // but today the frame_id is -1 for the main frame. + RenderViewHostDelegate::RendererManagement* manager = node ? + node->render_manager() : delegate_->GetRendererManagementDelegate(); + manager->OnCrossSiteResponse(this, global_request_id, is_transfer, + transfer_url_chain, referrer, page_transition, + frame_id, should_replace_current_entry); } void RenderViewHostImpl::SuppressDialogsUntilSwapOut() { diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc index cca1df0..781b97b 100644 --- a/content/browser/site_per_process_browsertest.cc +++ b/content/browser/site_per_process_browsertest.cc @@ -31,9 +31,20 @@ class SitePerProcessWebContentsObserver: public WebContentsObserver { public: explicit SitePerProcessWebContentsObserver(WebContents* web_contents) : WebContentsObserver(web_contents), - navigation_succeeded_(true) {} + navigation_succeeded_(false) {} virtual ~SitePerProcessWebContentsObserver() {} + virtual void DidStartProvisionalLoadForFrame( + int64 frame_id, + int64 parent_frame_id, + bool is_main_frame, + const GURL& validated_url, + bool is_error_page, + bool is_iframe_srcdoc, + RenderViewHost* render_view_host) OVERRIDE { + navigation_succeeded_ = false; + } + virtual void DidFailProvisionalLoad( int64 frame_id, const base::string16& frame_unique_name, @@ -184,9 +195,8 @@ class SitePerProcessBrowserTest : public ContentBrowserTest { } }; -// TODO(nasko): Disable this test until out-of-process iframes is ready and the -// security checks are back in place. -IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, DISABLED_CrossSiteIframe) { +// Ensure that we can complete a cross-process subframe navigation. +IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CrossSiteIframe) { ASSERT_TRUE(test_server()->Start()); net::SpawnedTestServer https_server( net::SpawnedTestServer::TYPE_HTTPS, @@ -198,21 +208,31 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, DISABLED_CrossSiteIframe) { NavigateToURL(shell(), main_url); SitePerProcessWebContentsObserver observer(shell()->web_contents()); - { - // Load same-site page into Iframe. - GURL http_url(test_server()->GetURL("files/title1.html")); - EXPECT_TRUE(NavigateIframeToURL(shell(), http_url, "test")); - EXPECT_EQ(observer.navigation_url(), http_url); - EXPECT_TRUE(observer.navigation_succeeded()); - } - { - // Load cross-site page into Iframe. - GURL https_url(https_server.GetURL("files/title1.html")); - EXPECT_TRUE(NavigateIframeToURL(shell(), https_url, "test")); - EXPECT_EQ(observer.navigation_url(), https_url); - EXPECT_FALSE(observer.navigation_succeeded()); - } + // Load same-site page into iframe. + GURL http_url(test_server()->GetURL("files/title1.html")); + EXPECT_TRUE(NavigateIframeToURL(shell(), http_url, "test")); + EXPECT_EQ(observer.navigation_url(), http_url); + EXPECT_TRUE(observer.navigation_succeeded()); + + // Load cross-site page into iframe. + GURL https_url(https_server.GetURL("files/title1.html")); + EXPECT_TRUE(NavigateIframeToURL(shell(), https_url, "test")); + EXPECT_EQ(observer.navigation_url(), https_url); + EXPECT_TRUE(observer.navigation_succeeded()); + + // Ensure that we have created a new process for the subframe. + FrameTreeNode* root = + static_cast<WebContentsImpl*>(shell()->web_contents())-> + GetFrameTree()->root(); + ASSERT_EQ(1U, root->child_count()); + FrameTreeNode* child = root->child_at(0); + EXPECT_NE(shell()->web_contents()->GetRenderViewHost(), + child->current_frame_host()->render_view_host()); + EXPECT_NE(shell()->web_contents()->GetSiteInstance(), + child->current_frame_host()->render_view_host()->GetSiteInstance()); + EXPECT_NE(shell()->web_contents()->GetRenderProcessHost(), + child->current_frame_host()->GetProcess()); } // TODO(nasko): Disable this test until out-of-process iframes is ready and the diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index d42ccb7..aa5193d 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc @@ -1738,10 +1738,17 @@ bool WebContentsImpl::NavigateToEntry( return false; } - // TODO(creis): Use entry->frame_tree_node_id() to pick which - // RenderFrameHostManager to use. + // Use entry->frame_tree_node_id() to pick which RenderFrameHostManager to + // use when --site-per-process is used. + RenderFrameHostManager* manager = GetRenderManager(); + if (entry.frame_tree_node_id() != -1 && + CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) { + int64 frame_tree_node_id = entry.frame_tree_node_id(); + manager = frame_tree_.FindByID(frame_tree_node_id)->render_manager(); + } + RenderViewHostImpl* dest_render_view_host = - static_cast<RenderViewHostImpl*>(GetRenderManager()->Navigate(entry)); + static_cast<RenderViewHostImpl*>(manager->Navigate(entry)); if (!dest_render_view_host) return false; // Unable to create the desired render view host. @@ -1768,6 +1775,9 @@ bool WebContentsImpl::NavigateToEntry( current_load_start_ = base::TimeTicks::Now(); // Navigate in the desired RenderViewHost. + // TODO(creis): As a temporary hack, we currently do cross-process subframe + // navigations in a top-level frame of the new process. Thus, we don't yet + // need to store the correct frame ID in ViewMsg_Navigate_Params. ViewMsg_Navigate_Params navigate_params; MakeNavigateParams(entry, controller_, delegate_, reload_type, &navigate_params); @@ -2323,6 +2333,14 @@ void WebContentsImpl::OnDidFinishLoad( const GURL& url, bool is_main_frame) { GURL validated_url(url); + + // --site-per-process mode has a short-term hack allowing cross-process + // subframe pages to commit thinking they are top-level. Correct it here to + // avoid confusing the observers. + if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess) && + render_view_message_source_ != GetRenderViewHost()) + is_main_frame = false; + RenderProcessHost* render_process_host = render_view_message_source_->GetProcess(); RenderViewHost::FilterURL(render_process_host, false, &validated_url); @@ -2968,13 +2986,48 @@ void WebContentsImpl::RenderViewDeleted(RenderViewHost* rvh) { void WebContentsImpl::DidNavigate( RenderViewHost* rvh, - const ViewHostMsg_FrameNavigate_Params& params) { + const ViewHostMsg_FrameNavigate_Params& orig_params) { + ViewHostMsg_FrameNavigate_Params params(orig_params); + bool use_site_per_process = + CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess); if (frame_tree_.IsFirstNavigationAfterSwap()) { // First navigation should be a main frame navigation. - DCHECK(PageTransitionIsMainFrame(params.transition)); + // TODO(creis): This DCHECK is currently disabled for --site-per-process + // because cross-process subframe navigations still have a main frame + // PageTransition. + if (!use_site_per_process) + DCHECK(PageTransitionIsMainFrame(params.transition)); frame_tree_.OnFirstNavigationAfterSwap(params.frame_id); } + // When using --site-per-process, look up the FrameTreeNode ID that the + // renderer-specific frame ID corresponds to. + int64 frame_tree_node_id = frame_tree_.root()->frame_tree_node_id(); + if (use_site_per_process) { + FrameTreeNode* source_node = frame_tree_.FindByFrameID(params.frame_id); + if (source_node) + frame_tree_node_id = source_node->frame_tree_node_id(); + + // TODO(creis): In the short term, cross-process subframe navigations are + // happening in the pending RenderViewHost's top-level frame. (We need to + // both mirror the frame tree and get the navigation to occur in the correct + // subframe to fix this.) Until then, we should check whether we have a + // pending NavigationEntry with a frame ID and if so, treat the + // cross-process "main frame" navigation as a subframe navigation. This + // limits us to a single cross-process subframe per RVH, and it affects + // NavigateToEntry, NavigatorImpl::DidStartProvisionalLoad, and + // OnDidFinishLoad. + NavigationEntryImpl* pending_entry = + NavigationEntryImpl::FromNavigationEntry(controller_.GetPendingEntry()); + int root_ftn_id = frame_tree_.root()->frame_tree_node_id(); + if (pending_entry && + pending_entry->frame_tree_node_id() != -1 && + pending_entry->frame_tree_node_id() != root_ftn_id) { + params.transition = PAGE_TRANSITION_AUTO_SUBFRAME; + frame_tree_node_id = pending_entry->frame_tree_node_id(); + } + } + if (PageTransitionIsMainFrame(params.transition)) { // When overscroll navigation gesture is enabled, a screenshot of the page // in its current state is taken so that it can be used during the @@ -2985,7 +3038,16 @@ void WebContentsImpl::DidNavigate( if (delegate_ && delegate_->CanOverscrollContent()) controller_.TakeScreenshot(); - GetRenderManager()->DidNavigateMainFrame(rvh); + if (!use_site_per_process) + GetRenderManager()->DidNavigateMainFrame(rvh); + } + + // When using --site-per-process, we notify the RFHM for all navigations, + // not just main frame navigations. + if (use_site_per_process) { + FrameTreeNode* frame = frame_tree_.FindByID(frame_tree_node_id); + // TODO(creis): Rename to DidNavigateFrame. + frame->render_manager()->DidNavigateMainFrame(rvh); } // Update the site of the SiteInstance if it doesn't have one yet, unless @@ -3009,7 +3071,7 @@ void WebContentsImpl::DidNavigate( contents_mime_type_ = params.contents_mime_type; LoadCommittedDetails details; - bool did_navigate = controller_.RendererDidNavigate(params, &details); + bool did_navigate = controller_.RendererDidNavigate(rvh, params, &details); // For now, keep track of each frame's URL in its FrameTreeNode. This lets // us estimate our process count for implementing OOP iframes. @@ -3284,8 +3346,14 @@ void WebContentsImpl::RequestTransferURL( GetSiteInstance(), url)) dest_url = GURL(kAboutBlankURL); - // TODO(creis): Look up the FrameTreeNode ID corresponding to source_frame_id. - int frame_tree_node_id = -1; + // Look up the FrameTreeNode ID corresponding to source_frame_id. + int64 frame_tree_node_id = -1; + if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess) && + source_frame_id != -1) { + FrameTreeNode* source_node = frame_tree_.FindByFrameID(source_frame_id); + if (source_node) + frame_tree_node_id = source_node->frame_tree_node_id(); + } OpenURLParams params(dest_url, referrer, source_frame_id, frame_tree_node_id, disposition, page_transition, true /* is_renderer_initiated */); diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc index a44ff44..8ac7161 100644 --- a/content/browser/web_contents/web_contents_impl_unittest.cc +++ b/content/browser/web_contents/web_contents_impl_unittest.cc @@ -302,7 +302,7 @@ TEST_F(WebContentsImplTest, UpdateTitle) { InitNavigateParams(¶ms, 0, GURL(kAboutBlankURL), PAGE_TRANSITION_TYPED); LoadCommittedDetails details; - cont.RendererDidNavigate(params, &details); + cont.RendererDidNavigate(test_rvh(), params, &details); contents()->UpdateTitle(rvh(), 0, base::ASCIIToUTF16(" Lots O' Whitespace\n"), @@ -349,7 +349,7 @@ TEST_F(WebContentsImplTest, NTPViewSource) { ViewHostMsg_FrameNavigate_Params params; InitNavigateParams(¶ms, 0, kGURL, PAGE_TRANSITION_TYPED); LoadCommittedDetails details; - cont.RendererDidNavigate(params, &details); + cont.RendererDidNavigate(test_rvh(), params, &details); // Also check title and url. EXPECT_EQ(base::ASCIIToUTF16(kUrl), contents()->GetTitle()); } |