diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 007e7ff0d3..d4afe6b7e8 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -147,3 +147,4 @@ viz_fix_visual_artifacts_due_to_resizing_root_render_pass_with_dcomp.patch viz_do_not_overallocate_surface_on_initial_render.patch viz_create_isbufferqueuesupportedandenabled.patch viz_fix_visual_artifacts_while_resizing_window_with_dcomp.patch +graphite_handle_out_of_order_recording_errors.patch diff --git a/patches/chromium/graphite_handle_out_of_order_recording_errors.patch b/patches/chromium/graphite_handle_out_of_order_recording_errors.patch new file mode 100644 index 0000000000..652ce436e6 --- /dev/null +++ b/patches/chromium/graphite_handle_out_of_order_recording_errors.patch @@ -0,0 +1,348 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Sunny Sachanandani +Date: Fri, 30 Jan 2026 12:51:05 -0800 +Subject: [graphite] Handle out of order recording errors + +Explicitly handle out of order recording errors to crash just like async +shader compile failed errors. Also, emit the insert status to an UMA +histogram at 1% subsampling and log all error statuses at runtime. + +Bug: 458722690 +Change-Id: Id94657be6ae870dcf8adba71aff216386a6a6964 +Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7533855 +Commit-Queue: Sunny Sachanandani +Auto-Submit: Sunny Sachanandani +Reviewed-by: Jonathan Ross +Reviewed-by: Michael Ludwig +Cr-Commit-Position: refs/heads/main@{#1577487} + +diff --git a/gpu/command_buffer/service/graphite_shared_context.cc b/gpu/command_buffer/service/graphite_shared_context.cc +index f9a47ac16bddc2352d81f43629d8ce8c2d2b3b99..9856d68f893e4329fda5d002835613161b346bc1 100644 +--- a/gpu/command_buffer/service/graphite_shared_context.cc ++++ b/gpu/command_buffer/service/graphite_shared_context.cc +@@ -4,7 +4,10 @@ + + #include "gpu/command_buffer/service/graphite_shared_context.h" + ++#include "base/logging.h" + #include "base/memory/ptr_util.h" ++#include "base/metrics/histogram_macros.h" ++#include "base/rand_util.h" + #include "base/task/single_thread_task_runner.h" + #include "gpu/command_buffer/common/shm_count.h" + #include "third_party/skia/include/core/SkColorSpace.h" +@@ -14,6 +17,44 @@ + namespace gpu { + + namespace { ++// This is emitted to UMA - values should not be reordered, only appended! ++// LINT.IfChange(InsertRecordingStatusUma) ++enum class InsertRecordingStatusUma { ++ kSuccess, ++ kInvalidRecording, ++ kPromiseImageInstantiationFailed, ++ kAddCommandsFailed, ++ kAsyncShaderCompilesFailed, ++ kOutOfOrderRecording, ++ kMaxValue = kOutOfOrderRecording ++}; ++// LINT.ThenChange(//tools/metrics/histograms/metadata/gpu/enums.xml:GraphiteInsertRecordingStatus) ++ ++InsertRecordingStatusUma InsertRecordingStatusUma( ++ skgpu::graphite::InsertStatus insert_status) { ++ // InsertStatus almost behaves like an enum class, but not quite since it can ++ // convert to both bool and integer types and can't be used in a switch. ++ if (insert_status == skgpu::graphite::InsertStatus::kSuccess) { ++ return InsertRecordingStatusUma::kSuccess; ++ } else if (insert_status == ++ skgpu::graphite::InsertStatus::kInvalidRecording) { ++ return InsertRecordingStatusUma::kInvalidRecording; ++ } else if (insert_status == ++ skgpu::graphite::InsertStatus::kPromiseImageInstantiationFailed) { ++ return InsertRecordingStatusUma::kPromiseImageInstantiationFailed; ++ } else if (insert_status == ++ skgpu::graphite::InsertStatus::kAddCommandsFailed) { ++ return InsertRecordingStatusUma::kAddCommandsFailed; ++ } else if (insert_status == ++ skgpu::graphite::InsertStatus::kAsyncShaderCompilesFailed) { ++ return InsertRecordingStatusUma::kAsyncShaderCompilesFailed; ++ } else if (insert_status == ++ skgpu::graphite::InsertStatus::kOutOfOrderRecording) { ++ return InsertRecordingStatusUma::kOutOfOrderRecording; ++ } ++ NOTREACHED(); ++} ++ + struct RecordingContext { + skgpu::graphite::GpuFinishedProc old_finished_proc; + skgpu::graphite::GpuFinishedContext old_context; +@@ -216,20 +257,38 @@ bool GraphiteSharedContext::InsertRecordingImpl( + + auto insert_status = graphite_context_->insertRecording(*info_ptr); + +- // TODO(433845560): Check the kAddCommandsFailed failures. +- // Crash only if we're not simulating a failure for testing. + const bool simulating_insert_failure = + info_ptr->fSimulatedStatus != skgpu::graphite::InsertStatus::kSuccess; + +- // InsertStatus::kAsyncShaderCompilesFailed is also an unrecoverable error for +- // which we should also clear the disk shader cache in case the error was due +- // to a corrupted cached shader blob. ++ // Crash, log, or emit UMA only if we're not simulating a failure for testing. ++ if (!simulating_insert_failure) { ++ if (base::ShouldRecordSubsampledMetric(0.01)) { ++ UMA_HISTOGRAM_ENUMERATION("GPU.Graphite.InsertRecordingStatus", ++ InsertRecordingStatusUma(insert_status)); ++ } ++ if (insert_status != skgpu::graphite::InsertStatus::kSuccess) { ++ // skgpu::graphite::InsertStatus almost behaves like an enum class, but ++ // not quite - it can't be static_cast to an int. ++ LOG(ERROR) << "Graphite insertRecording failed with status " ++ << static_cast(InsertRecordingStatusUma(insert_status)); ++ } ++ } ++ ++ // kAsyncShaderCompilesFailed and kOutOfOrderRecording are unrecoverable ++ // failures because they cause future recordings to be rendered incorrectly. ++ // TODO(433845560): Check the kAddCommandsFailed failures. ++ const bool is_unrecoverable_failure = ++ insert_status == ++ skgpu::graphite::InsertStatus::kAsyncShaderCompilesFailed || ++ insert_status == skgpu::graphite::InsertStatus::kOutOfOrderRecording; ++ // For kAsyncShaderCompilesFailed, we should also clear the disk shader ++ // cache in case the error was due to a corrupted cached shader blob. ++ std::optional use_shader_cache; + if (insert_status == + skgpu::graphite::InsertStatus::kAsyncShaderCompilesFailed) { +- GpuProcessShmCount::ScopedIncrement use_shader_cache( +- use_shader_cache_shm_count_); +- CHECK(simulating_insert_failure); ++ use_shader_cache.emplace(use_shader_cache_shm_count_); + } ++ CHECK(simulating_insert_failure || !is_unrecoverable_failure); + + // All other failure modes are recoverable in the sense that future recordings + // will be rendered correctly, so merely return a boolean here so that callers +diff --git a/gpu/command_buffer/service/graphite_shared_context_unittest.cc b/gpu/command_buffer/service/graphite_shared_context_unittest.cc +index 0de107a9d86ff8217a4c537598f198f313ecf9df..852343ab6804efed962b707b748ddf1920c20ea7 100644 +--- a/gpu/command_buffer/service/graphite_shared_context_unittest.cc ++++ b/gpu/command_buffer/service/graphite_shared_context_unittest.cc +@@ -6,6 +6,7 @@ + + #include "base/threading/thread.h" + #include "gpu/command_buffer/common/shm_count.h" ++#include "gpu/command_buffer/service/skia_utils.h" + #include "skia/buildflags.h" + #include "testing/gmock/include/gmock/gmock.h" + #include "testing/gtest/include/gtest/gtest.h" +@@ -86,8 +87,7 @@ class GraphiteSharedContextTest : public testing::TestWithParam { + + wgpu::DeviceDescriptor device_desc = {}; + +- wgpu::Device device = +- wgpu::Adapter(adapters[0].Get()).CreateDevice(&device_desc); ++ auto device = wgpu::Adapter(adapters[0].Get()).CreateDevice(&device_desc); + CHECK(device); + + skgpu::graphite::DawnBackendContext backend_context = {}; +@@ -95,7 +95,11 @@ class GraphiteSharedContextTest : public testing::TestWithParam { + backend_context.fDevice = device; + backend_context.fQueue = device.GetQueue(); + +- skgpu::graphite::ContextOptions context_options = {}; ++ // Use the default Graphite context options that Chromium uses e.g. disallow ++ // things like out of order recordings. ++ gpu::GpuDriverBugWorkarounds workarounds; ++ auto context_options = GetDefaultGraphiteContextOptions(workarounds); ++ + graphite_shared_context_ = std::make_unique( + skgpu::graphite::ContextFactory::MakeDawn(backend_context, + context_options), +@@ -133,14 +137,12 @@ TEST_P(GraphiteSharedContextTest, ConcurrentAccess) { + auto run_graphite_functions = + [](GraphiteSharedContext* graphite_shared_context) { + // Call a method that acquires the lock +- std::unique_ptr recorder = +- graphite_shared_context->makeRecorder(); ++ auto recorder = graphite_shared_context->makeRecorder(); + EXPECT_TRUE(recorder); + + for (int i = 0; i < 2; ++i) { + for (int j = 0; j < 10; ++j) { +- std::unique_ptr recording = +- recorder->snap(); ++ auto recording = recorder->snap(); + skgpu::graphite::InsertRecordingInfo info = {}; + info.fRecording = recording.get(); + EXPECT_TRUE(recording); +@@ -172,18 +174,17 @@ TEST_P(GraphiteSharedContextTest, ConcurrentAccess) { + } + + TEST_P(GraphiteSharedContextTest, AsyncShaderCompilesFailed) { +- std::unique_ptr recorder = +- graphite_shared_context_->makeRecorder(); ++ auto recorder = graphite_shared_context_->makeRecorder(); + EXPECT_TRUE(recorder); + + auto ii = SkImageInfo::Make(64, 64, kN32_SkColorType, kPremul_SkAlphaType); +- sk_sp surface1 = SkSurfaces::RenderTarget(recorder.get(), ii); ++ auto surface1 = SkSurfaces::RenderTarget(recorder.get(), ii); + surface1->getCanvas()->clear(SK_ColorRED); + +- sk_sp surface2 = SkSurfaces::RenderTarget(recorder.get(), ii); ++ auto surface2 = SkSurfaces::RenderTarget(recorder.get(), ii); + surface2->getCanvas()->drawImage(surface1->makeTemporaryImage(), 0, 0); + +- std::unique_ptr recording = recorder->snap(); ++ auto recording = recorder->snap(); + EXPECT_TRUE(recording); + + skgpu::graphite::InsertRecordingInfo info = {}; +@@ -197,19 +198,43 @@ TEST_P(GraphiteSharedContextTest, AsyncShaderCompilesFailed) { + EXPECT_FALSE(graphite_shared_context_->insertRecording(info)); + } + ++TEST_P(GraphiteSharedContextTest, OutOfOrderRecording) { ++ auto recorder = graphite_shared_context_->makeRecorder(); ++ EXPECT_TRUE(recorder); ++ ++ auto ii = SkImageInfo::Make(64, 64, kN32_SkColorType, kPremul_SkAlphaType); ++ auto surface1 = SkSurfaces::RenderTarget(recorder.get(), ii); ++ surface1->getCanvas()->clear(SK_ColorRED); ++ auto recording1 = recorder->snap(); ++ EXPECT_TRUE(recording1); ++ ++ auto surface2 = SkSurfaces::RenderTarget(recorder.get(), ii); ++ surface2->getCanvas()->drawImage(surface1->makeTemporaryImage(), 0, 0); ++ auto recording2 = recorder->snap(); ++ EXPECT_TRUE(recording2); ++ ++ skgpu::graphite::InsertRecordingInfo info = {}; ++ ++ info.fRecording = recording2.get(); ++ graphite_shared_context_->insertRecording(info); ++ ++ info.fRecording = recording1.get(); ++ EXPECT_DEATH_IF_SUPPORTED(graphite_shared_context_->insertRecording(info), ++ ""); ++} ++ + TEST_P(GraphiteSharedContextTest, AddCommandsFailed) { +- std::unique_ptr recorder = +- graphite_shared_context_->makeRecorder(); ++ auto recorder = graphite_shared_context_->makeRecorder(); + EXPECT_TRUE(recorder); + + auto ii = SkImageInfo::Make(64, 64, kN32_SkColorType, kPremul_SkAlphaType); +- sk_sp surface1 = SkSurfaces::RenderTarget(recorder.get(), ii); ++ auto surface1 = SkSurfaces::RenderTarget(recorder.get(), ii); + surface1->getCanvas()->clear(SK_ColorRED); + +- sk_sp surface2 = SkSurfaces::RenderTarget(recorder.get(), ii); ++ auto surface2 = SkSurfaces::RenderTarget(recorder.get(), ii); + surface2->getCanvas()->drawImage(surface1->makeTemporaryImage(), 0, 0); + +- std::unique_ptr recording = recorder->snap(); ++ auto recording = recorder->snap(); + EXPECT_TRUE(recording); + + skgpu::graphite::InsertRecordingInfo info = {}; +@@ -223,39 +248,37 @@ TEST_P(GraphiteSharedContextTest, AddCommandsFailed) { + } + + TEST_P(GraphiteSharedContextTest, LowPendingRecordings) { +- std::unique_ptr recorder = +- graphite_shared_context_->makeRecorder(); ++ auto recorder = graphite_shared_context_->makeRecorder(); + EXPECT_TRUE(recorder); + +- std::unique_ptr recording = recorder->snap(); +- EXPECT_TRUE(recording); +- +- skgpu::graphite::InsertRecordingInfo info = {}; +- info.fRecording = recording.get(); +- + // No flush is expected if the number of pending recordings is low. + EXPECT_CALL(backend_flush_callback_, Flush()).Times(0); + + for (size_t i = 0; i < kMaxPendingRecordings - 1; ++i) { ++ auto recording = recorder->snap(); ++ EXPECT_TRUE(recording); ++ ++ skgpu::graphite::InsertRecordingInfo info = {}; ++ info.fRecording = recording.get(); ++ + EXPECT_TRUE(graphite_shared_context_->insertRecording(info)); + } + } + + TEST_P(GraphiteSharedContextTest, MaxPendingRecordings) { +- std::unique_ptr recorder = +- graphite_shared_context_->makeRecorder(); ++ auto recorder = graphite_shared_context_->makeRecorder(); + EXPECT_TRUE(recorder); + +- std::unique_ptr recording = recorder->snap(); +- EXPECT_TRUE(recording); +- +- skgpu::graphite::InsertRecordingInfo info = {}; +- info.fRecording = recording.get(); +- + // Expect a flush when the number of pending recordings reaches the max. + EXPECT_CALL(backend_flush_callback_, Flush()).Times(1); + + for (size_t i = 0; i < kMaxPendingRecordings; ++i) { ++ auto recording = recorder->snap(); ++ EXPECT_TRUE(recording); ++ ++ skgpu::graphite::InsertRecordingInfo info = {}; ++ info.fRecording = recording.get(); ++ + EXPECT_TRUE(graphite_shared_context_->insertRecording(info)); + } + } +diff --git a/tools/metrics/histograms/metadata/gpu/enums.xml b/tools/metrics/histograms/metadata/gpu/enums.xml +index 58471a1d69c65cdf559f98ec2e11550bc16520d2..b62d8b78d34bfec491bcd5e6c65ff63eaedf00dd 100644 +--- a/tools/metrics/histograms/metadata/gpu/enums.xml ++++ b/tools/metrics/histograms/metadata/gpu/enums.xml +@@ -1104,6 +1104,19 @@ Called by update_gpu_driver_bug_workaround_entries.py.--> + + + ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ + + + +diff --git a/tools/metrics/histograms/metadata/gpu/histograms.xml b/tools/metrics/histograms/metadata/gpu/histograms.xml +index ea8b129a2f4aba4d8afd9c3338f21574a12185dd..80bf969cbf66eac7f970b7c096da83c69424c38a 100644 +--- a/tools/metrics/histograms/metadata/gpu/histograms.xml ++++ b/tools/metrics/histograms/metadata/gpu/histograms.xml +@@ -1004,6 +1004,16 @@ chromium-metrics-reviews@google.com. + + + ++ ++ sunnyps@chromium.org ++ michaelludwig@google.com ++ chrome-gpu-metric-alerts@chromium.org ++ ++ The return value for each Graphite insertRecording call made by Chromium. ++ ++ ++ + + vasilyt@chromium.org diff --git a/patches/config.json b/patches/config.json index 8d297c0fd3..14a5a44774 100644 --- a/patches/config.json +++ b/patches/config.json @@ -12,5 +12,6 @@ { "patch_dir": "src/electron/patches/ReactiveObjC", "repo": "src/third_party/squirrel.mac/vendor/ReactiveObjC" }, { "patch_dir": "src/electron/patches/webrtc", "repo": "src/third_party/webrtc" }, { "patch_dir": "src/electron/patches/reclient-configs", "repo": "src/third_party/engflow-reclient-configs" }, - { "patch_dir": "src/electron/patches/sqlite", "repo": "src/third_party/sqlite/src" } + { "patch_dir": "src/electron/patches/sqlite", "repo": "src/third_party/sqlite/src" }, + { "patch_dir": "src/electron/patches/skia", "repo": "src/third_party/skia" } ] diff --git a/patches/skia/.patches b/patches/skia/.patches new file mode 100644 index 0000000000..1a7f496d17 --- /dev/null +++ b/patches/skia/.patches @@ -0,0 +1 @@ +graphite_add_insertstatus_koutoforderrecording.patch diff --git a/patches/skia/graphite_add_insertstatus_koutoforderrecording.patch b/patches/skia/graphite_add_insertstatus_koutoforderrecording.patch new file mode 100644 index 0000000000..7b52752eba --- /dev/null +++ b/patches/skia/graphite_add_insertstatus_koutoforderrecording.patch @@ -0,0 +1,54 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Michael Ludwig +Date: Fri, 16 Jan 2026 15:28:27 -0500 +Subject: [graphite] Add InsertStatus::kOutOfOrderRecording + +Bug: b/458722690 +Change-Id: I1c3661b9c765f46f039f0044ae3557a0e7f619cf +Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1143598 +Auto-Submit: Michael Ludwig +Reviewed-by: Thomas Smith +Commit-Queue: Nicolette Prevost +Reviewed-by: Nicolette Prevost + +diff --git a/include/gpu/graphite/GraphiteTypes.h b/include/gpu/graphite/GraphiteTypes.h +index a638da8988b2e8963255537c36f98d0233c917e8..a8e40871531ce0c2a2b98a4c8ba1c44e9b374c2d 100644 +--- a/include/gpu/graphite/GraphiteTypes.h ++++ b/include/gpu/graphite/GraphiteTypes.h +@@ -52,7 +52,12 @@ public: + kAddCommandsFailed, + // Internal failure, shader pipeline compilation failed (driver issue, or disk corruption), + // state unrecoverable. +- kAsyncShaderCompilesFailed ++ kAsyncShaderCompilesFailed, ++ // The inserted Recording is out of order from what the Context expects (when ++ // `[Context|Recorder]Options::fRequireOrderedRecordings` is true), which can either ++ // represent a client synchronization error or an internal failure when a prior dependent ++ // Recording failed for some reason, no CB changes but state likely unrecoverable. ++ kOutOfOrderRecording, + }; + + constexpr InsertStatus() : fValue(kSuccess) {} +diff --git a/relnotes/out_of_order_status.md b/relnotes/out_of_order_status.md +new file mode 100644 +index 0000000000000000000000000000000000000000..74f7c889338925d132a2d80cdba40d321807c614 +--- /dev/null ++++ b/relnotes/out_of_order_status.md +@@ -0,0 +1,4 @@ ++* Graphite's InsertStatus now has an additional kOutOfOrderRecording to differentiate this ++ unrecoverable error from programming errors that would lead to kInvalidRecording. Out of order ++ recordings can currently arise "naturally" if prior dependent recordings failed due to resource ++ creation or update errors from the GPU driver. +diff --git a/src/gpu/graphite/QueueManager.cpp b/src/gpu/graphite/QueueManager.cpp +index 2948e071390095f3a1a1d7fdb6c7878adeaf9c53..f5f97f23e4957b819f9d92e8f03f49cd6fad1ab3 100644 +--- a/src/gpu/graphite/QueueManager.cpp ++++ b/src/gpu/graphite/QueueManager.cpp +@@ -122,7 +122,7 @@ InsertStatus QueueManager::addRecording(const InsertRecordingInfo& info, Context + if (recorderID != SK_InvalidGenID) { + uint32_t* recordingID = fLastAddedRecordingIDs.find(recorderID); + RETURN_FAIL_IF(recordingID && info.fRecording->priv().uniqueID() != *recordingID + 1, +- InsertStatus::kInvalidRecording, ++ InsertStatus::kOutOfOrderRecording, + "Recordings are expected to be replayed in order"); + + // Note the new Recording ID.