[CodeHealth] Fix dangling CompositorRenderPass* in FixedPassData
We now make sure the CompositorRenderPass* is only valid during the
aggregation. The ptr is set at the start of the aggregation and reset
back to null at the end of the aggregation.
Thanks to kylechar@.
Bug: 1512711
Change-Id: I9b541ad5da709dd5e641f489a492446743c55371
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5132533
Reviewed-by: Kyle Charbonneau <kylechar@chromium.org>
Commit-Queue: William Liu <liuwilliam@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1243446}
diff --git a/components/viz/service/display/resolved_frame_data.cc b/components/viz/service/display/resolved_frame_data.cc
index 25312cd..4aed39fa2 100644
--- a/components/viz/service/display/resolved_frame_data.cc
+++ b/components/viz/service/display/resolved_frame_data.cc
@@ -63,11 +63,26 @@
ResolvedPassData& ResolvedPassData::operator=(ResolvedPassData&& other) =
default;
+const CompositorRenderPass& ResolvedPassData::render_pass() const {
+ CHECK(fixed_.render_pass);
+ return *fixed_.render_pass;
+}
+
void ResolvedPassData::CopyAndResetPersistentPassData() {
previous_persistent_data_ = current_persistent_data_;
current_persistent_data_ = PersistentPassData();
}
+void ResolvedPassData::SetCompositorRenderPass(CompositorRenderPass* pass) {
+ CHECK(pass);
+ CHECK_EQ(pass->id, fixed_.render_pass_id);
+ fixed_.render_pass = pass;
+}
+
+void ResolvedPassData::ResetCompositorRenderPass() {
+ fixed_.render_pass = nullptr;
+}
+
ResolvedFrameData::ResolvedFrameData(DisplayResourceProvider* resource_provider,
Surface* surface,
uint64_t previous_frame_index,
@@ -252,6 +267,7 @@
for (auto& resolved_pass : resolved_passes_) {
resolved_pass.aggregation().Reset();
resolved_pass.CopyAndResetPersistentPassData();
+ resolved_pass.ResetCompositorRenderPass();
}
previous_frame_index_ = frame_index_;
@@ -320,6 +336,21 @@
return resolved_passes_.back().render_pass().output_rect;
}
+void ResolvedFrameData::SetRenderPassPointers() {
+ const CompositorRenderPassList& render_pass_list =
+ surface_->GetActiveFrame().render_pass_list;
+
+ // `render_pass_list` and `resolved_passes_` should have the same size and
+ // order.
+ CHECK_EQ(render_pass_list.size(), resolved_passes_.size());
+ for (size_t i = 0; i < resolved_passes_.size(); ++i) {
+ ResolvedPassData& resolved_pass = resolved_passes_[i];
+ const auto& render_pass = render_pass_list[i];
+ CHECK_EQ(resolved_pass.render_pass_id(), render_pass->id);
+ resolved_pass.SetCompositorRenderPass(render_pass.get());
+ }
+}
+
void ResolvedFrameData::RegisterWithResourceProvider() {
child_resource_id_ = resource_provider_->CreateChild(
base::BindRepeating(&SurfaceClient::UnrefResources, surface_->client()),
diff --git a/components/viz/service/display/resolved_frame_data.h b/components/viz/service/display/resolved_frame_data.h
index 12cf538..7116c875 100644
--- a/components/viz/service/display/resolved_frame_data.h
+++ b/components/viz/service/display/resolved_frame_data.h
@@ -43,8 +43,14 @@
FixedPassData& operator=(FixedPassData&& other);
~FixedPassData();
- raw_ptr<CompositorRenderPass, AcrossTasksDanglingUntriaged> render_pass =
- nullptr;
+ // Only valid during aggregation: set at the beginning a new round of
+ // aggregation and reset to null at the end of each aggregation.
+ //
+ // This shouldn't be dangling anymore because CompositorFrames are never
+ // destroyed during aggregation so the pointer will remain valid for the
+ // duration of aggregation (until it's set to null).
+ raw_ptr<CompositorRenderPass> render_pass = nullptr;
+
// DrawQuads in |render_pass| that can contribute additional damage (eg.
// surface and render passes) that need to be visited during the prewalk phase
// of aggregation. Stored in front-to-back order like in |render_pass|.
@@ -147,9 +153,7 @@
ResolvedPassData(ResolvedPassData&& other);
ResolvedPassData& operator=(ResolvedPassData&& other);
- const CompositorRenderPass& render_pass() const {
- return *fixed_.render_pass;
- }
+ const CompositorRenderPass& render_pass() const;
AggregatedRenderPassId remapped_id() const { return fixed_.remapped_id; }
CompositorRenderPassId render_pass_id() const {
return fixed_.render_pass_id;
@@ -186,6 +190,14 @@
void CopyAndResetPersistentPassData();
+ // Set `fixed_.render_pass` to `pass`. Should be called at the beginning of an
+ // aggregation.
+ void SetCompositorRenderPass(CompositorRenderPass* pass);
+
+ // Set `fixed_.render_pass` back to null, to avoid the dangling pointer
+ // after aggregation. Should be called at the end of an aggregation.
+ void ResetCompositorRenderPass();
+
private:
friend class ResolvedFrameData;
@@ -300,6 +312,10 @@
// Returns the root render pass output_rect.
const gfx::Rect& GetOutputRect() const;
+ // Set `CompositorRenderPass` for all `resolved_passes_`. Each
+ // `ResolvedPassData` must have been aggregated before.
+ void SetRenderPassPointers();
+
private:
void RegisterWithResourceProvider();
void MovePersistentPassDataFromPreviousFrame(
diff --git a/components/viz/service/display/surface_aggregator.cc b/components/viz/service/display/surface_aggregator.cc
index 4ee8bc5..ca0da76 100644
--- a/components/viz/service/display/surface_aggregator.cc
+++ b/components/viz/service/display/surface_aggregator.cc
@@ -733,13 +733,18 @@
// Mark the frame as used this aggregation so it persists.
resolved_frame.MarkAsUsedInAggregation();
- // If there is a new CompositorFrame for `surface` compute resolved frame
- // data for the new resolved CompositorFrame.
if (resolved_frame.previous_frame_index() !=
surface->GetActiveFrameIndex()) {
+ // If there is a new CompositorFrame for `surface` compute resolved frame
+ // data.
base::ElapsedTimer timer;
ProcessResolvedFrame(resolved_frame);
stats_->declare_resources_time += timer.Elapsed();
+ } else if (resolved_frame.is_valid()) {
+ // The same `CompositorFrame` since last aggregation. Set the
+ // `CompositorRenderPass` pointer back to `ResolvedPassData`. Only
+ // applicable to valid `ResolvedFrameData`.
+ resolved_frame.SetRenderPassPointers();
}
}
