Reland "PM: Elminate CuID in favor of Node* in private IPCs to graph nodes."
This is a reland of f5b288f95873153befd9410d0fb9d73561084771
Original change's description:
> PM: Elminate CuID in favor of Node* in private IPCs to graph nodes.
>
> Separate node teardown from destruction by making BeforeDestroyed
> virtual and overriding it in concrete node classes.
> Tighten up the contracts and enforce a stricter data invariant.
> Fix tests and test fixtures to destroy frames before pages.
> Add a SequenceChecker to the graph.
>
> Bug: 910288
> Change-Id: I2e6bd857462b8576212001df8128f273ba4f4d74
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1532342
> Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#642613}
Bug: 910288
Change-Id: Ib2f465d2f5e743a7525867bb2e4d78bfe6a19fef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1531034
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642949}diff --git a/chrome/browser/performance_manager/graph/frame_node_impl.cc b/chrome/browser/performance_manager/graph/frame_node_impl.cc
index 90c7eec..9822d54 100644
--- a/chrome/browser/performance_manager/graph/frame_node_impl.cc
+++ b/chrome/browser/performance_manager/graph/frame_node_impl.cc
@@ -26,54 +26,38 @@
FrameNodeImpl::~FrameNodeImpl() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- if (parent_frame_node_)
- parent_frame_node_->RemoveChildFrame(this);
- if (page_node_)
- page_node_->RemoveFrameImpl(this);
- if (process_node_)
- process_node_->RemoveFrame(this);
- for (auto* child_frame : child_frame_nodes_)
- child_frame->RemoveParentFrame(this);
}
-void FrameNodeImpl::SetProcess(
- const resource_coordinator::CoordinationUnitID& cu_id) {
+void FrameNodeImpl::SetProcess(ProcessNodeImpl* process_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- ProcessNodeImpl* process_node = ProcessNodeImpl::GetNodeByID(graph_, cu_id);
- if (!process_node)
- return;
+ DCHECK(NodeInGraph(process_node));
DCHECK(!process_node_);
process_node_ = process_node;
process_node->AddFrame(this);
}
-void FrameNodeImpl::AddChildFrame(
- const resource_coordinator::CoordinationUnitID& cu_id) {
+void FrameNodeImpl::AddChildFrame(FrameNodeImpl* child_frame_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- DCHECK(cu_id != id());
- FrameNodeImpl* frame_node = FrameNodeImpl::GetNodeByID(graph_, cu_id);
- if (!frame_node)
- return;
- if (HasFrameNodeInAncestors(frame_node) ||
- frame_node->HasFrameNodeInDescendants(this)) {
- DCHECK(false) << "Cyclic reference in frame coordination units detected!";
- return;
- }
- if (AddChildFrameImpl(frame_node)) {
- frame_node->AddParentFrame(this);
- }
+ DCHECK(child_frame_node);
+ DCHECK_NE(this, child_frame_node);
+ DCHECK(NodeInGraph(child_frame_node));
+ DCHECK(!HasFrameNodeInAncestors(child_frame_node) &&
+ !child_frame_node->HasFrameNodeInDescendants(this));
+
+ bool inserted = child_frame_nodes_.insert(child_frame_node).second;
+ DCHECK(inserted);
+ child_frame_node->AddParentFrame(this);
}
-void FrameNodeImpl::RemoveChildFrame(
- const resource_coordinator::CoordinationUnitID& cu_id) {
+void FrameNodeImpl::RemoveChildFrame(FrameNodeImpl* child_frame_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- DCHECK(cu_id != id());
- FrameNodeImpl* frame_node = FrameNodeImpl::GetNodeByID(graph_, cu_id);
- if (!frame_node)
- return;
- if (RemoveChildFrame(frame_node)) {
- frame_node->RemoveParentFrame(this);
- }
+ DCHECK(child_frame_node);
+ DCHECK_NE(this, child_frame_node);
+ DCHECK(NodeInGraph(child_frame_node));
+
+ size_t removed = child_frame_nodes_.erase(child_frame_node);
+ DCHECK_EQ(1u, removed);
+ child_frame_node->RemoveParentFrame(this);
}
void FrameNodeImpl::SetNetworkAlmostIdle(bool network_almost_idle) {
@@ -193,6 +177,20 @@
}
}
+void FrameNodeImpl::BeforeDestroyed() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ NodeBase::BeforeDestroyed();
+
+ if (parent_frame_node_)
+ parent_frame_node_->RemoveChildFrame(this);
+ if (page_node_)
+ page_node_->RemoveFrame(this);
+ if (process_node_)
+ process_node_->RemoveFrame(this);
+ for (auto* child_frame : child_frame_nodes_)
+ child_frame->RemoveParentFrame(this);
+}
+
void FrameNodeImpl::OnEventReceived(resource_coordinator::mojom::Event event) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto& observer : observers())
@@ -229,27 +227,16 @@
void FrameNodeImpl::AddParentFrame(FrameNodeImpl* parent_frame_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ DCHECK_EQ(nullptr, parent_frame_node_);
parent_frame_node_ = parent_frame_node;
}
-bool FrameNodeImpl::AddChildFrameImpl(FrameNodeImpl* child_frame_node) {
- DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- return child_frame_nodes_.count(child_frame_node)
- ? false
- : child_frame_nodes_.insert(child_frame_node).second;
-}
-
void FrameNodeImpl::RemoveParentFrame(FrameNodeImpl* parent_frame_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(parent_frame_node_ == parent_frame_node);
parent_frame_node_ = nullptr;
}
-bool FrameNodeImpl::RemoveChildFrame(FrameNodeImpl* child_frame_node) {
- DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- return child_frame_nodes_.erase(child_frame_node) > 0;
-}
-
void FrameNodeImpl::AddPageNode(PageNodeImpl* page_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!page_node_);
diff --git a/chrome/browser/performance_manager/graph/frame_node_impl.h b/chrome/browser/performance_manager/graph/frame_node_impl.h
index 9fd8b90..48182c7 100644
--- a/chrome/browser/performance_manager/graph/frame_node_impl.h
+++ b/chrome/browser/performance_manager/graph/frame_node_impl.h
@@ -43,9 +43,9 @@
resource_coordinator::mojom::InterventionPolicy policy) override;
void OnNonPersistentNotificationCreated() override;
- void SetProcess(const resource_coordinator::CoordinationUnitID& cu_id);
- void AddChildFrame(const resource_coordinator::CoordinationUnitID& cu_id);
- void RemoveChildFrame(const resource_coordinator::CoordinationUnitID& cu_id);
+ void SetProcess(ProcessNodeImpl* process_node);
+ void AddChildFrame(FrameNodeImpl* frame_node);
+ void RemoveChildFrame(FrameNodeImpl* frame_node);
FrameNodeImpl* GetParentFrameNode() const;
PageNodeImpl* GetPageNode() const;
@@ -74,6 +74,8 @@
friend class PageNodeImpl;
friend class ProcessNodeImpl;
+ void BeforeDestroyed() override;
+
// CoordinationUnitInterface implementation.
void OnEventReceived(resource_coordinator::mojom::Event event) override;
void OnPropertyChanged(
@@ -89,7 +91,6 @@
void AddParentFrame(FrameNodeImpl* parent_frame_node);
bool AddChildFrameImpl(FrameNodeImpl* child_frame_node);
void RemoveParentFrame(FrameNodeImpl* parent_frame_node);
- bool RemoveChildFrame(FrameNodeImpl* child_frame_node);
void AddPageNode(PageNodeImpl* page_node);
void AddProcessNode(ProcessNodeImpl* process_node);
void RemovePageNode(PageNodeImpl* page_node);
diff --git a/chrome/browser/performance_manager/graph/frame_node_impl_unittest.cc b/chrome/browser/performance_manager/graph/frame_node_impl_unittest.cc
index 9eecff3..6ebab27 100644
--- a/chrome/browser/performance_manager/graph/frame_node_impl_unittest.cc
+++ b/chrome/browser/performance_manager/graph/frame_node_impl_unittest.cc
@@ -34,8 +34,6 @@
base::SimpleTestTickClock clock_;
};
-using FrameNodeImplDeathTest = FrameNodeImplTest;
-
} // namespace
TEST_F(FrameNodeImplTest, AddChildFrameBasic) {
@@ -43,41 +41,14 @@
auto frame2_node = CreateNode<FrameNodeImpl>();
auto frame3_node = CreateNode<FrameNodeImpl>();
- frame1_node->AddChildFrame(frame2_node->id());
- frame1_node->AddChildFrame(frame3_node->id());
+ frame1_node->AddChildFrame(frame2_node.get());
+ frame1_node->AddChildFrame(frame3_node.get());
EXPECT_EQ(nullptr, frame1_node->GetParentFrameNode());
EXPECT_EQ(2u, frame1_node->child_frame_nodes_for_testing().size());
EXPECT_EQ(frame1_node.get(), frame2_node->GetParentFrameNode());
EXPECT_EQ(frame1_node.get(), frame3_node->GetParentFrameNode());
}
-TEST_F(FrameNodeImplDeathTest, AddChildFrameOnCyclicReference) {
- ::testing::FLAGS_gtest_death_test_style = "threadsafe";
-
- auto frame1_node = CreateNode<FrameNodeImpl>();
- auto frame2_node = CreateNode<FrameNodeImpl>();
- auto frame3_node = CreateNode<FrameNodeImpl>();
-
- frame1_node->AddChildFrame(frame2_node->id());
- frame2_node->AddChildFrame(frame3_node->id());
-// |frame3_node| can't add |frame1_node| because |frame1_node| is an ancestor of
-// |frame3_node|, and this will hit a DCHECK because of cyclic reference.
-#if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)
- EXPECT_DEATH_IF_SUPPORTED(frame3_node->AddChildFrame(frame1_node->id()), "");
-#else
- frame3_node->AddChildFrame(frame1_node->id());
-#endif // !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)
-
- EXPECT_EQ(1u, frame1_node->child_frame_nodes_for_testing().count(
- frame2_node.get()));
- EXPECT_EQ(1u, frame2_node->child_frame_nodes_for_testing().count(
- frame3_node.get()));
- // |frame1_node| was not added successfully because |frame1_node| is one of
- // the ancestors of |frame3_node|.
- EXPECT_EQ(0u, frame3_node->child_frame_nodes_for_testing().count(
- frame1_node.get()));
-}
-
TEST_F(FrameNodeImplTest, RemoveChildFrame) {
auto parent_frame_node = CreateNode<FrameNodeImpl>();
auto child_frame_node = CreateNode<FrameNodeImpl>();
@@ -88,7 +59,7 @@
EXPECT_EQ(0u, child_frame_node->child_frame_nodes_for_testing().size());
EXPECT_TRUE(!child_frame_node->GetParentFrameNode());
- parent_frame_node->AddChildFrame(child_frame_node->id());
+ parent_frame_node->AddChildFrame(child_frame_node.get());
// Ensure correct Parent-child relationships have been established.
EXPECT_EQ(1u, parent_frame_node->child_frame_nodes_for_testing().size());
@@ -96,7 +67,7 @@
EXPECT_EQ(0u, child_frame_node->child_frame_nodes_for_testing().size());
EXPECT_EQ(parent_frame_node.get(), child_frame_node->GetParentFrameNode());
- parent_frame_node->RemoveChildFrame(child_frame_node->id());
+ parent_frame_node->RemoveChildFrame(child_frame_node.get());
// Parent-child relationships should no longer exist.
EXPECT_EQ(0u, parent_frame_node->child_frame_nodes_for_testing().size());
diff --git a/chrome/browser/performance_manager/graph/graph.cc b/chrome/browser/performance_manager/graph/graph.cc
index dc7efa5..7a9a8879 100644
--- a/chrome/browser/performance_manager/graph/graph.cc
+++ b/chrome/browser/performance_manager/graph/graph.cc
@@ -23,9 +23,13 @@
namespace performance_manager {
-Graph::Graph() = default;
+Graph::Graph() {
+ DETACH_FROM_SEQUENCE(sequence_checker_);
+}
Graph::~Graph() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
// Because the graph has ownership of the CUs, and because the process CUs
// unregister on destruction, there is reentrancy to this class on
// destruction. The order of operations here is optimized to minimize the work
@@ -42,11 +46,13 @@
}
void Graph::RegisterObserver(std::unique_ptr<GraphObserver> observer) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
observer->set_node_graph(this);
observers_.push_back(std::move(observer));
}
void Graph::OnNodeAdded(NodeBase* node) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto& observer : observers_) {
if (observer->ShouldObserve(node)) {
node->AddObserver(observer.get());
@@ -56,10 +62,12 @@
}
void Graph::OnBeforeNodeRemoved(NodeBase* node) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
node->BeforeDestroyed();
}
SystemNodeImpl* Graph::FindOrCreateSystemNode() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!system_node_) {
// Create the singleton SystemCU instance. Ownership is taken by the graph.
resource_coordinator::CoordinationUnitID id(
@@ -74,6 +82,7 @@
NodeBase* Graph::GetNodeByID(
const resource_coordinator::CoordinationUnitID cu_id) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
const auto& it = nodes_.find(cu_id);
if (it == nodes_.end())
return nullptr;
@@ -81,6 +90,7 @@
}
ProcessNodeImpl* Graph::GetProcessNodeByPid(base::ProcessId pid) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
auto it = processes_by_pid_.find(pid);
if (it == processes_by_pid_.end())
return nullptr;
@@ -102,6 +112,7 @@
size_t Graph::GetNodeAttachedDataCountForTesting(NodeBase* node,
const void* key) const {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!node && !key)
return node_attached_data_map_.size();
@@ -118,12 +129,14 @@
}
void Graph::AddNewNode(NodeBase* new_node) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
auto it = nodes_.emplace(new_node->id(), new_node);
DCHECK(it.second); // Inserted successfully
OnNodeAdded(new_node);
}
void Graph::RemoveNode(NodeBase* node) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
OnBeforeNodeRemoved(node);
// Remove any node attached data affiliated with this node.
@@ -140,6 +153,7 @@
void Graph::BeforeProcessPidChange(ProcessNodeImpl* process,
base::ProcessId new_pid) {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// On Windows, PIDs are aggressively reused, and because not all process
// creation/death notifications are synchronized, it's possible for more than
// one CU to have the same PID. To handle this, the second and subsequent
@@ -156,6 +170,7 @@
template <typename CUType>
std::vector<CUType*> Graph::GetAllNodesOfType() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
const auto type = CUType::Type();
std::vector<CUType*> ret;
for (const auto& el : nodes_) {
diff --git a/chrome/browser/performance_manager/graph/graph.h b/chrome/browser/performance_manager/graph/graph.h
index c960747..ac68786 100644
--- a/chrome/browser/performance_manager/graph/graph.h
+++ b/chrome/browser/performance_manager/graph/graph.h
@@ -15,6 +15,7 @@
#include "base/macros.h"
#include "base/process/process_handle.h"
+#include "base/sequence_checker.h"
#include "chrome/browser/performance_manager/graph/node_attached_data.h"
#include "services/metrics/public/cpp/mojo_ukm_recorder.h"
#include "services/metrics/public/cpp/ukm_recorder.h"
@@ -97,8 +98,7 @@
std::map<NodeAttachedDataKey, std::unique_ptr<NodeAttachedData>>;
NodeAttachedDataMap node_attached_data_map_;
- static void Create();
-
+ SEQUENCE_CHECKER(sequence_checker_);
DISALLOW_COPY_AND_ASSIGN(Graph);
};
diff --git a/chrome/browser/performance_manager/graph/mock_graphs.cc b/chrome/browser/performance_manager/graph/mock_graphs.cc
index 751a36a..e19ab94 100644
--- a/chrome/browser/performance_manager/graph/mock_graphs.cc
+++ b/chrome/browser/performance_manager/graph/mock_graphs.cc
@@ -19,32 +19,38 @@
MockSinglePageInSingleProcessGraph::MockSinglePageInSingleProcessGraph(
Graph* graph)
: system(TestNodeWrapper<SystemNodeImpl>::Create(graph)),
- frame(TestNodeWrapper<FrameNodeImpl>::Create(graph)),
process(TestNodeWrapper<ProcessNodeImpl>::Create(graph)),
- page(TestNodeWrapper<PageNodeImpl>::Create(graph)) {
+ page(TestNodeWrapper<PageNodeImpl>::Create(graph)),
+ frame(TestNodeWrapper<FrameNodeImpl>::Create(graph)) {
frame->SetAllInterventionPoliciesForTesting(
resource_coordinator::mojom::InterventionPolicy::kDefault);
- page->AddFrame(frame->id());
- frame->SetProcess(process->id());
+ page->AddFrame(frame.get());
+ frame->SetProcess(process.get());
process->SetPID(1);
}
-MockSinglePageInSingleProcessGraph::~MockSinglePageInSingleProcessGraph() =
- default;
+MockSinglePageInSingleProcessGraph::~MockSinglePageInSingleProcessGraph() {
+ // Make sure frame nodes are torn down before pages.
+ frame.reset();
+ page.reset();
+}
MockMultiplePagesInSingleProcessGraph::MockMultiplePagesInSingleProcessGraph(
Graph* graph)
: MockSinglePageInSingleProcessGraph(graph),
- other_frame(TestNodeWrapper<FrameNodeImpl>::Create(graph)),
- other_page(TestNodeWrapper<PageNodeImpl>::Create(graph)) {
+ other_page(TestNodeWrapper<PageNodeImpl>::Create(graph)),
+ other_frame(TestNodeWrapper<FrameNodeImpl>::Create(graph)) {
other_frame->SetAllInterventionPoliciesForTesting(
resource_coordinator::mojom::InterventionPolicy::kDefault);
- other_page->AddFrame(other_frame->id());
- other_frame->SetProcess(process->id());
+ other_page->AddFrame(other_frame.get());
+ other_frame->SetProcess(process.get());
}
MockMultiplePagesInSingleProcessGraph::
- ~MockMultiplePagesInSingleProcessGraph() = default;
+ ~MockMultiplePagesInSingleProcessGraph() {
+ other_frame.reset();
+ other_page.reset();
+}
MockSinglePageWithMultipleProcessesGraph::
MockSinglePageWithMultipleProcessesGraph(Graph* graph)
@@ -53,9 +59,9 @@
other_process(TestNodeWrapper<ProcessNodeImpl>::Create(graph)) {
child_frame->SetAllInterventionPoliciesForTesting(
resource_coordinator::mojom::InterventionPolicy::kDefault);
- frame->AddChildFrame(child_frame->id());
- page->AddFrame(child_frame->id());
- child_frame->SetProcess(other_process->id());
+ frame->AddChildFrame(child_frame.get());
+ page->AddFrame(child_frame.get());
+ child_frame->SetProcess(other_process.get());
other_process->SetPID(2);
}
@@ -69,9 +75,9 @@
other_process(TestNodeWrapper<ProcessNodeImpl>::Create(graph)) {
child_frame->SetAllInterventionPoliciesForTesting(
resource_coordinator::mojom::InterventionPolicy::kDefault);
- other_frame->AddChildFrame(child_frame->id());
- other_page->AddFrame(child_frame->id());
- child_frame->SetProcess(other_process->id());
+ other_frame->AddChildFrame(child_frame.get());
+ other_page->AddFrame(child_frame.get());
+ child_frame->SetProcess(other_process.get());
other_process->SetPID(2);
}
diff --git a/chrome/browser/performance_manager/graph/mock_graphs.h b/chrome/browser/performance_manager/graph/mock_graphs.h
index 2a7e0fa..9937128 100644
--- a/chrome/browser/performance_manager/graph/mock_graphs.h
+++ b/chrome/browser/performance_manager/graph/mock_graphs.h
@@ -30,9 +30,9 @@
explicit MockSinglePageInSingleProcessGraph(Graph* graph);
~MockSinglePageInSingleProcessGraph();
TestNodeWrapper<SystemNodeImpl> system;
- TestNodeWrapper<FrameNodeImpl> frame;
TestNodeWrapper<ProcessNodeImpl> process;
TestNodeWrapper<PageNodeImpl> page;
+ TestNodeWrapper<FrameNodeImpl> frame;
};
// The following coordination unit graph topology is created to emulate a
@@ -52,8 +52,8 @@
: public MockSinglePageInSingleProcessGraph {
explicit MockMultiplePagesInSingleProcessGraph(Graph* graph);
~MockMultiplePagesInSingleProcessGraph();
- TestNodeWrapper<FrameNodeImpl> other_frame;
TestNodeWrapper<PageNodeImpl> other_page;
+ TestNodeWrapper<FrameNodeImpl> other_frame;
};
// The following coordination unit graph topology is created to emulate a
diff --git a/chrome/browser/performance_manager/graph/node_attached_data_unittest.cc b/chrome/browser/performance_manager/graph/node_attached_data_unittest.cc
index 6c2925a..5b0b3de 100644
--- a/chrome/browser/performance_manager/graph/node_attached_data_unittest.cc
+++ b/chrome/browser/performance_manager/graph/node_attached_data_unittest.cc
@@ -234,6 +234,7 @@
// Release the page node and expect the node attached data to have been
// cleaned up.
+ mock_graph.frame.reset();
mock_graph.page.reset();
EXPECT_EQ(1u, graph()->GetNodeAttachedDataCountForTesting(nullptr, nullptr));
EXPECT_EQ(0u,
diff --git a/chrome/browser/performance_manager/graph/node_base.cc b/chrome/browser/performance_manager/graph/node_base.cc
index 62d650dd..c952e038 100644
--- a/chrome/browser/performance_manager/graph/node_base.cc
+++ b/chrome/browser/performance_manager/graph/node_base.cc
@@ -59,6 +59,10 @@
return default_value;
}
+bool NodeBase::NodeInGraph(const NodeBase* other_node) const {
+ return graph_->GetNodeByID(other_node->id()) == other_node;
+}
+
void NodeBase::OnEventReceived(resource_coordinator::mojom::Event event) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto& observer : observers())
diff --git a/chrome/browser/performance_manager/graph/node_base.h b/chrome/browser/performance_manager/graph/node_base.h
index bcd9a9d..df44b0b0 100644
--- a/chrome/browser/performance_manager/graph/node_base.h
+++ b/chrome/browser/performance_manager/graph/node_base.h
@@ -33,7 +33,7 @@
NodeBase(const resource_coordinator::CoordinationUnitID& id, Graph* graph);
virtual ~NodeBase();
- void BeforeDestroyed();
+ virtual void BeforeDestroyed();
void AddObserver(GraphObserver* observer);
void RemoveObserver(GraphObserver* observer);
bool GetProperty(
@@ -61,6 +61,9 @@
}
protected:
+ // Returns true if |other_node| is in the same graph.
+ bool NodeInGraph(const NodeBase* other_node) const;
+
// Helper function for setting a property, and notifying observers if the
// value has changed.
template <typename NodeType,
diff --git a/chrome/browser/performance_manager/graph/page_node_impl.cc b/chrome/browser/performance_manager/graph/page_node_impl.cc
index 6a8f725..50230e1 100644
--- a/chrome/browser/performance_manager/graph/page_node_impl.cc
+++ b/chrome/browser/performance_manager/graph/page_node_impl.cc
@@ -41,30 +41,45 @@
PageNodeImpl::~PageNodeImpl() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- for (auto* child_frame : frame_nodes_)
- child_frame->RemovePageNode(this);
}
-void PageNodeImpl::AddFrame(
- const resource_coordinator::CoordinationUnitID& cu_id) {
+void PageNodeImpl::AddFrame(FrameNodeImpl* frame_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- DCHECK(cu_id.type == resource_coordinator::CoordinationUnitType::kFrame);
- FrameNodeImpl* frame_node = FrameNodeImpl::GetNodeByID(graph_, cu_id);
- if (!frame_node)
- return;
- if (AddFrameImpl(frame_node))
+ DCHECK(frame_node);
+ DCHECK(NodeInGraph(frame_node));
+
+ // TODO(https://crbug.com/944150): This method is called on navigation
+ // complete, and as such can fire more than once for a given frame in
+ // its lifetime. The |frame_nodes_| set is redundant to the frame tree and
+ // should be removed.
+ const bool inserted = frame_nodes_.insert(frame_node).second;
+ if (inserted) {
frame_node->AddPageNode(this);
+
+ OnNumFrozenFramesStateChange(
+ frame_node->lifecycle_state() ==
+ resource_coordinator::mojom::LifecycleState::kFrozen
+ ? 1
+ : 0);
+ MaybeInvalidateInterventionPolicies(frame_node, true /* adding_frame */);
+ }
}
-void PageNodeImpl::RemoveFrame(
- const resource_coordinator::CoordinationUnitID& cu_id) {
+void PageNodeImpl::RemoveFrame(FrameNodeImpl* frame_node) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- DCHECK(cu_id != id());
- FrameNodeImpl* frame_node = FrameNodeImpl::GetNodeByID(graph_, cu_id);
- if (!frame_node)
- return;
- if (RemoveFrameImpl(frame_node))
- frame_node->RemovePageNode(this);
+ DCHECK(frame_node);
+ DCHECK(NodeInGraph(frame_node));
+
+ size_t removed = frame_nodes_.erase(frame_node);
+ DCHECK_EQ(1u, removed);
+ frame_node->RemovePageNode(this);
+
+ OnNumFrozenFramesStateChange(
+ frame_node->lifecycle_state() ==
+ resource_coordinator::mojom::LifecycleState::kFrozen
+ ? -1
+ : 0);
+ MaybeInvalidateInterventionPolicies(frame_node, false /* adding_frame */);
}
void PageNodeImpl::SetIsLoading(bool is_loading) {
@@ -240,6 +255,19 @@
return intervention_policy_[kIndex];
}
+void PageNodeImpl::BeforeDestroyed() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+ // TODO(siggi): This fails browser_tests for some reason. Would be nice to
+ // assert this.
+ // DCHECK(frame_nodes_.empty());
+
+ NodeBase::BeforeDestroyed();
+
+ for (auto* child_frame : frame_nodes_)
+ child_frame->RemovePageNode(this);
+}
+
void PageNodeImpl::set_page_almost_idle(bool page_almost_idle) {
if (page_almost_idle_ == page_almost_idle)
return;
@@ -262,35 +290,6 @@
observer.OnPagePropertyChanged(this, property_type, value);
}
-bool PageNodeImpl::AddFrameImpl(FrameNodeImpl* frame_node) {
- DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- const bool inserted = frame_nodes_.insert(frame_node).second;
- if (inserted) {
- OnNumFrozenFramesStateChange(
- frame_node->lifecycle_state() ==
- resource_coordinator::mojom::LifecycleState::kFrozen
- ? 1
- : 0);
- MaybeInvalidateInterventionPolicies(frame_node, true /* adding_frame */);
- }
- return inserted;
-}
-
-bool PageNodeImpl::RemoveFrameImpl(FrameNodeImpl* frame_node) {
- DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- bool removed = frame_nodes_.erase(frame_node) > 0;
- if (removed) {
- OnNumFrozenFramesStateChange(
- frame_node->lifecycle_state() ==
- resource_coordinator::mojom::LifecycleState::kFrozen
- ? -1
- : 0);
- MaybeInvalidateInterventionPolicies(frame_node, false /* adding_frame */);
- }
-
- return removed;
-}
-
void PageNodeImpl::OnNumFrozenFramesStateChange(int num_frozen_frames_delta) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
num_frozen_frames_ += num_frozen_frames_delta;
diff --git a/chrome/browser/performance_manager/graph/page_node_impl.h b/chrome/browser/performance_manager/graph/page_node_impl.h
index 1713b31..89624e6 100644
--- a/chrome/browser/performance_manager/graph/page_node_impl.h
+++ b/chrome/browser/performance_manager/graph/page_node_impl.h
@@ -29,8 +29,8 @@
Graph* graph);
~PageNodeImpl() override;
- void AddFrame(const resource_coordinator::CoordinationUnitID& cu_id);
- void RemoveFrame(const resource_coordinator::CoordinationUnitID& cu_id);
+ void AddFrame(FrameNodeImpl* frame_node);
+ void RemoveFrame(FrameNodeImpl* frame_node);
void SetIsLoading(bool is_loading);
void SetIsVisible(bool is_visible);
void SetUKMSourceId(int64_t ukm_source_id);
@@ -125,6 +125,8 @@
private:
friend class FrameNodeImpl;
+ void BeforeDestroyed() override;
+
void set_page_almost_idle(bool page_almost_idle);
// CoordinationUnitInterface implementation.
@@ -133,9 +135,6 @@
resource_coordinator::mojom::PropertyType property_type,
int64_t value) override;
- bool AddFrameImpl(FrameNodeImpl* frame_node);
- bool RemoveFrameImpl(FrameNodeImpl* frame_node);
-
// This is called whenever |num_frozen_frames_| changes, or whenever
// |frame_nodes_.size()| changes. It is used to synthesize the
// value of |has_nonempty_beforeunload| and to update the LifecycleState of
diff --git a/chrome/browser/performance_manager/graph/page_node_impl_unittest.cc b/chrome/browser/performance_manager/graph/page_node_impl_unittest.cc
index 43d74a0..bea8d36 100644
--- a/chrome/browser/performance_manager/graph/page_node_impl_unittest.cc
+++ b/chrome/browser/performance_manager/graph/page_node_impl_unittest.cc
@@ -43,9 +43,9 @@
auto frame2_node = CreateNode<FrameNodeImpl>();
auto frame3_node = CreateNode<FrameNodeImpl>();
- page_node->AddFrame(frame1_node->id());
- page_node->AddFrame(frame2_node->id());
- page_node->AddFrame(frame3_node->id());
+ page_node->AddFrame(frame1_node.get());
+ page_node->AddFrame(frame2_node.get());
+ page_node->AddFrame(frame3_node.get());
EXPECT_EQ(3u, page_node->GetFrameNodes().size());
}
@@ -54,9 +54,9 @@
auto frame1_node = CreateNode<FrameNodeImpl>();
auto frame2_node = CreateNode<FrameNodeImpl>();
- page_node->AddFrame(frame1_node->id());
- page_node->AddFrame(frame2_node->id());
- page_node->AddFrame(frame1_node->id());
+ page_node->AddFrame(frame1_node.get());
+ page_node->AddFrame(frame2_node.get());
+ page_node->AddFrame(frame1_node.get());
EXPECT_EQ(2u, page_node->GetFrameNodes().size());
}
@@ -68,14 +68,14 @@
EXPECT_EQ(0u, page_node->GetFrameNodes().size());
EXPECT_FALSE(frame_node->GetPageNode());
- page_node->AddFrame(frame_node->id());
+ page_node->AddFrame(frame_node.get());
// Ensure correct Parent-child relationships have been established.
EXPECT_EQ(1u, page_node->GetFrameNodes().size());
EXPECT_EQ(1u, page_node->GetFrameNodes().count(frame_node.get()));
EXPECT_EQ(page_node.get(), frame_node->GetPageNode());
- page_node->RemoveFrame(frame_node->id());
+ page_node->RemoveFrame(frame_node.get());
// Parent-child relationships should no longer exist.
EXPECT_EQ(0u, page_node->GetFrameNodes().size());
@@ -302,7 +302,7 @@
// Add a frame and expect the values to be invalidated. Reaggregate and
// ensure the appropriate value results.
- page->AddFrame(f0->id());
+ page->AddFrame(f0.get());
EXPECT_EQ(1u, page->GetInterventionPolicyFramesReportedForTesting());
ExpectRawInterventionPolicy(
resource_coordinator::mojom::InterventionPolicy::kUnknown, page.get());
@@ -310,7 +310,7 @@
// Do it again. This time the raw values should be the same as the
// aggregated values above.
- page->AddFrame(f1->id());
+ page->AddFrame(f1.get());
EXPECT_EQ(2u, page->GetInterventionPolicyFramesReportedForTesting());
ExpectRawInterventionPolicy(
resource_coordinator::mojom::InterventionPolicy::kUnknown, page.get());
@@ -322,6 +322,9 @@
ExpectRawInterventionPolicy(
resource_coordinator::mojom::InterventionPolicy::kUnknown, page.get());
ExpectInterventionPolicy(f0_policy_aggregated, page.get());
+
+ f0.reset();
+ page.reset();
}
} // namespace
@@ -433,9 +436,9 @@
TestNodeWrapper<FrameNodeImpl> f1 =
TestNodeWrapper<FrameNodeImpl>::Create(mock_graph);
EXPECT_EQ(0u, page->GetInterventionPolicyFramesReportedForTesting());
- page->AddFrame(f0->id());
+ page->AddFrame(f0.get());
EXPECT_EQ(0u, page->GetInterventionPolicyFramesReportedForTesting());
- page->AddFrame(f1->id());
+ page->AddFrame(f1.get());
EXPECT_EQ(0u, page->GetInterventionPolicyFramesReportedForTesting());
// Set the policies on the first frame. This should be observed by the page
diff --git a/chrome/browser/performance_manager/graph/process_node_impl.cc b/chrome/browser/performance_manager/graph/process_node_impl.cc
index 221d0875..5c60dc26 100644
--- a/chrome/browser/performance_manager/graph/process_node_impl.cc
+++ b/chrome/browser/performance_manager/graph/process_node_impl.cc
@@ -19,13 +19,6 @@
ProcessNodeImpl::~ProcessNodeImpl() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- // Make as if we're transitioning to the null PID before we die to clear this
- // instance from the PID map.
- if (process_id_ != base::kNullProcessId)
- graph()->BeforeProcessPidChange(this, base::kNullProcessId);
-
- for (auto* child_frame : frame_nodes_)
- child_frame->RemoveProcessNode(this);
}
void ProcessNodeImpl::AddFrame(FrameNodeImpl* frame_node) {
@@ -128,6 +121,19 @@
IncrementNumFrozenFrames();
}
+void ProcessNodeImpl::BeforeDestroyed() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ NodeBase::BeforeDestroyed();
+
+ // Make as if we're transitioning to the null PID before we die to clear this
+ // instance from the PID map.
+ if (process_id_ != base::kNullProcessId)
+ graph()->BeforeProcessPidChange(this, base::kNullProcessId);
+
+ for (auto* child_frame : frame_nodes_)
+ child_frame->RemoveProcessNode(this);
+}
+
void ProcessNodeImpl::OnEventReceived(
resource_coordinator::mojom::Event event) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
diff --git a/chrome/browser/performance_manager/graph/process_node_impl.h b/chrome/browser/performance_manager/graph/process_node_impl.h
index b05e9c9..eeb2bae 100644
--- a/chrome/browser/performance_manager/graph/process_node_impl.h
+++ b/chrome/browser/performance_manager/graph/process_node_impl.h
@@ -80,6 +80,8 @@
resource_coordinator::mojom::LifecycleState old_state);
private:
+ void BeforeDestroyed() override;
+
// CoordinationUnitInterface implementation.
void OnEventReceived(resource_coordinator::mojom::Event event) override;
void OnPropertyChanged(
diff --git a/chrome/browser/performance_manager/observers/metrics_collector_unittest.cc b/chrome/browser/performance_manager/observers/metrics_collector_unittest.cc
index 71ba823a..10e694e 100644
--- a/chrome/browser/performance_manager/observers/metrics_collector_unittest.cc
+++ b/chrome/browser/performance_manager/observers/metrics_collector_unittest.cc
@@ -111,7 +111,7 @@
FromBackgroundedToFirstNonPersistentNotificationCreatedUMA) {
auto page_node = CreateNode<PageNodeImpl>();
auto frame_node = CreateNode<FrameNodeImpl>();
- page_node->AddFrame(frame_node->id());
+ page_node->AddFrame(frame_node.get());
page_node->OnMainFrameNavigationCommitted(
ResourceCoordinatorClock::NowTicks(), kDummyID, kDummyUrl);
@@ -147,7 +147,7 @@
FromBackgroundedToFirstNonPersistentNotificationCreatedUMA5MinutesTimeout) {
auto page_node = CreateNode<PageNodeImpl>();
auto frame_node = CreateNode<FrameNodeImpl>();
- page_node->AddFrame(frame_node->id());
+ page_node->AddFrame(frame_node.get());
page_node->OnMainFrameNavigationCommitted(
ResourceCoordinatorClock::NowTicks(), kDummyID, kDummyUrl);
@@ -219,8 +219,8 @@
auto process_node = CreateNode<ProcessNodeImpl>();
auto frame_node = CreateNode<FrameNodeImpl>();
- page_node->AddFrame(frame_node->id());
- frame_node->SetProcess(process_node->id());
+ page_node->AddFrame(frame_node.get());
+ frame_node->SetProcess(process_node.get());
ukm::TestUkmRecorder ukm_recorder;
graph()->set_ukm_recorder(&ukm_recorder);
diff --git a/chrome/browser/performance_manager/performance_manager_tab_helper.cc b/chrome/browser/performance_manager/performance_manager_tab_helper.cc
index bd0455e..1194f4ce 100644
--- a/chrome/browser/performance_manager/performance_manager_tab_helper.cc
+++ b/chrome/browser/performance_manager/performance_manager_tab_helper.cc
@@ -103,7 +103,7 @@
performance_manager_->task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&FrameNodeImpl::AddChildFrame,
- base::Unretained(parent_frame_node.get()), frame->id()));
+ base::Unretained(parent_frame_node.get()), frame.get()));
}
RenderProcessUserData* user_data =
@@ -117,7 +117,7 @@
performance_manager_->task_runner()->PostTask(
FROM_HERE, base::BindOnce(&FrameNodeImpl::SetProcess,
base::Unretained(frame.get()),
- user_data->process_node()->id()));
+ user_data->process_node()));
}
frames_[render_frame_host] = std::move(frame);
@@ -163,7 +163,7 @@
content::RenderFrameHost* render_frame_host =
navigation_handle->GetRenderFrameHost();
// Make sure the hierarchical structure is constructed before sending signal
- // to Resource Coordinator.
+ // to the performance manager.
// TODO(siggi): Ideally this would be a DCHECK, but it seems it's possible
// to get a DidFinishNavigation notification for a deleted frame with
// the network service.
@@ -173,7 +173,7 @@
performance_manager_->task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&PageNodeImpl::AddFrame,
- base::Unretained(page_node_.get()), it->second->id()));
+ base::Unretained(page_node_.get()), it->second.get()));
if (navigation_handle->IsInMainFrame()) {
OnMainFrameNavigation(navigation_handle->GetNavigationId());
