Revert "MojoIpcz: Tweak DataPipe peer closure behavior"
This reverts commit ae9cdc10010bb22328956ed6c0087dab1ab0213b.
Reason for revert: Seems to be spiking spurious READABLE signals on
data pipe consumers. Since we have some consumers CHECKing against
that, the crash rate is spiking.
This also reverts enabling of MojoIpcz by default, since without
other changes in this CL, peer closure signaling is not quite right.
Original change's description:
> 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}
Bug: 1299283,1409259
Change-Id: I2be7fc50f7979c5b85ab3ea9159b7926c53e14c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4257573
Reviewed-by: Srinivas Sista <srinivassista@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1105993}
NOKEYCHECK=True
GitOrigin-RevId: c4c8d718853e0bc25eb8a74cff39a530286c8c94
diff --git a/src/ipcz/router.cc b/src/ipcz/router.cc
index d286a0c..6c9f921 100644
--- a/src/ipcz/router.cc
+++ b/src/ipcz/router.cc
@@ -304,12 +304,14 @@
}
if (!inward_edge_ && !bridge_) {
- is_peer_closed_ = true;
+ status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
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)) {
@@ -358,12 +360,14 @@
forwarding_links.push_back(bridge_->ReleaseDecayingLink());
} else {
// Terminal routers may have trap events to fire.
- is_peer_closed_ = true;
+ status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
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);
}
}
@@ -590,7 +594,7 @@
descriptor.next_incoming_sequence_number,
descriptor.num_bytes_consumed);
if (descriptor.peer_closed) {
- router->is_peer_closed_ = true;
+ router->status_.flags |= IPCZ_PORTAL_STATUS_PEER_CLOSED;
router->status_.num_remote_parcels = 0;
router->status_.num_remote_bytes = 0;
if (!router->inbound_parcels_.SetFinalSequenceLength(
@@ -818,7 +822,7 @@
// which can only happen after `descriptor` is transmitted.
inward_edge_.emplace();
- if (is_peer_closed_) {
+ if (status_.flags & IPCZ_PORTAL_STATUS_PEER_CLOSED) {
descriptor.peer_closed = true;
descriptor.closed_peer_sequence_length =
*inbound_parcels_.final_sequence_length();
@@ -1303,7 +1307,6 @@
bool outward_link_decayed = false;
bool dropped_last_decaying_link = false;
ParcelsToFlush parcels_to_flush;
- TrapEventDispatcher dispatcher;
{
absl::MutexLock lock(&mutex_);
@@ -1367,17 +1370,6 @@
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 28d1633..78c9f25 100644
--- a/src/ipcz/router.h
+++ b/src/ipcz/router.h
@@ -416,13 +416,6 @@
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_)};
