diff --git a/content/browser/renderer_host/frame_tree_node.cc b/content/browser/renderer_host/frame_tree_node.cc index b05b6cc20ab4a3..47b0f6c46dfc30 100644 --- a/content/browser/renderer_host/frame_tree_node.cc +++ b/content/browser/renderer_host/frame_tree_node.cc @@ -320,14 +320,15 @@ void FrameTreeNode::SetOriginalOpener(FrameTreeNode* opener) { } void FrameTreeNode::SetCurrentURL(const GURL& url) { - if (!has_committed_real_load_ && !url.IsAboutBlank()) { - has_committed_real_load_ = true; - is_on_initial_empty_document_or_subsequent_empty_documents_ = false; - } current_frame_host()->SetLastCommittedUrl(url); blame_context_.TakeSnapshot(); } +void FrameTreeNode::DidCommitNonInitialEmptyDocument() { + has_committed_real_load_ = true; + is_on_initial_empty_document_ = false; +} + void FrameTreeNode::SetCurrentOrigin( const url::Origin& origin, bool is_potentially_trustworthy_unique_origin) { diff --git a/content/browser/renderer_host/frame_tree_node.h b/content/browser/renderer_host/frame_tree_node.h index c24bd7a5b3accd..ad097385047b40 100644 --- a/content/browser/renderer_host/frame_tree_node.h +++ b/content/browser/renderer_host/frame_tree_node.h @@ -166,30 +166,35 @@ class CONTENT_EXPORT FrameTreeNode { return current_frame_host()->GetLastCommittedURL(); } - // Sets the last committed URL for this frame and updates - // has_committed_real_load accordingly. + // Sets the last committed URL for this frame. void SetCurrentURL(const GURL& url); - // Returns true if SetCurrentURL has been called with a non-blank URL. + // The frame committed a document that is not the initial empty document. + // Update `has_committed_real_load_` and `is_on_initial_empty_document_` + // accordingly. + void DidCommitNonInitialEmptyDocument(); + + // Returns true if the frame has committed a document that is not the initial + // empty document. For more details, see the definition of + // `has_committed_real_load_`. // TODO(https://crbug.com/1215096): Migrate most usage of // has_committed_real_load() to call - // is_on_initial_empty_document_or_subsequent_empty_documents() instead. + // is_on_initial_empty_document() instead and remove this. bool has_committed_real_load() const { return has_committed_real_load_; } - // Returns true if SetCurrentURL has been called with a non-blank URL or - // if the current document's input stream has been opened with - // document.open(). For more details, see the definition of - // `is_on_initial_empty_document_or_subsequent_empty_documents_`. - bool is_on_initial_empty_document_or_subsequent_empty_documents() const { - return is_on_initial_empty_document_or_subsequent_empty_documents_; + // Returns true if the frame has committed a document that is not the initial + // empty document, or if the current document's input stream has been opened + // with document.open(), causing the document to lose its "initial empty + // document" status. For more details, see the definition of + // `is_on_initial_empty_document_`. + bool is_on_initial_empty_document() const { + return is_on_initial_empty_document_; } - // Sets `is_on_initial_empty_document_or_subsequent_empty_documents_` to + // Sets `is_on_initial_empty_document_` to // false. Must only be called after the current document's input stream has // been opened with document.open(). - void DidOpenDocumentInputStream() { - is_on_initial_empty_document_or_subsequent_empty_documents_ = false; - } + void DidOpenDocumentInputStream() { is_on_initial_empty_document_ = false; } // Returns whether the frame's owner element in the parent document is // collapsed, that is, removed from the layout as if it did not exist, as per @@ -593,28 +598,33 @@ class CONTENT_EXPORT FrameTreeNode { // Please refer to {Get,Set}PopupCreatorOrigin() documentation. url::Origin popup_creator_origin_; - // Returns true iff SetCurrentURL has been called with a non-blank URL. + // Whether this frame is still on the initial about:blank document or the + // synchronously committed about:blank document committed at frame creation, + // and its "initial empty document"-ness is still true. + // This will be false if either of these has happened: + // - SetCurrentUrl() was called after committing a document that is not the + // initial about:blank document or the synchronously committed about:blank + // document, per + // https://html.spec.whatwg.org/multipage/browsers.html#creating-browsing-contexts:is-initial-about:blank + // - The document's input stream has been opened with document.open(), per + // https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#opening-the-input-stream:is-initial-about:blank + // NOTE: we treat both the "initial about:blank document" and the + // "synchronously committed about:blank document" as the initial empty + // document. In the future, we plan to remove the synchronous about:blank + // commit so that this state will only be true if the frame is on the + // "initial about:blank document". See also: + // - https://github.com/whatwg/html/issues/6863 + // - https://crbug.com/1215096 + bool is_on_initial_empty_document_ = true; + + // Similar to `is_on_initial_empty_document_`, but is unaffected by + // document.open() opening the input stream of the document. In other words, + // `has_committed_real_load_` is false iff SetCurrentUrl() has been called + // for a non-initial-empty-document commit. // TODO(https://crbug.com/1215096): Migrate all current usage of this to - // use `is_on_initial_empty_document_or_subsequent_empty_documents_` instead. + // use `is_on_initial_empty_document_` instead and remove this. bool has_committed_real_load_ = false; - // Whether this frame is still on the initial about:blank document or any - // subsequent about:blank documents committed after the initial about:blank - // document. This will be false if either of these has happened: - // - SetCurrentUrl() has been called with a non about:blank URL. - // - The document's input stream has been opened with document.open(). - // See: - // https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#opening-the-input-stream:is-initial-about:blank - // TODO(https://crbug.com/1215096): Make this false after non-initial - // about:blank commits as well, making this only track whether the current - // document is the initial empty document or not. Currently we are still - // preserving most of the old behavior of `has_committed_real_load_` (except - // for the document.open() bit here) due to our current handling of initial - // empty document for session history and navigation (where we treat the - // the initial about:blank document and subsequent about:blank documents the - // same way). - bool is_on_initial_empty_document_or_subsequent_empty_documents_ = true; - // Whether the frame's owner element in the parent document is collapsed. bool is_collapsed_ = false; diff --git a/content/browser/renderer_host/isolated_app_throttle.cc b/content/browser/renderer_host/isolated_app_throttle.cc index c903e6ae33ccea..72ecb1b4c3a34c 100644 --- a/content/browser/renderer_host/isolated_app_throttle.cc +++ b/content/browser/renderer_host/isolated_app_throttle.cc @@ -95,8 +95,7 @@ IsolatedAppThrottle::WillStartRequest() { } FrameTreeNode* frame_tree_node = navigation_request->frame_tree_node(); - if (!frame_tree_node - ->is_on_initial_empty_document_or_subsequent_empty_documents()) { + if (!frame_tree_node->is_on_initial_empty_document()) { prev_origin_ = frame_tree_node->current_origin(); } diff --git a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc index c535d5a56ea814..5f710eec2ed5ec 100644 --- a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc +++ b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc @@ -3936,12 +3936,17 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, CreateSubframe(contents(), "child1", GURL(), false /* wait_for_navigation */); + // The new subframe should be on the initial empty document. + FrameTreeNode* new_subframe = root->child_at(subframe_index); + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_TRUE(new_subframe->is_on_initial_empty_document()); + // Do a navigation on the "child1" subframe to |url_2|. // The navigation is still classified as "auto", so we didn't append a new // NavigationEntry, and instead updated the current NavigationEntry. - NavigateSubframeAndCheckNavigationType( - contents(), root->child_at(subframe_index), "child1", url_2, - NAVIGATION_TYPE_AUTO_SUBFRAME); + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child1", + url_2, + NAVIGATION_TYPE_AUTO_SUBFRAME); EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); } @@ -3957,22 +3962,35 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, subframe_index++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); - // Do a navigation on the "child1" subframe to about:blank#foo, creating a + // The new subframe should be on the initial empty document. + FrameTreeNode* new_subframe = root->child_at(subframe_index); + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_TRUE(new_subframe->is_on_initial_empty_document()); + + // Do a navigation on the "child2" subframe to about:blank#foo, creating a // same-document navigation. The navigation will do a replacement and get // classified as "auto", so we won't append a new NavigationEntry, and // instead update the current NavigationEntry. - NavigateSubframeAndCheckNavigationType( - contents(), root->child_at(subframe_index), "child2", - GURL("about:blank#foo"), NAVIGATION_TYPE_AUTO_SUBFRAME); + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child2", + GURL("about:blank#foo"), + NAVIGATION_TYPE_AUTO_SUBFRAME); EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + // The subframe should still be on the initial empty document. + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_TRUE(new_subframe->is_on_initial_empty_document()); + // Do a navigation on the "child2" subframe to |url_2|. - NavigateSubframeAndCheckNavigationType( - contents(), root->child_at(subframe_index), "child2", url_2, - NAVIGATION_TYPE_AUTO_SUBFRAME); + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child2", + url_2, + NAVIGATION_TYPE_AUTO_SUBFRAME); // The navigation is still classified as "auto", so we didn't append a new // NavigationEntry, and instead updated the current NavigationEntry. EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + + // The subframe should no longer be on the initial empty document. + EXPECT_TRUE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); } // 3) Navigate to |url_2| on a new subframe that has done a navigation to a @@ -3987,12 +4005,16 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, subframe_index++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + // The new subframe should not be on the initial empty document. + FrameTreeNode* new_subframe = root->child_at(subframe_index); + EXPECT_TRUE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); + // Do a navigation on the "child3" subframe to |url_2|. // The navigation is classified as a new navigation, and appended a new // NavigationEntry. - NavigateSubframeAndCheckNavigationType( - contents(), root->child_at(subframe_index), "child3", url_2, - NAVIGATION_TYPE_NEW_SUBFRAME); + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child3", + url_2, NAVIGATION_TYPE_NEW_SUBFRAME); expected_entry_count++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); } @@ -4008,13 +4030,22 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, subframe_index++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + // The new subframe should be on the initial empty document. + FrameTreeNode* new_subframe = root->child_at(subframe_index); + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_TRUE(new_subframe->is_on_initial_empty_document()); + // Do a navigation on the "child4" subframe to |url_2|. // The navigation is still classified as "auto", so we didn't append a new // NavigationEntry, and instead updated the current NavigationEntry. - NavigateSubframeAndCheckNavigationType( - contents(), root->child_at(subframe_index), "child4", url_2, - NAVIGATION_TYPE_AUTO_SUBFRAME); + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child4", + url_2, + NAVIGATION_TYPE_AUTO_SUBFRAME); EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + + // The subframe should no longer be on the initial empty document. + EXPECT_TRUE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); } // 5) Navigate to |url_2| on a new subframe that has done a document.open(). @@ -4026,13 +4057,12 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, false /* wait_for_navigation */); subframe_index++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); - EXPECT_EQ(GURL("about:blank"), - root->child_at(subframe_index)->current_url()); - // The frame should be on its initial empty document. - EXPECT_FALSE(root->child_at(subframe_index)->has_committed_real_load()); - EXPECT_TRUE( - root->child_at(subframe_index) - ->is_on_initial_empty_document_or_subsequent_empty_documents()); + + // The new subframe should be on the initial empty document. + FrameTreeNode* new_subframe = root->child_at(subframe_index); + EXPECT_EQ(GURL("about:blank"), new_subframe->current_url()); + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_TRUE(new_subframe->is_on_initial_empty_document()); // Do a document.open() on it. EXPECT_TRUE(ExecJs(shell(), R"( @@ -4044,32 +4074,33 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, // Ensure the URL update from the document.open() above has finished before // continuing by waiting for a renderer round-trip to run this script. - EXPECT_TRUE(ExecJs(root->child_at(subframe_index), "true")); + EXPECT_TRUE(ExecJs(new_subframe, "true")); // The document.open() changed the iframe's URL in the renderer to be the // same as the main frame's URL, but doesn't actually commit a navigation. - EXPECT_EQ(GURL("about:blank"), root->child_at(subframe_index) - ->current_frame_host() - ->GetLastCommittedURL()); - EXPECT_EQ(url_1, root->child_at(subframe_index) - ->current_frame_host() - ->last_document_url_in_renderer()); + EXPECT_EQ(GURL("about:blank"), + new_subframe->current_frame_host()->GetLastCommittedURL()); + EXPECT_EQ( + url_1, + new_subframe->current_frame_host()->last_document_url_in_renderer()); // The frame lost its "initial empty document" status, but // `has_committed_real_load` is still false because the last committed URL // stays the same. - EXPECT_FALSE(root->child_at(subframe_index)->has_committed_real_load()); - EXPECT_FALSE( - root->child_at(subframe_index) - ->is_on_initial_empty_document_or_subsequent_empty_documents()); + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); // Do a navigation on the "child5" subframe to |url_2|. // The navigation is classified as a new navigation, and appended a new // NavigationEntry. - NavigateSubframeAndCheckNavigationType( - contents(), root->child_at(subframe_index), "child5", url_2, - NAVIGATION_TYPE_NEW_SUBFRAME); + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child5", + url_2, NAVIGATION_TYPE_NEW_SUBFRAME); expected_entry_count++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + + // The frame's '`has_committed_real_load` is now true now that it has + // committed another document. + EXPECT_TRUE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); } // 6) Navigate to |url_2| on a new subframe that has done a navigation to @@ -4083,13 +4114,67 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, subframe_index++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + // The new subframe should be on the initial empty document. + FrameTreeNode* new_subframe = root->child_at(subframe_index); + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_TRUE(new_subframe->is_on_initial_empty_document()); + // Do a navigation on the "child6" subframe to |url_2|. // The navigation is still classified as "auto", so we didn't append a new // NavigationEntry, and instead updated the current NavigationEntry. - NavigateSubframeAndCheckNavigationType( - contents(), root->child_at(subframe_index), "child6", url_2, - NAVIGATION_TYPE_AUTO_SUBFRAME); + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child6", + url_2, + NAVIGATION_TYPE_AUTO_SUBFRAME); + EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + + // The subframe should no longer be on the initial empty document. + EXPECT_TRUE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); + } + + // 7) Navigate to a non-initial/synchronous about:blank and then |url_2| on + // a new subframe. + { + SCOPED_TRACE(testing::Message() << " Testing case 7."); + + // Create the "child7" subframe with src set to about:blank, navigating it + // there. + CreateSubframe(contents(), "child7", GURL("about:blank"), + true /* wait_for_navigation */); + subframe_index++; + EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + + // The new subframe should be on the initial empty document. + FrameTreeNode* new_subframe = root->child_at(subframe_index); + EXPECT_FALSE(new_subframe->has_committed_real_load()); + EXPECT_TRUE(new_subframe->is_on_initial_empty_document()); + + // Do a navigation on the "child7" subframe to about:blank?foo, creating a + // new about:blank document that is neither the initial about:blank nor the + // synchronously committed about:blank document, so it's not treated as the + // initial empty document. The navigation will do a replacement and get + // classified as "auto", so we won't append a new NavigationEntry, and + // instead update the current NavigationEntry. + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child7", + GURL("about:blank?foo"), + NAVIGATION_TYPE_AUTO_SUBFRAME); + EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + + // The subframe should no longer be on the the initial empty document. + EXPECT_TRUE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); + + // Do a navigation on the "child7" subframe to |url_2|. + NavigateSubframeAndCheckNavigationType(contents(), new_subframe, "child7", + url_2, NAVIGATION_TYPE_NEW_SUBFRAME); + // The navigation is classified as a new navigation, and appended a new + // NavigationEntry. + expected_entry_count++; EXPECT_EQ(expected_entry_count, controller.GetEntryCount()); + + // The subframe is not on the initial empty document. + EXPECT_TRUE(new_subframe->has_committed_real_load()); + EXPECT_FALSE(new_subframe->is_on_initial_empty_document()); } } @@ -4129,9 +4214,6 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, } // 2) Navigate to about:blank on a new window that hasn't done any navigation. - // This case is not enabled for browser-initiated navigation because the - // browser-calculated vs renderer-calculated origin doesn't match, leading to - // a crash. { SCOPED_TRACE(testing::Message() << " Testing case 2."); @@ -4143,12 +4225,34 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, EXPECT_EQ(0, controller.GetEntryCount()); EXPECT_FALSE(controller.GetLastCommittedEntry()); + // The window should be on the initial empty document. + FrameTreeNode* new_root = new_contents->GetFrameTree()->root(); + EXPECT_FALSE(new_root->has_committed_real_load()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); + // Navigating the window to about:blank will be classified as NEW_ENTRY // and will add a new entry. NavigateWindowAndCheckNavigationTypeIsNewEntry(new_contents, GURL("about:blank")); EXPECT_EQ(1, controller.GetEntryCount()); EXPECT_TRUE(controller.GetLastCommittedEntry()); + + // The window is no longer on the initial empty document. + EXPECT_TRUE(new_root->has_committed_real_load()); + EXPECT_FALSE(new_root->is_on_initial_empty_document()); + + // Navigating the window to about:blank#foo will be classified as a same- + // document NEW_ENTRY, and will add a new entry. + NavigateWindowAndCheckNavigationTypeIsNewEntry( + new_contents, GURL("about:blank#foo"), + true /* wait_for_previous_navigations */, + true /* expect_same_document */); + EXPECT_EQ(2, controller.GetEntryCount()); + + // The window is still marked as no longer being on the initial empty + // document. + EXPECT_TRUE(new_root->has_committed_real_load()); + EXPECT_FALSE(new_root->is_on_initial_empty_document()); } // 3) Navigate to about:blank#foo on a new window that hasn't done any @@ -4164,6 +4268,11 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, EXPECT_EQ(0, controller.GetEntryCount()); EXPECT_FALSE(controller.GetLastCommittedEntry()); + // The window should be on the initial empty document. + FrameTreeNode* new_root = new_contents->GetFrameTree()->root(); + EXPECT_FALSE(new_root->has_committed_real_load()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); + // Navigating the window to about:blank#foo will be classified as a same- // document NEW_ENTRY, and will add a new entry. NavigateWindowAndCheckNavigationTypeIsNewEntry( @@ -4172,6 +4281,10 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, true /* expect_same_document */); EXPECT_EQ(1, controller.GetEntryCount()); EXPECT_TRUE(controller.GetLastCommittedEntry()); + + // The window should still be on the initial empty document. + EXPECT_FALSE(new_root->has_committed_real_load()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); } // 4) Navigate to |url_2| on a new window that initially loaded about:blank @@ -4189,6 +4302,11 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, NavigationEntryImpl* last_entry = controller.GetLastCommittedEntry(); EXPECT_TRUE(last_entry); + // The window should be on the initial empty document. + FrameTreeNode* new_root = new_contents->GetFrameTree()->root(); + EXPECT_FALSE(new_root->has_committed_real_load()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); + // Do a navigation on the window to about:blank#foo, creating a // same-document navigation. NavigateWindowAndCheckNavigationTypeIsNewEntry( @@ -4203,11 +4321,19 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, ->GetMainFrameDocumentSequenceNumber()); last_entry = controller.GetLastCommittedEntry(); + // The window should still be on the initial empty document. + EXPECT_FALSE(new_root->has_committed_real_load()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); + // Navigating the window to |url_2| will be classified as NEW_ENTRY and will // add a new entry. NavigateWindowAndCheckNavigationTypeIsNewEntry(new_contents, url_2); EXPECT_EQ(3, controller.GetEntryCount()); EXPECT_NE(last_entry, controller.GetLastCommittedEntry()); + + // The window is no longer on the initial empty document. + EXPECT_TRUE(new_root->has_committed_real_load()); + EXPECT_FALSE(new_root->is_on_initial_empty_document()); } // 5) Navigate to |url_2| on a new window that has started a navigation to @@ -4224,12 +4350,21 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, EXPECT_EQ(0, controller.GetEntryCount()); EXPECT_FALSE(controller.GetLastCommittedEntry()); + // The window should be on the initial empty document. + FrameTreeNode* new_root = new_contents->GetFrameTree()->root(); + EXPECT_FALSE(new_root->has_committed_real_load()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); + // Navigate to |url_2|, and ensure that we won't wait for the |hung_url| // navigation to finish. NavigateWindowAndCheckNavigationTypeIsNewEntry( new_contents, url_2, false /* wait_for_previous_navigations */); EXPECT_EQ(1, controller.GetEntryCount()); EXPECT_TRUE(controller.GetLastCommittedEntry()); + + // The window is no longer on the initial empty document. + EXPECT_TRUE(new_root->has_committed_real_load()); + EXPECT_FALSE(new_root->is_on_initial_empty_document()); } // 6) Navigate to |url_2| on a new window that has done a document.open(). @@ -4243,12 +4378,11 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, NavigationControllerImpl& controller = new_contents->GetController(); EXPECT_EQ(0, controller.GetEntryCount()); EXPECT_FALSE(controller.GetLastCommittedEntry()); - // The window should be on its initial empty document. + // The window should be on its initial empty document. FrameTreeNode* new_root = new_contents->GetPrimaryFrameTree().root(); EXPECT_FALSE(new_root->has_committed_real_load()); - EXPECT_TRUE( - new_root->is_on_initial_empty_document_or_subsequent_empty_documents()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); // Do a document.open() on the blank window. TestNavigationObserver nav_observer(new_contents); @@ -4274,14 +4408,18 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, // `has_committed_real_load` is still false because the last committed URL // stays the same. EXPECT_FALSE(new_root->has_committed_real_load()); - EXPECT_FALSE( - new_root->is_on_initial_empty_document_or_subsequent_empty_documents()); + EXPECT_FALSE(new_root->is_on_initial_empty_document()); // Navigating the window to |url_2| will be classified as NEW_ENTRY and will // add a new entry. NavigateWindowAndCheckNavigationTypeIsNewEntry(new_contents, url_2); EXPECT_EQ(1, controller.GetEntryCount()); EXPECT_TRUE(controller.GetLastCommittedEntry()); + + // The window's `has_committed_real_load` status is now true after it + // committed a new document that replaced the initial empty document. + EXPECT_TRUE(new_root->has_committed_real_load()); + EXPECT_FALSE(new_root->is_on_initial_empty_document()); } // 7) Navigate to |url_2| on a new window that has navigated to a javascript: @@ -4298,11 +4436,20 @@ IN_PROC_BROWSER_TEST_P(InitialEmptyDocNavigationControllerBrowserTest, EXPECT_EQ(0, controller.GetEntryCount()); EXPECT_FALSE(controller.GetLastCommittedEntry()); + // The window should be on its initial empty document. + FrameTreeNode* new_root = new_contents->GetFrameTree()->root(); + EXPECT_FALSE(new_root->has_committed_real_load()); + EXPECT_TRUE(new_root->is_on_initial_empty_document()); + // Navigating the window to |url_2| will be classified as NEW_ENTRY and will // add a new entry. NavigateWindowAndCheckNavigationTypeIsNewEntry(new_contents, url_2); EXPECT_EQ(1, controller.GetEntryCount()); EXPECT_TRUE(controller.GetLastCommittedEntry()); + + // The window is no longer on the initial empty document. + EXPECT_TRUE(new_root->has_committed_real_load()); + EXPECT_FALSE(new_root->is_on_initial_empty_document()); } } diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc index 72870788123478..de3fc220dacd9d 100644 --- a/content/browser/renderer_host/navigation_request.cc +++ b/content/browser/renderer_host/navigation_request.cc @@ -6969,12 +6969,8 @@ bool NavigationRequest:: return false; } - if (!frame_tree_node_ - ->is_on_initial_empty_document_or_subsequent_empty_documents()) { - // Return if we're not on the initial empty document (or subsequent empty - // documents). - // TODO(https://crbug.com/1215096): Only replace when we're on the initial - // empty document (don't replace on subsequent empty documents). + if (!frame_tree_node_->is_on_initial_empty_document()) { + // Return if we're not on the initial empty document. return false; } diff --git a/content/browser/renderer_host/navigator.cc b/content/browser/renderer_host/navigator.cc index a4da8b495c1149..812037846402d6 100644 --- a/content/browser/renderer_host/navigator.cc +++ b/content/browser/renderer_host/navigator.cc @@ -482,8 +482,7 @@ void Navigator::DidNavigate( // node to consider itself no longer on the initial empty document. Record // whether we're leaving the initial empty document before that. bool was_on_initial_empty_document = - frame_tree_node - ->is_on_initial_empty_document_or_subsequent_empty_documents(); + frame_tree_node->is_on_initial_empty_document(); render_frame_host->DidNavigate(params, navigation_request.get(), was_within_same_document); diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc index 445cca845ea7c9..ad2ed0b77153d3 100644 --- a/content/browser/renderer_host/render_frame_host_impl.cc +++ b/content/browser/renderer_host/render_frame_host_impl.cc @@ -2948,10 +2948,8 @@ bool RenderFrameHostImpl::CreateRenderFrame( params->frame_owner_properties = frame_tree_node()->frame_owner_properties().Clone(); - // TOOD(https://crbug.com/1215096): Make this use - // is_on_initial_empty_document_or_subsequent_empty_documents() instead. - params->has_committed_real_load = - frame_tree_node()->has_committed_real_load(); + params->is_on_initial_empty_document = + frame_tree_node()->is_on_initial_empty_document(); // The RenderWidgetHost takes ownership of its view. It is tied to the // lifetime of the current RenderProcessHost for this RenderFrameHost. @@ -3511,6 +3509,19 @@ void RenderFrameHostImpl::DidNavigate( navigation_request->frame_tree_node()->SetCurrentURL(params.url); SetLastCommittedOrigin(params.origin); + // If the navigation was a cross-document navigation and it's not the + // synchronous about:blank commit, then it committed a document that is not + // the initial empty document. Note that the + // DidCommitNonInitialEmptyDocument() call only actually changes the state of + // the FrameTreeNode the first time it was called (it changes the state from + // "is on the initial empty document" to "not on the initial empty document", + // and we never go back to the former state). + if (!navigation_request->IsSameDocument() && + (!navigation_request->is_synchronous_renderer_commit() || + !navigation_request->GetURL().IsAboutBlank())) { + navigation_request->frame_tree_node()->DidCommitNonInitialEmptyDocument(); + } + // For uuid-in-package: and urn: resources served from WebBundles, use the // Bundle's origin. // TODO(https://crbug.com/1257045): Remove urn: scheme support. @@ -9780,9 +9791,9 @@ void RenderFrameHostImpl::GetVirtualAuthenticatorManager( } bool IsInitialSynchronousAboutBlankCommit(const GURL& url, - bool has_committed_real_load) { + bool is_on_initial_empty_document) { return url.SchemeIs(url::kAboutScheme) && url != GURL(url::kAboutSrcdocURL) && - !has_committed_real_load; + is_on_initial_empty_document; } std::unique_ptr @@ -9804,7 +9815,7 @@ RenderFrameHostImpl::CreateNavigationRequestForSynchronousRendererCommit( // after the initial empty document. // 2) This was a renderer-initiated same-document navigation. DCHECK(IsInitialSynchronousAboutBlankCommit( - url, frame_tree_node_->has_committed_real_load()) || + url, frame_tree_node_->is_on_initial_empty_document()) || is_same_document); DCHECK(!is_same_document_history_api_navigation || is_same_document); @@ -10106,7 +10117,7 @@ bool RenderFrameHostImpl::ValidateDidCommitParams( // navigations won't have a NavigationRequest at this point, we need to check // |renderer_url_info_.was_loaded_from_load_data_with_base_url|. DCHECK(navigation_request || is_same_document_navigation || - !frame_tree_node_->has_committed_real_load()); + frame_tree_node_->is_on_initial_empty_document()); bool bypass_checks_for_webview = false; if ((navigation_request && navigation_request->IsLoadDataWithBaseURL()) || (is_same_document_navigation && @@ -10305,11 +10316,11 @@ bool RenderFrameHostImpl::DidCommitNavigationInternal( // FrameHostMsg_DidCommitProvisionalLoad_Params at all. // TODO(https://crbug.com/1215096): Tighten the checks for case 1 so that only // the synchronous about:blank commit can actually go through (e.g. check - // if the frame's initial empty document state, instead of checking the - // less-accurate `has_committed_real_load`). + // if the URL is exactly "about:blank", currently we allow any "about:" URL + // except for "about:srcdoc"). const bool is_synchronous_about_blank_commit = IsInitialSynchronousAboutBlankCommit( - params->url, frame_tree_node_->has_committed_real_load()); + params->url, frame_tree_node_->is_on_initial_empty_document()); if (!navigation_request && !is_synchronous_about_blank_commit && !is_same_document_navigation) { LogCannotCommitUrlCrashKeys(params->url, is_same_document_navigation, @@ -11881,8 +11892,7 @@ void RenderFrameHostImpl:: request->frame_tree_node()->has_committed_real_load()); SCOPED_CRASH_KEY_BOOL( "VerifyDidCommit", "on_initial_empty_doc", - request->frame_tree_node() - ->is_on_initial_empty_document_or_subsequent_empty_documents()); + request->frame_tree_node()->is_on_initial_empty_document()); SCOPED_CRASH_KEY_STRING256("VerifyDidCommit", "last_committed_url", GetLastCommittedURL().spec()); diff --git a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc index bd4d21c8c2fc04..74c6beb709fc70 100644 --- a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc +++ b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc @@ -4087,10 +4087,7 @@ IN_PROC_BROWSER_TEST_P(RenderFrameHostManagerTest, // it as having committed a real load. The FrameTreeVisualizer test should be // enough to ensure that the childmost frame is not loaded. EXPECT_FALSE(root->child_at(0)->child_at(0)->has_committed_real_load()); - EXPECT_FALSE( - root->child_at(0) - ->child_at(0) - ->is_on_initial_empty_document_or_subsequent_empty_documents()); + EXPECT_FALSE(root->child_at(0)->child_at(0)->is_on_initial_empty_document()); } // Ensure that navigating a subframe to the same URL as its parent twice in a diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index 9fda0d2e8f157b..7518b5e436961b 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc @@ -434,8 +434,8 @@ bool RenderViewHostImpl::CreateRenderView( main_rfh->BindBrowserInterfaceBrokerReceiver( local_frame_params->interface_broker.InitWithNewPipeAndPassReceiver()); - local_frame_params->has_committed_real_load = - main_rfh->frame_tree_node()->has_committed_real_load(); + local_frame_params->is_on_initial_empty_document = + main_rfh->frame_tree_node()->is_on_initial_empty_document(); // If this is a new RenderFrameHost for a frame that has already committed a // document, we don't have a PolicyContainerHost yet. Indeed, in that case, diff --git a/content/common/frame.mojom b/content/common/frame.mojom index 268a2395423ebc..afd57b6d28f828 100644 --- a/content/common/frame.mojom +++ b/content/common/frame.mojom @@ -176,8 +176,8 @@ struct CreateLocalMainFrameParams { pending_remote interface_broker; - // Whether or not the frame has previously committed a real load. - bool has_committed_real_load; + // Whether the frame is still on the initial empty document or not. + bool is_on_initial_empty_document = true; // Null when the main frame has no policy container yet (for example, because // it is a speculative RenderFrameHost), and the policy container will be @@ -283,8 +283,8 @@ struct CreateFrameParams { // new RenderFrame (if one is needed). CreateFrameWidgetParams? widget_params; - // Whether or not the frame has previously committed a real load. - bool has_committed_real_load; + // Whether the frame is still on the initial empty document or not. + bool is_on_initial_empty_document = true; // The policy container for the frame to be created. This can be null if we // could not determine a policy container yet, for example in case of a diff --git a/content/renderer/agent_scheduling_group.cc b/content/renderer/agent_scheduling_group.cc index c651a73b3e3c77..4d5a704e9c622d 100644 --- a/content/renderer/agent_scheduling_group.cc +++ b/content/renderer/agent_scheduling_group.cc @@ -235,7 +235,8 @@ void AgentSchedulingGroup::CreateFrame(mojom::CreateFrameParamsPtr params) { params->tree_scope_type, std::move(params->replication_state), std::move(params->widget_params), std::move(params->frame_owner_properties), - params->has_committed_real_load, std::move(params->policy_container)); + params->is_on_initial_empty_document, + std::move(params->policy_container)); } void AgentSchedulingGroup::CreateFrameProxy( diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc index 0a366dab6ac61c..8a887e8f90c6a4 100644 --- a/content/renderer/render_frame_impl.cc +++ b/content/renderer/render_frame_impl.cc @@ -1501,8 +1501,8 @@ RenderFrameImpl* RenderFrameImpl::CreateMainFrame( // WebString... WebString::FromUTF8(replication_state->name), replication_state->frame_policy.sandbox_flags); - if (params->has_committed_real_load) - render_frame->frame_->SetCommittedFirstRealLoad(); + if (!params->is_on_initial_empty_document) + render_frame->frame_->SetIsNotOnInitialEmptyDocument(); // Non-owning pointer that is self-referencing and destroyed by calling // Close(). The RenderViewImpl has a RenderWidget already, but not a @@ -1580,7 +1580,7 @@ void RenderFrameImpl::CreateFrame( blink::mojom::FrameReplicationStatePtr replicated_state, mojom::CreateFrameWidgetParamsPtr widget_params, blink::mojom::FrameOwnerPropertiesPtr frame_owner_properties, - bool has_committed_real_load, + bool is_on_initial_empty_document, blink::mojom::PolicyContainerPtr policy_container) { // TODO(danakj): Split this method into two pieces. The first block makes a // WebLocalFrame and collects the RenderView and RenderFrame for it. The @@ -1762,8 +1762,8 @@ void RenderFrameImpl::CreateFrame( web_frame_widget->ApplyVisualProperties(widget_params->visual_properties); } - if (has_committed_real_load) - render_frame->frame_->SetCommittedFirstRealLoad(); + if (!is_on_initial_empty_document) + render_frame->frame_->SetIsNotOnInitialEmptyDocument(); render_frame->Initialize(web_frame->Parent()); } @@ -5247,9 +5247,10 @@ void RenderFrameImpl::BeginNavigation( // Mainly a proxy for checking about:blank, even though it can match // other things like about:srcdoc (or any empty document schemes that // are registered). - // TODO(https://crbug.com/1215096): This should be changed to only check - // for about:blank because the navigation triggered by browsing context - // creation will always navigate to about:blank. + // TODO(https://crbug.com/1215096): Tighten the condition to only accept + // about:blank or an empty URL which defaults to about:blank, per the + // spec: + // https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element:about:blank WebDocumentLoader::WillLoadUrlAsEmpty(url) && // The navigation method must be "GET". This is to avoid issues like // https://crbug.com/1210653, where a form submits to about:blank @@ -5262,16 +5263,16 @@ void RenderFrameImpl::BeginNavigation( // navigation can't possibly be triggered by browsing context creation, // which would have triggered the navigation synchronously as the first // navigation in this frame. Note that we check both - // HasCommittedFirstRealLoad() and `first_navigation_in_render_frame` + // IsOnInitialEmptyDocument() and `first_navigation_in_render_frame` // here because `first_navigation_in_render_frame` only tracks the state // in this *RenderFrame*, so it will be true even if this navigation // happens on a frame that has existed before in another process (e.g. // an