Limit the maximum length of frame unique names.
The unique name is a semi-stable identifier used to identify the target
frame for back/forward and session restore. The original unique name
generation algorithm included the browsing context name: unfortunately,
certain sites use window.name to transport large amounts of data,
causing session restore data to balloon in size.
The original plan was to strictly limit the length of unique names;
however, ensuring backwards compatibility was complex and difficult to
understand. Instead, this patch enforces a weaker guarantee: if a frame
provides a hint for the unique name that is over 80 characters, hash
the requested name and use the result as if it were the requested name
instead. It's still possible to get fairly long names with deeply
nested frames, but this should be a large improvement over the current
situation with no limit at all.
Note that even the simpler version of this algorithm does not result in
perfect backwards compatibility: a malicious page can intentionally
pick browsing context names that only collide once the name is hashed.
Since this only affects the page itself, the algorithm retains the
current best effort collision avoidance strategy of picking a name that
is unlikely to collide, without guaranteeing full collision resistance.
Browsing a small assortment of control pages shows that unique name
length is reduced from an average of ~1260 characters to 70 characters.
Note that this metric was originally implemented incorrectly: for the
purpose of comparison, the new metric was recorded in the exact same
way. Actual numbers in the field are probably somewhat lower than this.
Bug: 626202, 645123
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I63c481feaf708c5e0d4087dafc8fcbf59b9091a6
Reviewed-on: https://chromium-review.googlesource.com/579031
Reviewed-by: Mark Pearson <[email protected]>
Reviewed-by: Charlie Reis <[email protected]>
Cr-Commit-Position: refs/heads/master@{#493153}diff --git a/content/browser/frame_host/frame_tree_node.cc b/content/browser/frame_host/frame_tree_node.cc
index c4682bd5..c2f3a0ec 100644
--- a/content/browser/frame_host/frame_tree_node.cc
+++ b/content/browser/frame_host/frame_tree_node.cc
@@ -4,6 +4,8 @@
#include "content/browser/frame_host/frame_tree_node.h"
+#include <math.h>
+
#include <queue>
#include <utility>
@@ -13,6 +15,7 @@
#include "base/metrics/histogram_macros.h"
#include "base/profiler/scoped_tracker.h"
#include "base/stl_util.h"
+#include "base/strings/string_util.h"
#include "content/browser/frame_host/frame_tree.h"
#include "content/browser/frame_host/navigation_request.h"
#include "content/browser/frame_host/navigator.h"
@@ -42,8 +45,44 @@
const double kLoadingProgressMinimum = 0.1;
const double kLoadingProgressDone = 1.0;
-void RecordUniqueNameLength(size_t length) {
- UMA_HISTOGRAM_COUNTS("SessionRestore.FrameUniqueNameLength", length);
+void RecordUniqueNameSize(FrameTreeNode* node) {
+ const auto& unique_name = node->current_replication_state().unique_name;
+
+ // Don't record numbers for the root node, which always has an empty unique
+ // name.
+ if (!node->parent()) {
+ DCHECK(unique_name.empty());
+ return;
+ }
+
+ // The original requested name is derived from the browsing context name and
+ // is essentially unbounded in size...
+ UMA_HISTOGRAM_COUNTS_1M(
+ "SessionRestore.FrameUniqueNameOriginalRequestedNameSize",
+ node->current_replication_state().name.size());
+ // If the name is a frame path, attempt to normalize the statistics based on
+ // the number of frames in the frame path.
+ if (base::StartsWith(unique_name, "<!--framePath //",
+ base::CompareCase::SENSITIVE)) {
+ size_t depth = 1;
+ while (node->parent()) {
+ ++depth;
+ node = node->parent();
+ }
+ // The max possible size of a unique name is 80 characters, so the expected
+ // size per component shouldn't be much more than that.
+ UMA_HISTOGRAM_COUNTS_100(
+ "SessionRestore.FrameUniqueNameWithFramePathSizePerComponent",
+ round(unique_name.size() / static_cast<float>(depth)));
+ // Blink allows a maximum of ~1024 subframes in a document, so this should
+ // be less than (80 character name + 1 character delimiter) * 1024.
+ UMA_HISTOGRAM_COUNTS_100000(
+ "SessionRestore.FrameUniqueNameWithFramePathSize", unique_name.size());
+ } else {
+ UMA_HISTOGRAM_COUNTS_100(
+ "SessionRestore.FrameUniqueNameFromRequestedNameSize",
+ unique_name.size());
+ }
}
} // namespace
@@ -124,7 +163,7 @@
std::make_pair(frame_tree_node_id_, this));
CHECK(result.second);
- RecordUniqueNameLength(unique_name.size());
+ RecordUniqueNameSize(this);
// Note: this should always be done last in the constructor.
blame_context_.Initialize();
@@ -286,7 +325,10 @@
DCHECK(unique_name.empty());
}
- RecordUniqueNameLength(unique_name.size());
+ // Note the unique name should only be able to change before the first real
+ // load is committed, but that's not strongly enforced here.
+ if (unique_name != replication_state_.unique_name)
+ RecordUniqueNameSize(this);
render_manager_.OnDidUpdateName(name, unique_name);
replication_state_.name = name;
replication_state_.unique_name = unique_name;
