Skip to content

Commit

Permalink
Fix Rust Mojo tests with ipcz enabled
Browse files Browse the repository at this point in the history
Addresses a few test failures that appeared when turning on
MojoIpcz by default.

1. ipcz data pipes were not properly guarding against an immediate read
   overlapping with a two-phase read.

2. The trap_signals_on_readable_unwritable and safe_trap tests were both
   testing traps that watch for unsatisfied signals, but this feature
   has no production uses and was therefore omitted from MojoIpcz. These
   tests have been updated to not exercise that feature, while
   preserving other coverage.

Fixed: 1418865
Change-Id: I9d1a1fb380292e6fdcaa6d08b8648e13a261e2f6
Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4289768
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1109617}
  • Loading branch information
krockot authored and Chromium LUCI CQ committed Feb 24, 2023
1 parent 9a09169 commit 8f4042d
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 16 deletions.
4 changes: 4 additions & 0 deletions mojo/core/ipcz_driver/data_pipe.cc
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,10 @@ MojoResult DataPipe::ReadData(void* elements,
scoped_refptr<PortalWrapper> portal;
{
base::AutoLock lock(lock_);
if (two_phase_reader_) {
return MOJO_RESULT_BUSY;
}

const size_t data_size = data_.data_size();
if (query) {
num_bytes = base::checked_cast<uint32_t>(data_size);
Expand Down
33 changes: 17 additions & 16 deletions mojo/public/rust/tests/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ tests! {
assert!(output[0].signals_state.satisfied().is_readable());
}

fn trap_signals_on_readable_unwritable() {
fn trap_signals_on_readable() {
// These tests unfortunately need global state, so we have to ensure
// exclusive access (generally Rust tests run on multiple threads).
let _test_lock = TRAP_TEST_LOCK.lock().unwrap();
Expand All @@ -227,8 +227,8 @@ tests! {
1));
assert_eq!(MojoResult::Okay,
trap.add_trigger(prod.get_native_handle(),
HandleSignals::WRITABLE,
TriggerCondition::SignalsUnsatisfied,
HandleSignals::PEER_CLOSED,
TriggerCondition::SignalsSatisfied,
2));

let mut blocking_events_buf = [std::mem::MaybeUninit::uninit(); 16];
Expand Down Expand Up @@ -281,10 +281,12 @@ tests! {
ArmResult::Failed(e) => panic!("unexpected Mojo error {:?}", e),
}

// Close `cons` making `prod` unwritable.
drop(cons);
// Close `prod` making `cons` permanently unreadable.
drop(prod);

// Now we should have two events indicating `prod` is not writable.
// Now we should have two events: one to indicate that cons will never
// be readable again, and one to indicate that `prod` has been closed
// and removed from the trap.
{
let list = wait_for_trap_events(TRAP_EVENT_LIST.lock().unwrap(), 1);
assert_eq!(list.len(), 2);
Expand All @@ -296,15 +298,14 @@ tests! {
(event2, event1)
};

// 1. `cons` was closed, yielding a `Cancelled` event.
// 1. `cons` can no longer be readable.
assert_eq!(cons_event.trigger_context(), 1);
assert_eq!(cons_event.result(), MojoResult::Cancelled);
assert_eq!(cons_event.result(), MojoResult::FailedPrecondition);
assert!(!cons_event.signals_state().satisfiable().is_readable());

// 2. `prod`'s trigger condition (being unwritable) was met,
// yielding a normal event.
// 2. `prod` was closed, yielding a `Cancelled` event.
assert_eq!(prod_event.trigger_context(), 2);
assert_eq!(prod_event.result(), MojoResult::Okay);
assert!(!prod_event.signals_state().satisfiable().is_writable())
assert_eq!(prod_event.result(), MojoResult::Cancelled);
};

drop(trap);
Expand Down Expand Up @@ -368,8 +369,8 @@ tests! {
context.clone());
let _prod_token = trap.add_trigger(
prod.get_native_handle(),
HandleSignals::WRITABLE,
TriggerCondition::SignalsUnsatisfied,
HandleSignals::PEER_CLOSED,
TriggerCondition::SignalsSatisfied,
context.clone());

assert_eq!(trap.arm(), MojoResult::Okay);
Expand All @@ -387,7 +388,7 @@ tests! {
events.clear();
}

// Close `cons` to get two events: unreadable on `prod`, and Cancelled on `cons`.
// Close `cons` to get two events: peer closure on `prod`, and Cancelled on `cons`.
let cons_native = cons.get_native_handle();
drop(cons);
{
Expand All @@ -410,7 +411,7 @@ tests! {
let event = events[0];
assert_eq!(event.handle(), prod.get_native_handle());
assert_eq!(event.result(), MojoResult::Okay);
assert!(!event.signals_state().satisfied().is_writable(),
assert!(event.signals_state().satisfied().is_peer_closed(),
"{:?}", event.signals_state());
events.clear();
}
Expand Down

0 comments on commit 8f4042d

Please sign in to comment.