Skip to content

Commit

Permalink
Improve initial empty document state tracking
Browse files Browse the repository at this point in the history
Currently knowing whether a frame is showing the initial empty document
or not is hard to do because there are multiple places that try to
track the "initial empty document" state but actually fail to do so
due to various reasons:
- is_on_initial_empty_document_or_subsequent_empty_documents_ in
  FrameTreeNode and empty_document_status_ in FrameLoader are still
  true after multiple about:blank (and other about: URLs) commits that
  are not the initial/synchronous about:blank.
- has_committed_real_load_ in FrameTreeNode is used to determine the
  initial empty document state (it is used as input to
  empty_document_status_ in FrameLoader and IsInitialEmptyDocument() in
  Document), even though it does not consider document.open()

This CL fixes those problems and more:
- Sets is_on_initial_empty_document_or_subsequent_empty_documents_ and
  empty_document_status_ to false on the first commit that is not the
  initial/synchronous about:blank commit (and renames the former to
  is_on_initial_empty_document now that it actually represents the
  exact state)
- Removes uses of has_committed_real_load_ that use it to determine
  the initial empty document state (it's going to be removed completely
  in crrev.com/c/3244814)
- Renames and adds comments to various bits for easier understanding

After this CL, the "initial empty document" state tracking matches the
spec except that Chrome treats the initial about:blank document and the
synchronously loaded about:blank document as the same document (the
initial empty document), while the synchronously loaded about:blank
document does not exist in the spec (especially after
whatwg/html#6863).

Some subtle changes:
- about:blank documents that committed immediately after the
  initial/synchronous about:blank document are no longer treated as the
  initial empty document. This mainly affects history replacement
  decisions (since the initial empty document's history entry always
  gets replaced on the next navigation), so these documents' history
  entry will no longer be replaced based on their
  initial-empty-document-ness.
  Example of previous behavior:
  1. <iframe src="about:blank">. This will commit the synchronous
     about:blank document, and is correctly treated as the initial
     empty document.
  2. Navigate to about:blank or any other variants of it, e.g.
     about:blank?foo. This is a new document that is not the initial
     about:blank or the synchronous about:blank, but since it's an
     about:blank URL, it used to be treated as the initial empty
     document. With this CL, it's no longer treated as the initial
     empty document.
  3. Navigate to some other URL. This used to do replacement because
     we thought we're still on the initial empty document, but we
     actually are not. This CL fixes this case.
- Non-about:blank commits that accidentally go through the synchronous
  about:blank commit path are now correctly treated as non-initial
  empty document commits (but still go through the path for now, might
  be removed later). This includes all variants of about: URLs except
  for about:srcdoc that are not about:blank (e.g. about:mumble) - see
  the conditions in RenderFrameImpl::BeginNavigation(). This makes our
  behavior closer to the spec, which only allows about:blank
  variations to be treated as the initial empty document.

Bug: 1215096
Change-Id: Ic52bc562a4fbfdd92a63d980c75dd952a4e099d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3238476
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936787}
  • Loading branch information
rakina authored and Chromium LUCI CQ committed Nov 1, 2021
1 parent 8a3b97d commit 86c88fa
Show file tree
Hide file tree
Showing 23 changed files with 390 additions and 164 deletions.
9 changes: 5 additions & 4 deletions content/browser/renderer_host/frame_tree_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
76 changes: 43 additions & 33 deletions content/browser/renderer_host/frame_tree_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
3 changes: 1 addition & 2 deletions content/browser/renderer_host/isolated_app_throttle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Loading

0 comments on commit 86c88fa

Please sign in to comment.