mirror of
https://github.com/electron/electron.git
synced 2026-04-10 03:01:51 -04:00
chore: cherry-pick 1 change from Release-0-M122 (#41405)
This commit is contained in:
Pedro Pontes
@@ -144,3 +144,4 @@ feat_allow_code_cache_in_custom_schemes.patch
|
||||
enable_partition_alloc_ref_count_size.patch
|
||||
ensure_an_axcontext_before_painting.patch
|
||||
safely_crash_on_dangling_profile.patch
|
||||
prevent_mojotrap_event_re-ordering.patch
|
||||
|
||||
156
patches/chromium/prevent_mojotrap_event_re-ordering.patch
Normal file
156
patches/chromium/prevent_mojotrap_event_re-ordering.patch
Normal file
@@ -0,0 +1,156 @@
|
||||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
|
||||
From: Ken Rockot <rockot@google.com>
|
||||
Date: Thu, 15 Feb 2024 20:30:22 +0000
|
||||
Subject: Prevent MojoTrap event re-ordering
|
||||
|
||||
(cherry picked from commit 3557a2fcbdd8167f97ca81171be2e0da9c4f0647)
|
||||
|
||||
Fixed: 1508753
|
||||
Change-Id: I9ec14a12e7d1d147bda63703e1d6619fa30c8a51
|
||||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5253039
|
||||
Commit-Queue: Ken Rockot <rockot@google.com>
|
||||
Reviewed-by: Robert Sesek <rsesek@chromium.org>
|
||||
Cr-Original-Commit-Position: refs/heads/main@{#1254840}
|
||||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5299857
|
||||
Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
|
||||
Commit-Queue: Alex Gough <ajgo@chromium.org>
|
||||
Cr-Commit-Position: refs/branch-heads/6261@{#794}
|
||||
Cr-Branched-From: 9755d9d81e4a8cb5b4f76b23b761457479dbb06b-refs/heads/main@{#1250580}
|
||||
|
||||
diff --git a/mojo/core/ipcz_driver/mojo_trap.cc b/mojo/core/ipcz_driver/mojo_trap.cc
|
||||
index 7a5765a74a7f64e584b0a080e659c88c019adcbb..2c98335dd7846fe44fd048265078dfffcb0b63f6 100644
|
||||
--- a/mojo/core/ipcz_driver/mojo_trap.cc
|
||||
+++ b/mojo/core/ipcz_driver/mojo_trap.cc
|
||||
@@ -544,7 +544,15 @@ void MojoTrap::DispatchOrQueueEvent(Trigger& trigger,
|
||||
}
|
||||
|
||||
dispatching_thread_ = base::PlatformThread::CurrentRef();
|
||||
- DispatchEvent(event);
|
||||
+
|
||||
+ // If `trigger.removed` is true, then either this is the cancellation event
|
||||
+ // for the trigger (in which case it's OK to dispatch), or it was cancelled on
|
||||
+ // some other thread while we were blocked above. In the latter case, this
|
||||
+ // event is no longer valid and cannot be dispatched.
|
||||
+ // See https://crbug.com/1508753.
|
||||
+ if (!trigger.removed || event.result == MOJO_RESULT_CANCELLED) {
|
||||
+ DispatchEvent(event);
|
||||
+ }
|
||||
|
||||
// NOTE: This vector is only shrunk by the clear() below, but it may
|
||||
// accumulate more events during each iteration. Hence we iterate by index.
|
||||
diff --git a/mojo/core/trap_unittest.cc b/mojo/core/trap_unittest.cc
|
||||
index 0fd449d9598810fd34372d69d1d1599a0c88b955..4058da72eef8b5a11432b9a17d6ff3ecfd1306e8 100644
|
||||
--- a/mojo/core/trap_unittest.cc
|
||||
+++ b/mojo/core/trap_unittest.cc
|
||||
@@ -1747,6 +1747,111 @@ TEST_F(TrapTest, TriggerDuringDestruction) {
|
||||
MojoClose(b);
|
||||
}
|
||||
|
||||
+TEST_F(TrapTest, RaceDispatchAndBlockedCancel) {
|
||||
+ // Regression test for https://crbug.com/1508753. This bug was caused by
|
||||
+ // reordering of a MOJO_RESULT_CANCELLED event to before some other event for
|
||||
+ // the same trap context, violating an API constraint that must be upheld for
|
||||
+ // memory safety in application code. The scenario which could elicit the bug
|
||||
+ // was as follows:
|
||||
+ //
|
||||
+ // 1. A single trap is watching two pipes, P and Q.
|
||||
+ // 2. Thread A closes pipe P, triggering a CANCELLED event.
|
||||
+ // 3. Thread A re-arms the trap from within the CANCELLED event handler.
|
||||
+ // 4. Thread B changes Q's state to elicit a event for Q (not CANCELLED).
|
||||
+ // 5. Thread B dispatch is blocked because thread A is still dispatching.
|
||||
+ // 6. Before thread B gets a chance to be scheduled, thread A closes Q.
|
||||
+ // 7. Thread A dispatches a CANCELLED event for Q.
|
||||
+ // 8. Thread B is scheduled and proceeds to dispatch its Q event. [BAD]
|
||||
+
|
||||
+ struct State;
|
||||
+
|
||||
+ struct Pipe {
|
||||
+ explicit Pipe(State* state) : state(state) { CreateMessagePipe(&a, &b); }
|
||||
+
|
||||
+ uintptr_t context() const { return reinterpret_cast<uintptr_t>(this); }
|
||||
+
|
||||
+ MojoHandle a;
|
||||
+ MojoHandle b;
|
||||
+ bool trigger_cancelled = false;
|
||||
+
|
||||
+ // Back-reference to common state so it's reachable from the event handler.
|
||||
+ const raw_ptr<State> state;
|
||||
+ };
|
||||
+
|
||||
+ struct State {
|
||||
+ Pipe pipe0{this};
|
||||
+ Pipe pipe1{this};
|
||||
+ MojoHandle trap;
|
||||
+ base::WaitableEvent event;
|
||||
+ };
|
||||
+ State state;
|
||||
+
|
||||
+ // NOTE: + to turn the lambda into a function pointer.
|
||||
+ const MojoTrapEventHandler event_handler = +[](const MojoTrapEvent* event) {
|
||||
+ auto& pipe = *reinterpret_cast<Pipe*>(event->trigger_context);
|
||||
+ auto& state = *pipe.state;
|
||||
+
|
||||
+ // If the bug is present, this expectation can fail flakily. No event should
|
||||
+ // fire for a pipe after its watch has been cancelled.
|
||||
+ EXPECT_FALSE(pipe.trigger_cancelled);
|
||||
+
|
||||
+ if (event->result == MOJO_RESULT_CANCELLED) {
|
||||
+ pipe.trigger_cancelled = true;
|
||||
+
|
||||
+ if (&pipe == &state.pipe0) {
|
||||
+ // When pipe0's watch is cancelled (on the main thread by closure down
|
||||
+ // below) we re-arm the trap immediately. This must succeed because
|
||||
+ // `pipe1.a` is now the only handle being watched, and it's still in an
|
||||
+ // uninteresting state.
|
||||
+ EXPECT_EQ(MOJO_RESULT_OK,
|
||||
+ MojoArmTrap(state.trap, nullptr, nullptr, nullptr));
|
||||
+
|
||||
+ // Unblock the other thread so it can elicit a trap event on pipe1 now
|
||||
+ // that the trap is re-armed. It will still block just before
|
||||
+ // dispatching as long as we're still in this event handler on the main
|
||||
+ // thread.
|
||||
+ state.event.Signal();
|
||||
+
|
||||
+ // A nice long delay to make it very likely for the waiting
|
||||
+ // ThreadedRunner to progress right up to its event dispatch.
|
||||
+ base::PlatformThread::Sleep(base::Milliseconds(10));
|
||||
+
|
||||
+ // Trigger cancellation for pipe1 by closing its `a`. This will queue a
|
||||
+ // CANCELLED event to fire on the same thread immediately after we
|
||||
+ // return from this handler.
|
||||
+ MojoClose(state.pipe1.a);
|
||||
+ }
|
||||
+ }
|
||||
+ };
|
||||
+
|
||||
+ EXPECT_EQ(MOJO_RESULT_OK,
|
||||
+ MojoCreateTrap(event_handler, nullptr, &state.trap));
|
||||
+ EXPECT_EQ(
|
||||
+ MOJO_RESULT_OK,
|
||||
+ MojoAddTrigger(state.trap, state.pipe0.a, MOJO_HANDLE_SIGNAL_READABLE,
|
||||
+ MOJO_TRIGGER_CONDITION_SIGNALS_SATISFIED,
|
||||
+ state.pipe0.context(), nullptr));
|
||||
+ EXPECT_EQ(
|
||||
+ MOJO_RESULT_OK,
|
||||
+ MojoAddTrigger(state.trap, state.pipe1.a, MOJO_HANDLE_SIGNAL_READABLE,
|
||||
+ MOJO_TRIGGER_CONDITION_SIGNALS_SATISFIED,
|
||||
+ state.pipe1.context(), nullptr));
|
||||
+ EXPECT_EQ(MOJO_RESULT_OK, MojoArmTrap(state.trap, nullptr, nullptr, nullptr));
|
||||
+
|
||||
+ ThreadedRunner close_pipe1_b(base::BindLambdaForTesting([&] {
|
||||
+ state.event.Wait();
|
||||
+ MojoClose(state.pipe1.b);
|
||||
+ }));
|
||||
+ close_pipe1_b.Start();
|
||||
+
|
||||
+ // Trigger cancellation of the watch on `pipe0.a`. See event_handler above.
|
||||
+ MojoClose(state.pipe0.a);
|
||||
+
|
||||
+ close_pipe1_b.Join();
|
||||
+ MojoClose(state.pipe0.b);
|
||||
+ MojoClose(state.trap);
|
||||
+}
|
||||
+
|
||||
base::RepeatingClosure g_do_random_thing_callback;
|
||||
|
||||
void ReadAllMessages(const MojoTrapEvent* event) {
|
||||
Reference in New Issue
Block a user