MojoIpcz: Tweak DataPipe peer closure behavior
Before this change, MojoIpcz emulated data pipe peer closure by having
it reflect whether the underlying control portal is dead or alive. This
is OK, but it's not strictly correct and it breaks some test
expectations when enabling MojoIpcz by default.
This CL corrects the behavior through a few combined changes:
- ipcz Routers no longer surface the peer closure status bit to
the application until all expected inbound parcels have been
received and queued in the router. This isn't strictly necessary,
but it eliminates ambiguity in the event of some subtly incorrect
Mojo data pipe API usage encountered in the wild.
- Data pipes no longer reflect portal live/dead status with their
peer closure signal; instead they reflect the portal's own peer
closure bit.
- Data pipes no longer serialize their peer closure state separately
in the driver object header; instead it is derived from the portal
status upon deserialization.
All of this is in preparation for enabling MojoIpcz by default in most
runtime environments and test suites on most platforms.
Bug: 1299283
Change-Id: I686a7bb24592aa4f240b5236e14031c40a7b3eda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4243001
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#1104822}
NOKEYCHECK=True
GitOrigin-RevId: ae9cdc10010bb22328956ed6c0087dab1ab0213b
diff --git a/src/ipcz/router.cc b/src/ipcz/router.cc
index 6c9f921..d286a0c 100644
--- a/src/ipcz/router.cc
+++ b/src/ipcz/router.cc
@@ -304,14 +304,12 @@
}
if (!inward_edge_ && !bridge_) {
- status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
+ is_peer_closed_ = true;
if (inbound_parcels_.IsSequenceFullyConsumed()) {
status_.flags |= IPCZ_PORTAL_STATUS_DEAD;
}
status_.num_remote_bytes = 0;
status_.num_remote_parcels = 0;
- traps_.UpdatePortalStatus(
- context, status_, TrapSet::UpdateReason::kPeerClosed, dispatcher);
}
} else if (link_type.is_peripheral_inward()) {
if (!outbound_parcels_.SetFinalSequenceLength(sequence_length)) {
@@ -360,14 +358,12 @@
forwarding_links.push_back(bridge_->ReleaseDecayingLink());
} else {
// Terminal routers may have trap events to fire.
- status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
+ is_peer_closed_ = true;
if (inbound_parcels_.IsSequenceFullyConsumed()) {
status_.flags |= IPCZ_PORTAL_STATUS_DEAD;
}
status_.num_remote_parcels = 0;
status_.num_remote_bytes = 0;
- traps_.UpdatePortalStatus(context, status_,
- TrapSet::UpdateReason::kPeerClosed, dispatcher);
}
}
@@ -594,7 +590,7 @@
descriptor.next_incoming_sequence_number,
descriptor.num_bytes_consumed);
if (descriptor.peer_closed) {
- router->status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
+ router->is_peer_closed_ = true;
router->status_.num_remote_parcels = 0;
router->status_.num_remote_bytes = 0;
if (!router->inbound_parcels_.SetFinalSequenceLength(
@@ -822,7 +818,7 @@
// which can only happen after `descriptor` is transmitted.
inward_edge_.emplace();
- if (status_.flags & IPCZ_PORTAL_STATUS_PEER_CLOSED) {
+ if (is_peer_closed_) {
descriptor.peer_closed = true;
descriptor.closed_peer_sequence_length =
*inbound_parcels_.final_sequence_length();
@@ -1307,6 +1303,7 @@
bool outward_link_decayed = false;
bool dropped_last_decaying_link = false;
ParcelsToFlush parcels_to_flush;
+ TrapEventDispatcher dispatcher;
{
absl::MutexLock lock(&mutex_);
@@ -1370,6 +1367,17 @@
bridge_.reset();
}
+ if (is_peer_closed_ &&
+ (status_.flags & IPCZ_PORTAL_STATUS_PEER_CLOSED) == 0 &&
+ !inbound_parcels_.ExpectsMoreElements()) {
+ // Set the PEER_CLOSED bit and trigger any relevant traps, if and only if
+ // the peer is actually closed and there are no more inbound parcels in
+ // flight towards us.
+ status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
+ traps_.UpdatePortalStatus(context, status_,
+ TrapSet::UpdateReason::kPeerClosed, dispatcher);
+ }
+
// If we're dropping the last of our decaying links, our outward link may
// now be stable. This may unblock proxy bypass or other operations.
const bool inward_edge_stable =
diff --git a/src/ipcz/router.h b/src/ipcz/router.h
index 78c9f25..28d1633 100644
--- a/src/ipcz/router.h
+++ b/src/ipcz/router.h
@@ -416,6 +416,13 @@
absl::Mutex mutex_;
+ // Indicates whether the opposite end of the route has been closed. This is
+ // the source of truth for peer closure status. The status bit
+ // (IPCZ_PORTAL_STATUS_PEER_CLOSED) within `status_`, and the corresponding
+ // trap condition (IPCZ_TRAP_PEER_CLOSED) are only raised when this is true
+ // AND we are not expecting any more in-flight parcels.
+ bool is_peer_closed_ ABSL_GUARDED_BY(mutex_) = false;
+
// The current computed portal status to be reflected by a portal controlling
// this router, iff this is a terminal router.
IpczPortalStatus status_ ABSL_GUARDED_BY(mutex_) = {sizeof(status_)};
