Skip to content

Commit

Permalink
[Sheriff] Revert "Implement appHistory.entries()"
Browse files Browse the repository at this point in the history 
This reverts commit b2189ba.

Reason for revert: This CL is suspicious for causing webkit leak tests to fail. First build: https://ci.chromium.org/ui/p/chromium/builders/ci/WebKit%20Linux%20Leak/25321/overview

Original change's description:
> Implement appHistory.entries()
>
> Proposal: https://github.com/WICG/app-history
>
> This introduces the entries() function to the appHistory API. It
> returns an array of AppHistoryEntries that represent the subset of the
> back/forward list that is same-origin and contiguous to the current
> entry. This is constructed in NaivgationRequest just before a commit
> is sent to the renderer for cross-document commits. For same-document
> commits, the renderer modifies the existing appHistory object's
> entries_ array directly.
>
> Bug: 1183545
> Change-Id: I4652b7f8f6c9cc3c26b99314555b6109a58f2d1f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2803480
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Domenic Denicola <domenic@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Mason Freed <masonf@chromium.org>
> Commit-Queue: Nate Chapin <japhet@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#872181}

Bug: 1183545
Change-Id: I733ba216e21c37a39c4eb2d15492c2480141793d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2825789
Reviewed-by: Anatoliy Potapchuk <apotapchuk@google.com>
Reviewed-by: Anatoliy Potapchuk <apotapchuk@chromium.org>
Owners-Override: Anatoliy Potapchuk <apotapchuk@google.com>
Auto-Submit: Anatoliy Potapchuk <apotapchuk@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Anatoliy Potapchuk <apotapchuk@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#872340}
  • Loading branch information
Anatoliy Potapchuk authored and Chromium LUCI CQ committed Apr 14, 2021
1 parent e15473b commit 4b0a605
Show file tree
Hide file tree
Showing 40 changed files with 40 additions and 586 deletions.
96 changes: 2 additions & 94 deletions content/browser/renderer_host/navigation_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@
#include "content/browser/web_package/web_bundle_navigation_info.h"
#include "content/common/content_constants_internal.h"
#include "content/common/frame_messages.h"
#include "content/common/navigation_params_utils.h"
#include "content/common/trace_utils.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/content_browser_client.h"
Expand Down Expand Up @@ -3565,12 +3564,8 @@ NavigationControllerImpl::CreateNavigationRequestFromLoadParams(
false /* origin_agent_cluster */,
std::vector<
network::mojom::WebClientHintsType>() /* enabled_client_hints */,
false /* is_cross_browsing_instance */, nullptr /* old_page_info */,
-1 /* http_response_code */,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_back_entries */,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_forward_entries */);
false /* is_cross_browsing_instance */,
nullptr /* old_page_info */, -1 /* http_response_code */);
#if defined(OS_ANDROID)
if (ValidateDataURLAsString(params.data_url_as_string)) {
commit_params->data_url_as_string = params.data_url_as_string->data();
Expand Down Expand Up @@ -4136,91 +4131,4 @@ void NavigationControllerImpl::LogStoragePartitionIdCrashKeys(
base::debug::DumpWithoutCrashing();
}

std::vector<mojom::AppHistoryEntryPtr>
NavigationControllerImpl::PopulateSingleAppHistoryEntryVector(
Direction direction,
int entry_index,
const url::Origin& pending_origin,
FrameTreeNode* node,
SiteInstance* site_instance,
int64_t previous_item_sequence_number) {
std::vector<mojom::AppHistoryEntryPtr> entries;
int offset = direction == Direction::kForward ? 1 : -1;
for (int i = entry_index + offset; i >= 0 && i < GetEntryCount();
i += offset) {
FrameNavigationEntry* frame_entry = GetEntryAtIndex(i)->GetFrameEntry(node);
if (!frame_entry || !frame_entry->committed_origin())
break;
if (site_instance != frame_entry->site_instance())
break;
if (!pending_origin.IsSameOriginWith(*frame_entry->committed_origin()))
break;
if (previous_item_sequence_number == frame_entry->item_sequence_number())
continue;
blink::ExplodedPageState exploded_page_state;
if (blink::DecodePageState(frame_entry->page_state().ToEncodedData(),
&exploded_page_state)) {
blink::ExplodedFrameState frame_state = exploded_page_state.top;
mojom::AppHistoryEntryPtr entry = mojom::AppHistoryEntry::New(
frame_state.app_history_key.value_or(std::u16string()),
frame_state.app_history_id.value_or(std::u16string()),
frame_state.url_string.value_or(std::u16string()));
DCHECK(pending_origin.CanBeDerivedFrom(GURL(entry->url)));
entries.push_back(std::move(entry));
previous_item_sequence_number = frame_entry->item_sequence_number();
}
}
// If |entries| was constructed by iterating backwards from
// |entry_index|, it's latest-at-the-front, but the renderer will want it
// earliest-at-the-front. Reverse it.
if (direction == Direction::kBack)
std::reverse(entries.begin(), entries.end());
return entries;
}

void NavigationControllerImpl::PopulateAppHistoryEntryVectors(
NavigationRequest* request) {
url::Origin pending_origin =
request->commit_params().origin_to_commit
? *request->commit_params().origin_to_commit
: url::Origin::Create(request->common_params().url);

FrameTreeNode* node = request->frame_tree_node();
scoped_refptr<SiteInstance> site_instance =
request->GetRenderFrameHost()->GetSiteInstance();

// NOTE: |entry_index| is an estimate of the index where this entry will
// commit, but it may be wrong in corner cases (e.g., if we are at the max
// entry limit, the earliest entry will be dropped). This is ok because this
// algorithm only uses |entry_index| to walk the entry list as it stands right
// now, and it isn't saved for anything post-commit.
int entry_index = GetPendingEntryIndex();
bool will_create_new_entry = false;
if (NavigationTypeUtils::IsReload(request->common_params().navigation_type) ||
request->common_params().should_replace_current_entry) {
entry_index = GetLastCommittedEntryIndex();
} else if (entry_index == -1) {
will_create_new_entry = true;
entry_index = GetLastCommittedEntryIndex() + 1;
}

int64_t pending_item_sequence_number = 0;
if (auto* pending_entry = GetPendingEntry()) {
if (auto* frame_entry = pending_entry->GetFrameEntry(node))
pending_item_sequence_number = frame_entry->item_sequence_number();
}

request->set_app_history_back_entries(PopulateSingleAppHistoryEntryVector(
Direction::kBack, entry_index, pending_origin, node, site_instance.get(),
pending_item_sequence_number));

// Don't populate forward entries if they will be truncated by a new entry.
if (!will_create_new_entry) {
request->set_app_history_forward_entries(
PopulateSingleAppHistoryEntryVector(
Direction::kForward, entry_index, pending_origin, node,
site_instance.get(), pending_item_sequence_number));
}
}

} // namespace content
17 changes: 0 additions & 17 deletions content/browser/renderer_host/navigation_controller_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,13 +354,6 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
bool should_replace_entry,
WebContents* web_contents);

// Called just before sending the commit to the renderer. Walks the
// session history entries for the committing FrameTreeNode, forward and
// backward from the pending entry. All contiguous and same-origin
// FrameNavigationEntries are serialized and added to |request|'s commit
// params.
void PopulateAppHistoryEntryVectors(NavigationRequest* request);

private:
friend class RestoreHelper;

Expand Down Expand Up @@ -669,16 +662,6 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
const StoragePartitionId& original_partition_id,
const StoragePartitionId& new_partition_id);

// Used by PopulateAppHistoryEntryVectors to initialize a single vector.
enum class Direction { kForward, kBack };
std::vector<mojom::AppHistoryEntryPtr> PopulateSingleAppHistoryEntryVector(
Direction direction,
int entry_index,
const url::Origin& pending_origin,
FrameTreeNode* node,
SiteInstance* site_instance,
int64_t previous_item_sequence_number);

// ---------------------------------------------------------------------------

// The FrameTree this instance belongs to. Each FrameTree gets its own
Expand Down
8 changes: 2 additions & 6 deletions content/browser/renderer_host/navigation_entry_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -804,12 +804,8 @@ NavigationEntryImpl::ConstructCommitNavigationParams(
false /* origin_agent_cluster */,
std::vector<
network::mojom::WebClientHintsType>() /* enabled_client_hints */,
false /* is_cross_browsing_instance */, nullptr /* old_page_info */,
-1 /* http_response_code */,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_back_entries */,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_forward_entries */);
false /* is_cross_browsing_instance */,
nullptr /* old_page_info */, -1 /* http_response_code */);
#if defined(OS_ANDROID)
if (NavigationControllerImpl::ValidateDataURLAsString(GetDataURLAsString())) {
commit_params->data_url_as_string = GetDataURLAsString()->data();
Expand Down
17 changes: 3 additions & 14 deletions content/browser/renderer_host/navigation_request.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -967,11 +967,7 @@ std::unique_ptr<NavigationRequest> NavigationRequest::CreateRendererInitiated(
/*enabled_client_hints=*/
std::vector<network::mojom::WebClientHintsType>(),
/*is_cross_browsing_instance=*/false,
/*old_page_info=*/nullptr, /*http_response_code=*/-1,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_back_entries */,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_forward_entries */);
/*old_page_info=*/nullptr, /*http_response_code=*/-1);

// CreateRendererInitiated() should only be triggered when the navigation is
// initiated by a frame in the same process.
Expand Down Expand Up @@ -1082,12 +1078,8 @@ std::unique_ptr<NavigationRequest> NavigationRequest::CreateForCommit(
false /* origin_agent_cluster */,
std::vector<
network::mojom::WebClientHintsType>() /* enabled_client_hints */,
false /* is_cross_browsing_instance */, nullptr /* old_page_info */,
http_response_code,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_back_entries */,
std::vector<
mojom::AppHistoryEntryPtr>() /* app_history_forward_entries */);
false /* is_cross_browsing_instance */,
nullptr /* old_page_info */, http_response_code);
mojom::BeginNavigationParamsPtr begin_params =
mojom::BeginNavigationParams::New();
std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(
Expand Down Expand Up @@ -3935,9 +3927,6 @@ void NavigationRequest::CommitNavigation() {
commit_params_->is_prerendering =
frame_tree_node_->frame_tree()->is_prerendering();

if (!IsSameDocument())
GetNavigationController()->PopulateAppHistoryEntryVectors(this);

auto common_params = common_params_->Clone();
auto commit_params = commit_params_.Clone();
auto response_head = response_head_.Clone();
Expand Down
10 changes: 0 additions & 10 deletions content/browser/renderer_host/navigation_request.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,16 +380,6 @@ class CONTENT_EXPORT NavigationRequest
commit_params_->is_cross_browsing_instance = is_cross_browsing_instance;
}

void set_app_history_back_entries(
std::vector<mojom::AppHistoryEntryPtr> entries) {
commit_params_->app_history_back_entries = std::move(entries);
}

void set_app_history_forward_entries(
std::vector<mojom::AppHistoryEntryPtr> entries) {
commit_params_->app_history_forward_entries = std::move(entries);
}

NavigationURLLoader* loader_for_testing() const { return loader_.get(); }

NavigationState state() const { return state_; }
Expand Down
16 changes: 0 additions & 16 deletions content/common/navigation_params.mojom
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ module content.mojom;

import "content/common/prefetched_signed_exchange_info.mojom";
import "content/public/common/was_activated_option.mojom";
import "mojo/public/mojom/base/string16.mojom";
import "mojo/public/mojom/base/time.mojom";
import "mojo/public/mojom/base/unguessable_token.mojom";
import "mojo/public/mojom/base/values.mojom";
Expand Down Expand Up @@ -280,16 +279,6 @@ struct OldPageInfo {
blink.mojom.PageLifecycleState new_lifecycle_state_for_old_page;
};

// AppHistoryEntry contains a subset of a session history item that is used by
// the appHistory API to represent a non-current history entry. The browser
// ensures that an AppHistoryEntry is only sent to the renderer if its |url| is
// same-origin to the navigation being committed.
struct AppHistoryEntry {
mojo_base.mojom.String16 key;
mojo_base.mojom.String16 id;
mojo_base.mojom.String16 url;
};

// Used by commit IPC messages. Holds the parameters needed by the renderer to
// commit a navigation besides those in CommonNavigationParams.
struct CommitNavigationParams {
Expand Down Expand Up @@ -482,9 +471,4 @@ struct CommitNavigationParams {
// The HTTP response status code for the navigation. Set to -1 if we never
// received response headers.
int32 http_response_code = -1;

// A same-origin subset of the back/forward list exposed by the appHistory
// API.
array<AppHistoryEntry> app_history_back_entries;
array<AppHistoryEntry> app_history_forward_entries;
};
22 changes: 0 additions & 22 deletions content/renderer/render_frame_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1003,28 +1003,6 @@ void FillMiscNavigationParams(

if (commit_params.http_response_code != -1)
navigation_params->http_status_code = commit_params.http_response_code;

navigation_params->app_history_back_entries.reserve(
commit_params.app_history_back_entries.size());
for (const auto& entry : commit_params.app_history_back_entries) {
WebHistoryItem item;
item.Initialize();
item.SetAppHistoryKey(WebString::FromUTF16(entry->key));
item.SetAppHistoryId(WebString::FromUTF16(entry->id));
item.SetURLString(WebString::FromUTF16(entry->url));
navigation_params->app_history_back_entries.emplace_back(item);
}

navigation_params->app_history_forward_entries.reserve(
commit_params.app_history_forward_entries.size());
for (const auto& entry : commit_params.app_history_forward_entries) {
WebHistoryItem item;
item.Initialize();
item.SetAppHistoryKey(WebString::FromUTF16(entry->key));
item.SetAppHistoryId(WebString::FromUTF16(entry->id));
item.SetURLString(WebString::FromUTF16(entry->url));
navigation_params->app_history_forward_entries.emplace_back(item);
}
}

// Fills in the origin policy associated with this response, if any is present.
Expand Down
6 changes: 0 additions & 6 deletions third_party/blink/public/web/web_navigation_params.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,12 +421,6 @@ struct BLINK_EXPORT WebNavigationParams {
// Blink's copy of the policy container containing security policies to be
// enforced on the document created by this navigation.
std::unique_ptr<WebPolicyContainer> policy_container;

// These are used to construct a subset of the back/forward list for the
// appHistory API. They only have the attributes that are needed for
// appHistory.
WebVector<WebHistoryItem> app_history_back_entries;
WebVector<WebHistoryItem> app_history_forward_entries;
};

} // namespace blink
Expand Down
87 changes: 4 additions & 83 deletions third_party/blink/renderer/core/app_history/app_history.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,91 +28,12 @@ AppHistory* AppHistory::appHistory(LocalDOMWindow& window) {
AppHistory::AppHistory(LocalDOMWindow& window)
: Supplement<LocalDOMWindow>(window) {}

void AppHistory::InitializeForNavigation(
HistoryItem& current,
const WebVector<WebHistoryItem>& back_entries,
const WebVector<WebHistoryItem>& forward_entries) {
DCHECK(entries_.IsEmpty());

// Construct |entries_|. Any back entries are inserted, then the current
// entry, then any forward entries.
entries_.ReserveCapacity(back_entries.size() + forward_entries.size() + 1);
for (const auto& entry : back_entries) {
entries_.emplace_back(
MakeGarbageCollected<AppHistoryEntry>(GetSupplementable(), entry));
}

current_index_ = back_entries.size();
entries_.emplace_back(
MakeGarbageCollected<AppHistoryEntry>(GetSupplementable(), &current));

for (const auto& entry : forward_entries) {
entries_.emplace_back(
MakeGarbageCollected<AppHistoryEntry>(GetSupplementable(), entry));
}
}

void AppHistory::CloneFromPrevious(AppHistory& previous) {
DCHECK(entries_.IsEmpty());
// We want to copy the backing state from |previous|, but we need to create
// new AppHistoryEntry objects because the previous ones are associated with
// the previous LocalDOMWindow.
entries_.ReserveCapacity(previous.entries_.size());
for (size_t i = 0; i < previous.entries_.size(); i++) {
entries_.emplace_back(MakeGarbageCollected<AppHistoryEntry>(
GetSupplementable(), previous.entries_[i]->GetItem()));
}
current_index_ = previous.current_index_;
}

void AppHistory::UpdateForNavigation(HistoryItem& item, WebFrameLoadType type) {
// A same-document navigation (e.g., a document.open()) in a newly created
// iframe will try to operate on an empty |entries_|. appHistory considers
// this a no-op.
if (entries_.IsEmpty())
return;

if (type == WebFrameLoadType::kBackForward) {
// If this is a same-document back/forward navigation, the new current_
// should already be present in entries_.
// We just need to update current_index_ to its index, so find it.
size_t new_current_index = 0;
for (; new_current_index < entries_.size(); new_current_index++) {
if (entries_[new_current_index]->key() == item.GetAppHistoryKey())
break;
}
DCHECK_LT(new_current_index, entries_.size());
current_index_ = new_current_index;
return;
}

if (type == WebFrameLoadType::kStandard) {
// For a new back/forward entry, truncate any forward entries and prepare to
// append.
current_index_++;
entries_.resize(current_index_ + 1);
}

// current_index_ is now correctly set (for type of
// WebFrameLoadType::kReplaceCurrentItem/kReload/kReloadBypassingCache, it
// didn't change). Create the new current entry.
entries_[current_index_] =
MakeGarbageCollected<AppHistoryEntry>(GetSupplementable(), &item);
void AppHistory::UpdateForCommit(HistoryItem* item) {
current_ = MakeGarbageCollected<AppHistoryEntry>(GetSupplementable(), item);
}

AppHistoryEntry* AppHistory::current() const {
// current_index_ is initialized to -1 and set >= 0 when entries_ is
// populated. It will still be negative if the appHistory of an initial empty
// document is accessed.
return current_index_ >= 0 && GetSupplementable()->GetFrame()
? entries_[current_index_]
: nullptr;
}

HeapVector<Member<AppHistoryEntry>> AppHistory::entries() {
return GetSupplementable()->GetFrame()
? entries_
: HeapVector<Member<AppHistoryEntry>>();
return GetSupplementable()->GetFrame() ? current_ : nullptr;
}

bool AppHistory::DispatchNavigateEvent(const KURL& url,
Expand Down Expand Up @@ -150,7 +71,7 @@ const AtomicString& AppHistory::InterfaceName() const {
void AppHistory::Trace(Visitor* visitor) const {
EventTargetWithInlineData::Trace(visitor);
Supplement<LocalDOMWindow>::Trace(visitor);
visitor->Trace(entries_);
visitor->Trace(current_);
}

} // namespace blink
Loading

0 comments on commit 4b0a605

Please sign in to comment.