Skip to content

Commit

Permalink
[PM] Split opener/embedder tracking.
Browse files Browse the repository at this point in the history 
It is possible for a page to have both an opener and an embedder, and
the existing code was built under the assumption that was not possible.

BUG=1199086

Change-Id: Icc7b6f2b1ebb3e3a53a5b48e0948ecc5c941b861
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2838089
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#879502}
  • Loading branch information
@chhamilton
chhamilton authored and Chromium LUCI CQ committed May 5, 2021
1 parent 42a87be commit adff1bb
Show file tree
Hide file tree
Showing 17 changed files with 264 additions and 119 deletions.
8 changes: 6 additions & 2 deletions chrome/browser/resources/discards/graph_doc.js
View file
Expand Up @@ -396,10 +396,14 @@ class PageNode extends GraphNode {

/** override */
get dashedLinkTargets() {
const targets = [];
if (this.page.openerFrameId) {
targets.push(this.page.openerFrameId);
}
if (this.page.embedderFrameId) {
return [this.page.embedderFrameId];
targets.push(this.page.embedderFrameId);
}
return [];
return targets;
}
}

Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/ui/webui/discards/discards.mojom
View file
Expand Up @@ -100,6 +100,9 @@ struct PageInfo {

url.mojom.Url main_frame_url;

// The id of the frame that opened this page via window.open, if any.
int64 opener_frame_id;

// The id of the frame that embedded this page, if any.
int64 embedder_frame_id;

Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/ui/webui/discards/graph_dump_impl.cc
View file
Expand Up @@ -291,6 +291,13 @@ void DiscardsGraphDumpImpl::OnBeforePageNodeRemoved(
RemoveNode(page_node);
}

void DiscardsGraphDumpImpl::OnOpenerFrameNodeChanged(
const performance_manager::PageNode* page_node,
const performance_manager::FrameNode* previous_opener) {
DCHECK(HasNode(page_node));
SendPageNotification(page_node, false);
}

void DiscardsGraphDumpImpl::OnEmbedderFrameNodeChanged(
const performance_manager::PageNode* page_node,
const performance_manager::FrameNode*,
Expand Down Expand Up @@ -505,6 +512,7 @@ void DiscardsGraphDumpImpl::SendPageNotification(

page_info->id = GetNodeId(page_node);
page_info->main_frame_url = page_node->GetMainFrameUrl();
page_info->opener_frame_id = GetNodeId(page_node->GetOpenerFrameNode());
page_info->embedder_frame_id = GetNodeId(page_node->GetEmbedderFrameNode());
page_info->description_json = ToJSON(
graph_->GetNodeDataDescriberRegistry()->DescribeNodeData(page_node));
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/ui/webui/discards/graph_dump_impl.h
View file
Expand Up @@ -113,6 +113,9 @@ class DiscardsGraphDumpImpl : public discards::mojom::GraphDump,
void OnPageNodeAdded(const performance_manager::PageNode* page_node) override;
void OnBeforePageNodeRemoved(
const performance_manager::PageNode* page_node) override;
void OnOpenerFrameNodeChanged(
const performance_manager::PageNode* page_node,
const performance_manager::FrameNode* previous_opener) override;
void OnEmbedderFrameNodeChanged(
const performance_manager::PageNode* page_node,
const performance_manager::FrameNode* previous_embedder,
Expand Down
85 changes: 70 additions & 15 deletions components/performance_manager/graph/frame_node_impl.cc
View file
Expand Up @@ -53,6 +53,7 @@ FrameNodeImpl::FrameNodeImpl(ProcessNodeImpl* process_node,
FrameNodeImpl::~FrameNodeImpl() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(child_worker_nodes_.empty());
DCHECK(opened_page_nodes_.empty());
DCHECK(embedded_page_nodes_.empty());
DCHECK(!execution_context_);
}
Expand Down Expand Up @@ -166,6 +167,11 @@ const base::flat_set<FrameNodeImpl*>& FrameNodeImpl::child_frame_nodes() const {
return child_frame_nodes_;
}

const base::flat_set<PageNodeImpl*>& FrameNodeImpl::opened_page_nodes() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return opened_page_nodes_;
}

const base::flat_set<PageNodeImpl*>& FrameNodeImpl::embedded_page_nodes()
const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand Down Expand Up @@ -367,6 +373,28 @@ base::WeakPtr<FrameNodeImpl> FrameNodeImpl::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}

void FrameNodeImpl::AddOpenedPage(base::PassKey<PageNodeImpl>,
PageNodeImpl* page_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(page_node);
DCHECK_NE(page_node_, page_node);
DCHECK(graph()->NodeInGraph(page_node));
DCHECK_EQ(this, page_node->opener_frame_node());
bool inserted = opened_page_nodes_.insert(page_node).second;
DCHECK(inserted);
}

void FrameNodeImpl::RemoveOpenedPage(base::PassKey<PageNodeImpl>,
PageNodeImpl* page_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(page_node);
DCHECK_NE(page_node_, page_node);
DCHECK(graph()->NodeInGraph(page_node));
DCHECK_EQ(this, page_node->opener_frame_node());
size_t removed = opened_page_nodes_.erase(page_node);
DCHECK_EQ(1u, removed);
}

void FrameNodeImpl::AddEmbeddedPage(base::PassKey<PageNodeImpl>,
PageNodeImpl* page_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand Down Expand Up @@ -442,6 +470,22 @@ const base::flat_set<const FrameNode*> FrameNodeImpl::GetChildFrameNodes()
return UpcastNodeSet<FrameNode>(child_frame_nodes());
}

bool FrameNodeImpl::VisitOpenedPageNodes(const PageNodeVisitor& visitor) const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto* page_impl : opened_page_nodes()) {
const PageNode* page = page_impl;
if (!visitor.Run(page))
return false;
}
return true;
}

const base::flat_set<const PageNode*> FrameNodeImpl::GetOpenedPageNodes()
const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return UpcastNodeSet<PageNode>(opened_page_nodes());
}

bool FrameNodeImpl::VisitEmbeddedPageNodes(
const PageNodeVisitor& visitor) const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand Down Expand Up @@ -592,8 +636,7 @@ void FrameNodeImpl::OnBeforeLeavingGraph() {

DCHECK(child_frame_nodes_.empty());

// Sever embedder relationships.
SeverEmbeddedPagesAndMaybeReparent();
SeverPageRelationshipsAndMaybeReparent();

// Leave the page.
DCHECK(graph()->NodeInGraph(page_node_));
Expand All @@ -619,32 +662,44 @@ void FrameNodeImpl::RemoveNodeAttachedData() {
execution_context_.reset();
}

void FrameNodeImpl::SeverEmbeddedPagesAndMaybeReparent() {
void FrameNodeImpl::SeverPageRelationshipsAndMaybeReparent() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

// Copy |embedded_page_nodes_| as we'll be modifying it in this loop: when we
// call PageNodeImpl::(Set|Clear)EmbedderFrameNodeAndEmbeddingType() this will
// call back into this frame node and call RemoveEmbeddedPage().
base::flat_set<PageNodeImpl*> embedded_nodes = embedded_page_nodes_;
for (auto* embedded_node : embedded_nodes) {
auto embedding_type = embedded_node->embedding_type();
// Be careful when iterating: when we call
// PageNodeImpl::(Set|Clear)(Opener|Embedder)FrameNode() this will call
// back into this frame node and call Remove(Opened|Embedded)Page(), which
// modifies |opened_page_nodes_| and |embedded_page_nodes_|.
//
// We also reparent related pages to this frame's parent to maintain the
// relationship between the distinct frame trees for bookkeeping. For the
// relationship to be finally severed one of the frame trees must completely
// disappear, or it must be explicitly severed (this can happen with
// portals).
while (!opened_page_nodes_.empty()) {
auto* opened_node = *opened_page_nodes_.begin();
if (parent_frame_node_) {
opened_node->SetOpenerFrameNode(parent_frame_node_);
} else {
opened_node->ClearOpenerFrameNode();
}
DCHECK(!base::Contains(opened_page_nodes_, opened_node));
}

// Reparent embedded pages to this frame's parent to maintain the
// relationship between the frame trees for bookkeeping. For the
// relationship to be finally severed one of the frame trees must completely
// disappear, or it must be explicitly severed (this can happen with
// portals).
while (!embedded_page_nodes_.empty()) {
auto* embedded_node = *embedded_page_nodes_.begin();
auto embedding_type = embedded_node->embedding_type();
if (parent_frame_node_) {
embedded_node->SetEmbedderFrameNodeAndEmbeddingType(parent_frame_node_,
embedding_type);
} else {
// There's no new parent, so simply clear the embedder.
embedded_node->ClearEmbedderFrameNodeAndEmbeddingType();
}
DCHECK(!base::Contains(embedded_page_nodes_, embedded_node));
}

// Expect each page node to have called RemoveEmbeddedPage(), and for this to
// now be empty.
DCHECK(opened_page_nodes_.empty());
DCHECK(embedded_page_nodes_.empty());
}

Expand Down
26 changes: 19 additions & 7 deletions components/performance_manager/graph/frame_node_impl.h
View file
Expand Up @@ -108,6 +108,7 @@ class FrameNodeImpl

// Getters for non-const properties. These are not thread safe.
const base::flat_set<FrameNodeImpl*>& child_frame_nodes() const;
const base::flat_set<PageNodeImpl*>& opened_page_nodes() const;
const base::flat_set<PageNodeImpl*>& embedded_page_nodes() const;
LifecycleState lifecycle_state() const;
bool has_nonempty_beforeunload() const;
Expand Down Expand Up @@ -145,12 +146,18 @@ class FrameNodeImpl
base::WeakPtr<FrameNodeImpl> GetWeakPtrOnUIThread();
base::WeakPtr<FrameNodeImpl> GetWeakPtr();

void SeverEmbeddedPagesAndMaybeReparentForTesting() {
SeverEmbeddedPagesAndMaybeReparent();
void SeverPageRelationshipsAndMaybeReparentForTesting() {
SeverPageRelationshipsAndMaybeReparent();
}

// Implementation details below this point.

// Invoked by opened pages when this frame is set/cleared as their opener.
// See PageNodeImpl::(Set|Clear)OpenerFrameNode.
void AddOpenedPage(base::PassKey<PageNodeImpl> key, PageNodeImpl* page_node);
void RemoveOpenedPage(base::PassKey<PageNodeImpl> key,
PageNodeImpl* page_node);

// Invoked by embedded pages when this frame is set/cleared as their embedder.
// See PageNodeImpl::(Set|Clear)EmbedderFrameNodeAndEmbeddingType.
void AddEmbeddedPage(base::PassKey<PageNodeImpl> key,
Expand Down Expand Up @@ -180,6 +187,8 @@ class FrameNodeImpl
int32_t GetSiteInstanceId() const override;
bool VisitChildFrameNodes(const FrameNodeVisitor& visitor) const override;
const base::flat_set<const FrameNode*> GetChildFrameNodes() const override;
bool VisitOpenedPageNodes(const PageNodeVisitor& visitor) const override;
const base::flat_set<const PageNode*> GetOpenedPageNodes() const override;
bool VisitEmbeddedPageNodes(const PageNodeVisitor& visitor) const override;
const base::flat_set<const PageNode*> GetEmbeddedPageNodes() const override;
LifecycleState GetLifecycleState() const override;
Expand Down Expand Up @@ -237,11 +246,11 @@ class FrameNodeImpl
void OnBeforeLeavingGraph() override;
void RemoveNodeAttachedData() override;

// Helper function to sever all embedded page relationships. This is called
// before destroying the frame node in "OnBeforeLeavingGraph". Note that this
// will reparent embedded pages to this frame's parent so that tracking is
// maintained.
void SeverEmbeddedPagesAndMaybeReparent();
// Helper function to sever all opened/embedded page relationships. This is
// called before destroying the frame node in "OnBeforeLeavingGraph". Note
// that this will reparent embedded pages to this frame's parent so that
// tracking is maintained.
void SeverPageRelationshipsAndMaybeReparent();

// This is not quite the same as GetMainFrame, because there can be multiple
// main frames while the main frame is navigating. This explicitly walks up
Expand Down Expand Up @@ -287,6 +296,9 @@ class FrameNodeImpl

base::flat_set<FrameNodeImpl*> child_frame_nodes_;

// The set of pages that have been opened by this frame.
base::flat_set<PageNodeImpl*> opened_page_nodes_;

// The set of pages that have been embedded by this frame.
base::flat_set<PageNodeImpl*> embedded_page_nodes_;

Expand Down

0 comments on commit adff1bb

Please sign in to comment.