diff --git a/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp b/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp index 39dbebd1..1d0bbd0a 100644 --- a/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp +++ b/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp @@ -65,6 +65,7 @@ namespace MobileGL::MG_Remote { case SurfaceWireError::InprocOnlyOpOnTheWire: return "InprocOnlyOpOnTheWire"; case SurfaceWireError::WindowKindNamesNoBackend: return "WindowKindNamesNoBackend"; case SurfaceWireError::UnknownWindowKind: return "UnknownWindowKind"; + case SurfaceWireError::MetalLayerArrived: return "MetalLayerArrived"; case SurfaceWireError::AndroidNativeWindowArrived: return "AndroidNativeWindowArrived"; } return ""; @@ -159,6 +160,10 @@ namespace MobileGL::MG_Remote { // arrival is P12; until then this is refused by name at ServerApplyWireSurfaceOp. return SurfaceWireError::AndroidNativeWindowArrived; } + if (op.windowKind() == ::MobileGL::Wire::WindowKind::MetalLayer) { + // CAMetalLayer* is a client-process object address, just like ANativeWindow*. + return SurfaceWireError::MetalLayerArrived; + } MG_Backend::WindowBackend backend; if (!WindowBackendForWireWindowKind(op.windowKind(), &backend)) { return ::flatbuffers::IsOutRange(op.windowKind(), ::MobileGL::Wire::WindowKind::None, @@ -199,6 +204,10 @@ namespace MobileGL::MG_Remote { "CLIENT's process and means nothing here. Real window arrival is P12; " "until then the spawn surface path is pbuffer/surfaceless only", ::MobileGL::Wire::EnumNameSurfaceOpKind(op.kind())); + } else if (error == SurfaceWireError::MetalLayerArrived) { + MGLOG_F("MGPipe: Fatal{UnmigratedSurface, \"MetalLayer@P12\"} - a wire " + "SurfaceOp named a CAMetalLayer* in the CLIENT's process. " + "Real window arrival is P12"); } else { MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"SurfaceOp\"} - a wire surface op " "failed validation: %s (wire kind %u, window kind %u)", diff --git a/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.h b/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.h index 691111bd..58b73568 100644 --- a/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.h +++ b/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.h @@ -7,17 +7,17 @@ // End of Source File Header // P5f, package fc: the codec between the inproc SurfaceControlFrame and the wire's -// Wire::SurfaceOp / Wire::SurfaceReply (protocol.fbs). +// ::MobileGL::Wire::SurfaceOp / ::MobileGL::Wire::SurfaceReply (protocol.fbs). // // The frame is the inproc payload (Server/SurfaceControlFrame.h); this is the ONLY place the // frame meets the schema. Nothing in the inproc hot path encodes - the frame crosses the one-slot // channel by value - but the codec is what makes "the same struct encodes under spawn" a tested // claim rather than a plan, and it owns the two schema seams that are NOT integer translations: // -// * SurfaceControlOp <-> Wire::SurfaceOpKind: numerically aligned for the ten framed ops and +// * SurfaceControlOp <-> ::MobileGL::Wire::SurfaceOpKind: numerically aligned for the ten framed ops and // PINNED by static_asserts in the .cpp, but mapped through an explicit table. The four // inproc-only kinds have no wire kind and refuse to encode by name. -// * MG_Backend::WindowBackend <-> Wire::WindowKind: the two enums do not agree (WindowKind's +// * MG_Backend::WindowBackend <-> ::MobileGL::Wire::WindowKind: the two enums do not agree (WindowKind's // Surfaceless/Pbuffer are surface shapes, not window backends; WindowBackend's MetalLayer had // no wire enumerator until fc added it), so the mapping is a table with named failures, never // a cast (f0-egl census, schema gap 3). @@ -28,7 +28,8 @@ // Fatal{UnmigratedSurface, "AndroidNativeWindow@P12"} (the ANativeWindow* means nothing in the // server's process; real window arrival is P12) and everything else dies // Fatal{ProtocolCorruption, "SurfaceOp"}. P6's control pump is the intended caller; P5f's unit -// tests drive it directly. +// tests drive it directly. MetalLayerArrived likewise dies +// Fatal{UnmigratedSurface, "MetalLayer@P12"}: CAMetalLayer* is also process-local. #pragma once #include @@ -53,6 +54,7 @@ namespace MobileGL::MG_Remote { InprocOnlyOpOnTheWire, // a kind that may ride the inproc channel but never the wire WindowKindNamesNoBackend,// Surfaceless/Pbuffer where a window backend is required UnknownWindowKind, // a WindowKind the mapping table does not know + MetalLayerArrived, // -> Fatal{UnmigratedSurface, "MetalLayer@P12"} AndroidNativeWindowArrived, // -> Fatal{UnmigratedSurface, "AndroidNativeWindow@P12"} }; @@ -60,15 +62,15 @@ namespace MobileGL::MG_Remote { // frame.kind <-> wire kind. Both answer false (and leave *out untouched) for None, the four // inproc-only kinds, and any wire value the schema does not define. - bool WireKindForSurfaceControlOp(Server::SurfaceControlOp op, Wire::SurfaceOpKind* out); - bool SurfaceControlOpForWireKind(Wire::SurfaceOpKind kind, Server::SurfaceControlOp* out); + bool WireKindForSurfaceControlOp(Server::SurfaceControlOp op, ::MobileGL::Wire::SurfaceOpKind* out); + bool SurfaceControlOpForWireKind(::MobileGL::Wire::SurfaceOpKind kind, Server::SurfaceControlOp* out); // The explicit WindowBackend <-> WindowKind table. WireWindowKindForWindowBackend also maps // WindowBackend::Unknown to WindowKind::None; WindowBackendForWireWindowKind maps // WindowKind::None back to Unknown and answers FALSE for Surfaceless/Pbuffer (surface shapes, // not window backends). - bool WireWindowKindForWindowBackend(MG_Backend::WindowBackend backend, Wire::WindowKind* out); - bool WindowBackendForWireWindowKind(Wire::WindowKind kind, MG_Backend::WindowBackend* out); + bool WireWindowKindForWindowBackend(MG_Backend::WindowBackend backend, ::MobileGL::Wire::WindowKind* out); + bool WindowBackendForWireWindowKind(::MobileGL::Wire::WindowKind kind, MG_Backend::WindowBackend* out); // Encode one frame as a complete CtrlEnvelope{SurfaceOp} buffer (the builder is left // Finished). Fails BEFORE touching the builder for a kind that may not cross. @@ -77,17 +79,17 @@ namespace MobileGL::MG_Remote { // Validate + decode a wire SurfaceOp into a frame. Pure - it never aborts; the Fatal // decisions belong to ServerApplyWireSurfaceOp. The reply half of the frame is reset. - SurfaceWireError DecodeWireSurfaceOp(const Wire::SurfaceOp& op, Server::SurfaceControlFrame* frame); + SurfaceWireError DecodeWireSurfaceOp(const ::MobileGL::Wire::SurfaceOp& op, Server::SurfaceControlFrame* frame); // The reply direction, same shape (CtrlEnvelope{SurfaceReply}). void EncodeSurfaceReplyFrame(const Server::SurfaceControlFrame& frame, flatbuffers::FlatBufferBuilder* builder); - void DecodeWireSurfaceReply(const Wire::SurfaceReply& reply, Server::SurfaceControlFrame* frame); + void DecodeWireSurfaceReply(const ::MobileGL::Wire::SurfaceReply& reply, Server::SurfaceControlFrame* frame); // The server-side wire entry: decode, then post the frame through ServerLoop's channel and // hand the reply half back. A decode error is a NAMED Fatal (see the header block), never a // dropped message. No transport calls this yet - P6's control pump will. - MobileGLResult ServerApplyWireSurfaceOp(const Wire::SurfaceOp& op, + MobileGLResult ServerApplyWireSurfaceOp(const ::MobileGL::Wire::SurfaceOp& op, Server::SurfaceControlFrame* replyOut); } // namespace MobileGL::MG_Remote diff --git a/MobileGL/MG_Remote/Server/ServerLoop.cpp b/MobileGL/MG_Remote/Server/ServerLoop.cpp index 748bf13e..a2017266 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.cpp +++ b/MobileGL/MG_Remote/Server/ServerLoop.cpp @@ -562,7 +562,9 @@ namespace MobileGL::MG_Remote::Server { // The dispatch runs OUTSIDE the lock, exactly as the old work(user) did, and fills the // frame's reply half; the reply is then published back into the slot under the lock so // the poster's copy-out after m_controlDone sees it. - const MobileGLResult result = ApplySurfaceControlFrame(frame); + const MobileGLResult result = ApplySurfaceControlFrame( + frame, m_controlProbeHook.load(std::memory_order_acquire), + m_controlProbeUser.load(std::memory_order_acquire)); { const std::lock_guard lock(m_controlMutex); m_controlFrame = frame; @@ -650,10 +652,9 @@ namespace MobileGL::MG_Remote::Server { MobileGLResult ServerLoop::RunSurfaceControlFrame(SurfaceControlFrame& frame) { if (frame.kind == SurfaceControlOp::None) return MOBILEGL_ERR_INVALID_ARGUMENT; - // Minted here and echoed back with the reply half: inproc it is purely diagnostic; under - // spawn it is how a SurfaceReply names its op. Atomic rather than under m_callerMutex - // because the re-entrant arm below mints without taking that lock. - frame.seq = m_controlSeq.fetch_add(1, std::memory_order_acq_rel) + 1; + // Zero marks an unnumbered local request. A wire request is already numbered by its + // client and MUST retain that number all the way through dispatch and the reply. + if (frame.seq == 0) frame.seq = m_controlSeq.fetch_add(1, std::memory_order_acq_rel) + 1; // RE-ENTRANCY IS NOT A DEADLOCK. The teardown path posts from the apply thread itself - // ~BackendObject_DirectGLES reaches ReleaseEGLResources - and so does anything the // applier calls that wants "run this where the context is". Running inline is the @@ -661,6 +662,10 @@ namespace MobileGL::MG_Remote::Server { if (OnApplyThread()) return ApplySurfaceControlFrame(frame); const std::lock_guard callerLock(m_callerMutex); + return PostSurfaceControlFrameWithCallerLock(frame); + } + + MobileGLResult ServerLoop::PostSurfaceControlFrameWithCallerLock(SurfaceControlFrame& frame) { std::unique_lock lock(m_controlMutex); // C2 + M-7: the m_running check and the publish are ONE critical section, and m_running is // cleared under this same lock on the way out (ApplyThreadMain's exit block), so this read @@ -717,14 +722,16 @@ namespace MobileGL::MG_Remote::Server { } MobileGLResult ServerLoop::RunProbeOnApplyThreadForTesting(ControlProbeHook hook, void* user) { - // The hook pair is published BEFORE the frame: the mailbox handshake (publish under - // m_controlMutex, shadow last, then the bell) orders these two relaxed-typed stores for - // the apply thread, which reads them inside the dispatch. - m_controlProbeHook.store(hook, std::memory_order_release); - m_controlProbeUser.store(user, std::memory_order_release); SurfaceControlFrame frame; frame.kind = SurfaceControlOp::ProbeForTesting; - return RunSurfaceControlFrame(frame); + frame.seq = m_controlSeq.fetch_add(1, std::memory_order_acq_rel) + 1; + // Nested probes never touch the shared pair: another poster may already be waiting, + // and its arguments must survive a probe run inline on the apply thread. + if (OnApplyThread()) return ApplySurfaceControlFrame(frame, hook, user); + const std::lock_guard callerLock(m_callerMutex); + m_controlProbeHook.store(hook, std::memory_order_release); + m_controlProbeUser.store(user, std::memory_order_release); + return PostSurfaceControlFrameWithCallerLock(frame); } Uint64 ServerLoop::ControlFramesDispatched() const { @@ -870,15 +877,15 @@ namespace MobileGL::MG_Remote::Server { } // namespace - MobileGLResult ServerLoop::ApplySurfaceControlFrame(SurfaceControlFrame& frame) { + MobileGLResult ServerLoop::ApplySurfaceControlFrame(SurfaceControlFrame& frame, + ControlProbeHook probeHook, void* probeUser) { // The frame channel's own tally, bumped for EVERY dispatch including probes: a forwarder // that stopped posting frames (the fc red-once revert shape) leaves this unmoved, which // is what ServerLoopTest's AVoidForwarderCrossesAsOneDispatchedFrame reads. m_controlFramesDispatched.fetch_add(1, std::memory_order_acq_rel); if (frame.kind == SurfaceControlOp::ProbeForTesting) { - const ControlProbeHook hook = m_controlProbeHook.load(std::memory_order_acquire); - if (hook == nullptr) return MOBILEGL_ERR_INVALID_ARGUMENT; - return hook(m_controlProbeUser.load(std::memory_order_acquire)); + if (probeHook == nullptr) return MOBILEGL_ERR_INVALID_ARGUMENT; + return probeHook(probeUser); } // Null means the hook never ran or the session is already down. Every op answers // NOT_INITIALIZED rather than dereferencing: a client that calls eglMakeCurrent on a @@ -970,6 +977,7 @@ namespace MobileGL::MG_Remote::Server { } case SurfaceControlOp::SetSwapInterval: backend->SetEGLSwapInterval(frame.swapInterval); + frame.ok = true; return MOBILEGL_OK; case SurfaceControlOp::ReleaseSurface: { const EGLSurface surface = HandleFromToken(frame.surface); @@ -980,6 +988,7 @@ namespace MobileGL::MG_Remote::Server { // costs one forwarded bind; remembering when it did not would cost a silent // no-context-current. ForgetCurrentTupleIfItNames(surface); + frame.ok = true; return MOBILEGL_OK; } case SurfaceControlOp::ReleaseResources: @@ -994,9 +1003,11 @@ namespace MobileGL::MG_Remote::Server { // by deleting it: ServerLoopTest's recreate control reads NativeBindCount() == 1 // where 2 is required. ForgetCurrentTuple(); + frame.ok = true; return MOBILEGL_OK; case SurfaceControlOp::SetWindowHandle: backend->SetWindowHandle(UnpackWindowHandle(frame)); + frame.ok = true; return MOBILEGL_OK; case SurfaceControlOp::InitCapabilitiesInprocOnly: // Inproc-only (f0-egl §4.2): on the wire the ANSWER to this op is the CapsSnapshot diff --git a/MobileGL/MG_Remote/Server/ServerLoop.h b/MobileGL/MG_Remote/Server/ServerLoop.h index f5c560c1..edfa097b 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.h +++ b/MobileGL/MG_Remote/Server/ServerLoop.h @@ -370,7 +370,11 @@ namespace MobileGL::MG_Remote::Server { // The dispatch, on the apply thread (or inline for a re-entrant post): executes the // frame's op against the private backend and fills the frame's reply half. The eleven // wire+inproc op bodies from the old forwarder Args structs live here now. - MobileGLResult ApplySurfaceControlFrame(SurfaceControlFrame& frame); + MobileGLResult ApplySurfaceControlFrame(SurfaceControlFrame& frame, + ControlProbeHook probeHook = nullptr, void* probeUser = nullptr); + // The caller already holds m_callerMutex; shared by normal and test-probe posting so + // the probe's metadata is protected for the same lifetime as its frame. + MobileGLResult PostSurfaceControlFrameWithCallerLock(SurfaceControlFrame& frame); // Pops and applies every record currently in the ring; returns how many it applied. Uint64 DrainRing(); void SignalExited(); @@ -418,8 +422,8 @@ namespace MobileGL::MG_Remote::Server { std::atomic m_controlSeq{0}; std::atomic m_controlFramesDispatched{0}; // The test probe (RunProbeOnApplyThreadForTesting). Members, NOT slot content - the slot - // carries only the frame. Written before the probe frame is published, so the mailbox's - // own release/acquire handshake orders them for the apply thread. + // carries only the frame. Written under m_callerMutex before the probe frame is + // published; held until its reply returns. Re-entrant probes use call-local arguments. std::atomic m_controlProbeHook{nullptr}; std::atomic m_controlProbeUser{nullptr}; diff --git a/MobileGL/MG_Remote/Server/SurfaceControlFrame.h b/MobileGL/MG_Remote/Server/SurfaceControlFrame.h index 62145b0a..7aef9b82 100644 --- a/MobileGL/MG_Remote/Server/SurfaceControlFrame.h +++ b/MobileGL/MG_Remote/Server/SurfaceControlFrame.h @@ -16,9 +16,9 @@ // blocking, one-slot) channel by value; under spawn the SAME struct is what SurfaceOpCodec // encodes into a Wire::SurfaceOp. The thread model does not move - only the payload's shape. // -// THE FRAME CARRIES NO POINTERS, and that is enforced by construction (the static_assert below) -// rather than by review: a field that is a pointer would also be a compile error here. The three -// places the old Args structs held client addresses map onto values: +// THE FRAME CARRIES NO POINTERS, and that is enforced by the exhaustive member check below: +// adding a member requires updating its structured binding; pointer types fail the assertion. +// The three places the old Args structs held client addresses map onto values: // // ServerInitializeEGLDisplay's major/minor out-pointers -> the reply fields eglMajor/eglMinor; // ServerCreateEGLWindowSurface / ServerSetWindowHandle's const WindowHandle* -> windowBackend + @@ -28,10 +28,9 @@ // under spawn the client mints dense tokens instead (P6-CONTRACT-DRAFT table 0) - the field // width is already the token's, so nothing here changes shape on that day. // -// AN ANativeWindow* ON THE WIRE IS REFUSED BY NAME, not carried: nativeToken is only meaningful -// inside the client's process for AndroidNativeWindow, and a spawn-side decode of that kind dies -// with Fatal{UnmigratedSurface, "AndroidNativeWindow@P12"} (SurfaceOpCodec). Real window arrival -// is P12. +// PROCESS-LOCAL WINDOW OBJECTS ARE REFUSED BY NAME on the wire: ANativeWindow* and +// CAMetalLayer* mean nothing in the server process. SurfaceOpCodec names their refusals +// AndroidNativeWindow@P12 and MetalLayer@P12; real window arrival is P12. #pragma once #include @@ -93,13 +92,34 @@ namespace MobileGL::MG_Remote::Server { Int eglMinor = 0; }; - // THE INVARIANT THE PACKAGE EXISTS FOR. The frame crosses by VALUE - copied into the one-slot - // channel inproc, encoded into a FlatBuffers table under spawn - so it can never carry an - // address: no pointer members, no ownership, trivially copyable, standard layout. + namespace Detail { + // Pointers are trivially copyable and standard-layout too. Only these value types are + // allowed; the constexpr negatives pin the distinction from the old, insufficient check. + template + inline constexpr bool IsSurfaceControlValue = std::is_integral_v || std::is_enum_v; + static_assert(!IsSurfaceControlValue && !IsSurfaceControlValue); + + constexpr bool SurfaceControlFrameHasOnlyValues() { + SurfaceControlFrame frame{}; + // Exhaustive by the language's aggregate decomposition rule: adding ANY member + // without extending this binding is a compile error, so new fields cannot evade it. + const auto& [kind, seq, display, surface, readSurface, context, windowBackend, + nativeToken, width, height, swapInterval, ok, eglMajor, eglMinor] = frame; + return IsSurfaceControlValue && IsSurfaceControlValue && + IsSurfaceControlValue && IsSurfaceControlValue && + IsSurfaceControlValue && IsSurfaceControlValue && + IsSurfaceControlValue && IsSurfaceControlValue && + IsSurfaceControlValue && IsSurfaceControlValue && + IsSurfaceControlValue && IsSurfaceControlValue && + IsSurfaceControlValue && IsSurfaceControlValue; + } + } // namespace Detail + + static_assert(Detail::SurfaceControlFrameHasOnlyValues(), + "every surface control frame member must be an integer or enum, never a pointer"); static_assert(std::is_trivially_copyable_v && std::is_standard_layout_v, - "the surface control frame must stay a plain bag of scalars - a pointer member " - "would be a client address crossing to the server, which is what P5f removes"); + "the surface control frame must remain a trivially copyable value aggregate"); const char* SurfaceControlOpName(SurfaceControlOp op); diff --git a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp index 4f8ff1c7..4d062139 100644 --- a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp @@ -42,6 +42,7 @@ #include #include #include +#include #include #include #include @@ -466,6 +467,72 @@ TEST(ServerLoopTest, APostFromTheApplyThreadItselfRunsInlineRatherThanDeadlockin fixture.Stop(); } +TEST(ServerLoopTest, WireSequenceSurvivesRealDispatchAndFailureReply) { + ServerFixture fixture; + ASSERT_TRUE(fixture.Handshake()); + ASSERT_TRUE(fixture.StartLoop()); + flatbuffers::FlatBufferBuilder builder(256); + const auto op = ::MobileGL::Wire::CreateSurfaceOp( + builder, 4242, ::MobileGL::Wire::SurfaceOpKind::SetSwapInterval); + const auto envelope = ::MobileGL::Wire::CreateCtrlEnvelope( + builder, ::MobileGL::Wire::CtrlMsg::SurfaceOp, op.Union()); + ::MobileGL::Wire::FinishCtrlEnvelopeBuffer(builder, envelope); + Server::SurfaceControlFrame reply; + EXPECT_EQ(MG_Remote::ServerApplyWireSurfaceOp( + *::MobileGL::Wire::GetCtrlEnvelope(builder.GetBufferPointer())->msg_as_SurfaceOp(), &reply), + MOBILEGL_ERR_NOT_INITIALIZED); + EXPECT_EQ(Server::ServerLoopInstance().ControlFramesDispatched(), 1u); + flatbuffers::FlatBufferBuilder replies(256); + MG_Remote::EncodeSurfaceReplyFrame(reply, &replies); + const auto* wireReply = ::MobileGL::Wire::GetCtrlEnvelope(replies.GetBufferPointer())->msg_as_SurfaceReply(); + EXPECT_EQ(wireReply->seq(), 4242u); + EXPECT_FALSE(wireReply->ok()); + fixture.Stop(); +} + +TEST(ServerLoopTest, ConcurrentProbesKeepTheirArgumentsAcrossNestedProbes) { + ServerFixture fixture; + ASSERT_TRUE(fixture.Handshake()); + ASSERT_TRUE(fixture.StartLoop()); + constexpr int kPosters = 8; + constexpr int kCalls = 128; + struct Probe { + std::atomic calls{0}; + std::atomic nested{0}; + std::atomic errors{0}; + } probes[kPosters]; + std::atomic go{false}; + std::vector posters; + for (auto& probe : probes) { + posters.emplace_back([&go, &probe] { + while (!go.load(std::memory_order_acquire)) std::this_thread::yield(); + for (int i = 0; i < kCalls; ++i) { + const auto rc = Server::ServerLoopInstance().RunProbeOnApplyThreadForTesting( + +[](void* user) -> MobileGLResult { + auto* p = static_cast(user); + p->calls.fetch_add(1); + // Same argument type keeps a regressed mixed pair observable without UB. + return Server::ServerLoopInstance().RunProbeOnApplyThreadForTesting( + +[](void* nestedUser) -> MobileGLResult { + static_cast(nestedUser)->nested.fetch_add(1); + return MOBILEGL_OK; + }, p); + }, &probe); + if (rc != MOBILEGL_OK) probe.errors.fetch_add(1); + } + }); + } + go.store(true, std::memory_order_release); + for (auto& poster : posters) poster.join(); + for (const auto& probe : probes) { + EXPECT_EQ(probe.calls.load(), kCalls) << "another poster or nested probe replaced this hook/user pair"; + EXPECT_EQ(probe.nested.load(), kCalls); + EXPECT_EQ(probe.errors.load(), 0); + } + EXPECT_EQ(Server::ServerLoopInstance().ControlFramesDispatched(), 2u * kPosters * kCalls); + fixture.Stop(); +} + // The bounded join. The thread is parked on kWaitForever when Stop() is called, so this is the // exact lost-wakeup shape table 3 step 2 is about - and the bound is what turns a regression // into a red test rather than a wedged CI job. @@ -1454,6 +1521,35 @@ namespace { } // namespace +TEST(ServerLoopEglTest, SuccessfulVoidWireOperationsReplyWithSuccessAndOriginalSequence) { + EglServerFixture fixture; + MGL_EGL_BRING_UP_OR_BAIL(fixture); + // Release last: it deliberately destroys the native context used by the preceding ops. + const auto kinds = {::MobileGL::Wire::SurfaceOpKind::SetSwapInterval, + ::MobileGL::Wire::SurfaceOpKind::SetWindowHandle, + ::MobileGL::Wire::SurfaceOpKind::ReleaseSurface, + ::MobileGL::Wire::SurfaceOpKind::ReleaseResources}; + Uint64 seq = 9000; + for (const auto kind : kinds) { + flatbuffers::FlatBufferBuilder builder(256); + const auto op = ::MobileGL::Wire::CreateSurfaceOp( + builder, ++seq, kind, 0, 0, ::MobileGL::Wire::WindowKind::None, 0, 0, 0, 1); + const auto envelope = ::MobileGL::Wire::CreateCtrlEnvelope( + builder, ::MobileGL::Wire::CtrlMsg::SurfaceOp, op.Union()); + ::MobileGL::Wire::FinishCtrlEnvelopeBuffer(builder, envelope); + Server::SurfaceControlFrame reply; + EXPECT_EQ(MG_Remote::ServerApplyWireSurfaceOp( + *::MobileGL::Wire::GetCtrlEnvelope(builder.GetBufferPointer())->msg_as_SurfaceOp(), &reply), + MOBILEGL_OK) << static_cast(kind); + flatbuffers::FlatBufferBuilder replies(256); + MG_Remote::EncodeSurfaceReplyFrame(reply, &replies); + const auto* wireReply = ::MobileGL::Wire::GetCtrlEnvelope(replies.GetBufferPointer())->msg_as_SurfaceReply(); + EXPECT_EQ(wireReply->seq(), seq) << static_cast(kind); + EXPECT_TRUE(wireReply->ok()) << "successful void operation encoded failure: " << static_cast(kind); + } + fixture.TearDown(); +} + // C7 / ID-54, THE NATIVE HALF, measured where the review said it was not: at the driver. Surface // creation binds the context natively on the apply thread (that is bind #1 of the "2 per process" // the review counted); the client's first eglMakeCurrent onto that surface used to be bind #2 and is diff --git a/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp b/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp index 306e502d..5f935d5c 100644 --- a/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp +++ b/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp @@ -99,6 +99,17 @@ TEST(SurfaceControlFrameTest, TheFrameIsAPlainBagOfScalars) { "shapes must stay obvious to a reader"; } +TEST(SurfaceControlFrameTest, EveryMemberIsMechanicallyCheckedAsAnIntegerOrEnum) { + static_assert(Server::Detail::SurfaceControlFrameHasOnlyValues()); + static_assert(!Server::Detail::IsSurfaceControlValue); + static_assert(!Server::Detail::IsSurfaceControlValue); + EXPECT_TRUE(Server::Detail::SurfaceControlFrameHasOnlyValues()); + // Why trivially-copyable alone did not enforce this invariant. + struct PointerBag { void* pointer; }; + static_assert(std::is_trivially_copyable_v && std::is_standard_layout_v); + EXPECT_FALSE(Server::Detail::IsSurfaceControlValue); +} + TEST(SurfaceControlFrameTest, ExactlyTheTenFramedOpsHaveWireKindsAndTheValuesArePinned) { // The ten framed ops, in schema order. The numeric agreement with ::MobileGL::Wire::SurfaceOpKind is // asserted per op rather than trusted from the static_asserts, because THIS is the table a @@ -322,6 +333,25 @@ TEST(SurfaceControlFrameTest, AnAndroidNativeWindowOnTheWireIsRefusedByName) { // A kind the schema does not define (a NEWER client's op reaching an OLDER server, or // corruption) is Fatal{ProtocolCorruption}, never ignored: an ignored control op is a client // waiting on a reply that never comes, which is a hang wearing a green lane. +TEST(SurfaceControlFrameTest, AMetalLayerOnTheWireIsRefusedByName) { + // Both operations that consume a native window must refuse the process-local object. + for (const auto kind : {::MobileGL::Wire::SurfaceOpKind::CreateWindowSurface, + ::MobileGL::Wire::SurfaceOpKind::SetWindowHandle}) { + flatbuffers::FlatBufferBuilder builder(256); + const auto op = ::MobileGL::Wire::CreateSurfaceOp( + builder, 19, kind, 1, 2, ::MobileGL::Wire::WindowKind::MetalLayer, 0x1234, 800, 600); + const auto envelope = ::MobileGL::Wire::CreateCtrlEnvelope( + builder, ::MobileGL::Wire::CtrlMsg::SurfaceOp, op.Union()); + ::MobileGL::Wire::FinishCtrlEnvelopeBuffer(builder, envelope); + const auto* wireOp = ::MobileGL::Wire::GetCtrlEnvelope(builder.GetBufferPointer())->msg_as_SurfaceOp(); + ASSERT_NE(wireOp, nullptr); + SurfaceControlFrame frame; + EXPECT_EQ(DecodeWireSurfaceOp(*wireOp, &frame), SurfaceWireError::MetalLayerArrived); + EXPECT_EXIT(ServerApplyWireSurfaceOp(*wireOp, nullptr), ::testing::KilledBySignal(SIGABRT), ".*"); + EXPECT_NE(ReadLog().find("Fatal{UnmigratedSurface, \"MetalLayer@P12\"}"), std::string::npos); + } +} + TEST(SurfaceControlFrameTest, AnUnknownWireOpKindIsProtocolCorruptionByName) { flatbuffers::FlatBufferBuilder builder(256); const auto op = ::MobileGL::Wire::CreateSurfaceOp(builder, 9, static_cast<::MobileGL::Wire::SurfaceOpKind>(200), 1, 2, diff --git a/docs/Disaggregated/notes/p5f/fc-contract-draft.md b/docs/Disaggregated/notes/p5f/fc-contract-draft.md index c67ac89f..9db265de 100644 --- a/docs/Disaggregated/notes/p5f/fc-contract-draft.md +++ b/docs/Disaggregated/notes/p5f/fc-contract-draft.md @@ -17,11 +17,13 @@ slot's content passes to the channel at publish; the dispatch on the apply thread fills the reply half (`ok`, `eglMajor`, `eglMinor`) into the same value; the poster's copy-out happens after the blocking handshake returns. No field is a pointer and none may become one — the - `static_assert(std::is_trivially_copyable_v<...>)` in `SurfaceControlFrame.h` is the - enforcement, not a courtesy. -2. `seq` is minted by the channel's poster side (`RunSurfaceControlFrame`), monotonically from 1 - per session, and echoed in the reply. Under spawn the client mints (P6-CONTRACT-DRAFT table 0); - the field does not change shape. + exhaustive constexpr structured binding in `SurfaceControlFrame.h` checks every member + as an integer or enum. Adding a member without extending the binding fails compilation; + changing a checked member to a pointer fails its static assertion. Trivially-copyable / + standard-layout remain separate copyability checks: neither trait rejects pointers. +2. An unnumbered local request (`seq == 0`) is numbered by `RunSurfaceControlFrame`, from 1 + per session. An already numbered wire request retains its client-minted sequence through + dispatch and reply (P6-CONTRACT-DRAFT table 0); the server never renumbers it. 3. `display` / `surface` / `readSurface` / `context` are Uint64 **tokens**. Inproc they are the client's EGL handle values bit-cast — legal only because both roles share a process and a driver, and the N-3 tuple bookkeeping compares them by value. Under spawn they are @@ -31,7 +33,8 @@ names the **backend** enum, never the wire's `WindowKind`; the two enums meet only inside `SurfaceOpCodec.cpp`'s explicit tables (`Surfaceless`/`Pbuffer` are surface shapes and answer "no backend" there). A `nativeToken` whose backend is Android is an `ANativeWindow*` — an - address in the client's process — and may never cross (§3.4). + address in the client's process — and may never cross (§3.5). MetalLayer carries a + `CAMetalLayer*` and has the same restriction. ## 2. Ordering @@ -58,16 +61,22 @@ unchanged). 3. Backend null at dispatch → `MOBILEGL_ERR_NOT_INITIALIZED` with `ok=false`; the forwarder's answer is the old `false`/void-drop, unchanged. -4. Wire entry (`ServerApplyWireSurfaceOp`, the future P6 control pump's entry point; P5f drives +4. Successful void operations (`SetSwapInterval`, `ReleaseSurface`, `ReleaseResources`, + `SetWindowHandle`) reply `ok=true` after completing on the apply thread; a null backend + replies `ok=false`. The wire therefore preserves the success/failure distinction even + though the inproc forwarder returns void. +5. Wire entry (`ServerApplyWireSurfaceOp`, the future P6 control pump's entry point; P5f drives it from tests only): a decode failure is **`Fatal{ProtocolCorruption, "SurfaceOp"}`**, never a dropped frame (a dropped control frame is a client waiting on a reply that never comes); a decoded `WindowKind::AndroidNativeWindow` is **`Fatal{UnmigratedSurface, "AndroidNativeWindow@P12"}`** — the window kind is legal schema, the token is the part that means nothing in the server's process, and real window arrival is - P12. Inproc-only kinds (`InitCapabilitiesInprocOnly`, `SwapBuffersInprocOnly`, + P12. A decoded `MetalLayer` is similarly **`Fatal{UnmigratedSurface, "MetalLayer@P12"}`**, + because its `CAMetalLayer*` also belongs to the client process. Inproc-only kinds + (`InitCapabilitiesInprocOnly`, `SwapBuffersInprocOnly`, `InitWindowSurfaceInprocOnly`, `ProbeForTesting`) cannot arrive: the encoder refuses them (`SurfaceWireError::InprocOnlyOpOnTheWire`). -5. A frame that reaches the inproc dispatch with `kind == None` or a kind the switch does not know +6. A frame that reaches the inproc dispatch with `kind == None` or a kind the switch does not know is `Fatal{ProtocolCorruption, "SurfaceOp.kind"}`; a `windowBackend` tag outside the enum is `Fatal{ProtocolCorruption, "SurfaceOp.windowBackend"}`. Inproc these are corruption shapes, not bad input — same discipline as the ring's header check. diff --git a/docs/Disaggregated/notes/p5f/fc-report.md b/docs/Disaggregated/notes/p5f/fc-report.md index ed0ca2a9..54bed508 100644 --- a/docs/Disaggregated/notes/p5f/fc-report.md +++ b/docs/Disaggregated/notes/p5f/fc-report.md @@ -12,7 +12,7 @@ |---|---| | `MobileGL/MG_Remote/Protocol/protocol.fbs` | **append-only**：`SurfaceOpKind` 补 `SetSwapInterval=8` / `ReleaseResources=9` / `SetWindowHandle=10`（与 P6-SPAWN-PLAN 的预测逐字一致）；`WindowKind` 补 `MetalLayer=6`（f0-egl schema 缺口 3：两枚举不互洽的另一半）；`SurfaceOp` 补 `readSurface`、`context` 两字段（MakeCurrent 的语义单元是四元组） | | `.../Protocol/generated/protocol_generated.h` | 用 pinned flatc（`~/w7/p5-s1-flatc-build/flatc`，25.12.19）重新生成；diff 仅 +58/−6 行，全部落在追加的枚举值与字段上 | -| `.../Server/SurfaceControlFrame.{h,cpp}` | 纯值帧：`SurfaceControlOp`（10 个 wire op + 4 个 inproc-only kind）+ `SurfaceControlFrame`（`static_assert` 钉死 trivially-copyable / standard-layout——帧里**不可能**有指针） | +| `.../Server/SurfaceControlFrame.{h,cpp}` | 纯值帧：`SurfaceControlOp`（10 个 wire op + 4 个 inproc-only kind）+ `SurfaceControlFrame`（constexpr 穷举结构化绑定 + 每成员 integer/enum 断言禁止指针；trivially-copyable / standard-layout 单独约束复制形态（§8 修正初版断言）） | | `.../Protocol/SurfaceOpCodec.{h,cpp}` | 帧 ↔ `Wire::SurfaceOp`/`SurfaceReply` 编解码；`WindowBackend` ↔ `WindowKind` 显式映射表（Surfaceless/Pbuffer 具名无后端）；wire 入口 `ServerApplyWireSurfaceOp` 的两个具名 Fatal（见 §3）；枚举对齐用 static_assert 钉死而非强转 | | `.../Server/ServerLoop.{h,cpp}` | **单槽邮箱的载荷从"函数指针 + 栈上 void\*"换成按值携带的一个帧**。`RunOnApplyThread(ControlWork, void*)` → `RunSurfaceControlFrame(SurfaceControlFrame&)`；十二个 forwarder 的 Args::Run 本体逐字搬进成员分发 `ApplySurfaceControlFrame`（C7/ID-54 分类、N-3 遗忘、R-12 重发布全部原位保留）；阻塞握手、影子位、publish-then-ring、C2 出口块全部不动。测试缝：`RunProbeOnApplyThreadForTesting`（hook 是成员变量，**不进槽位**） | | `MG_Test/Wire/SurfaceControlFrameTest.cpp` | 新套件 9 用例（见 §3） | @@ -23,8 +23,8 @@ **grep 证明**（验收项）：`grep -rn "ControlWork\|m_controlWork\|m_controlUser\|RunOnApplyThread" MobileGL/ --include="*.h" --include="*.cpp" --include="*.inc"` → **0 命中**；邮箱槽位类型是 -`SurfaceControlFrame m_controlFrame;`（按值，`ServerLoop.h:412`），其 trivially-copyable 由 -`SurfaceControlFrame.h` 的 static_assert 钉死。披露：测试缝的 `m_controlProbeHook` / +`SurfaceControlFrame m_controlFrame;`（按值，`ServerLoop.h:412`），其无指针约束由 +`SurfaceControlFrame.h` 的穷举成员检查钉死；仅 trivially-copyable 并不能拒绝指针（§8）。披露：测试缝的 `m_controlProbeHook` / `m_controlProbeUser` 是 ServerLoop 的**成员**而非槽位内容（同 `m_beforeRetireHook` 的既有先例）， 只在 `ProbeForTesting` kind 的分发臂被读。 @@ -115,3 +115,47 @@ ID-67 的重发布计数、M-7/C2 的两个 !m_running 臂、影子位双清用 append-only 后重生成。 - `docs/Disaggregated/ROADMAP.md` G4 行、`P5F-WIRE-COMPLETENESS.md` 进度行、`P6-CONTRACT-DRAFT.md` §4：文档同点编辑。 + + +## 8 集成前局部复审修复（2026-09-19） + +本节追加独立修复，§5 的全门数字保留为初版历史；本次只重跑受影响测试，集成全门由集成者负责。 +这次局部复审不代替 P5f 不同模型族终审。 + +| 缺陷 | 修复与回归用例 | +|---|---| +| wire `seq` 在真实 dispatch 入口被重新 mint，codec-only 往返漏检 | 仅 `seq==0` 的本地帧 mint；wire 序号保留。`WireSequenceSurvivesRealDispatchAndFailureReply` 驱动 wire → apply → reply 编码，要求无 backend 的失败回复仍回声 4242；成功臂亦断言原序号。 | +| 4 个 void 成功臂未填 `ok`，编码成 `false` | `SetSwapInterval` / `ReleaseSurface` / `ReleaseResources` / `SetWindowHandle` 完成后写 `ok=true`。`SuccessfulVoidWireOperationsReplyWithSuccessAndOriginalSequence` 用真实 EGL backend 驱动四种 op 并检查实际编码回复。 | +| MetalLayer 与 Android 一样携带客户端对象地址，却未拒绝 | `DecodeWireSurfaceOp` 对两种 window op 都返回 `MetalLayerArrived`，入口具名 Fatal `MetalLayer@P12`；新增纯解码检查与死亡用例。 | +| trivially-copyable + standard-layout 不能排除指针；初版的机械证明错误 | constexpr 结构化绑定穷举 14 个字段，每项只准 integral/enum。添加任何成员而不更新绑定会编译失败；将既有字段改为指针会触发 static_assert。保留旧两个 trait，并加入 object/function pointer 的 constexpr 反例和新用例。 | +| probe hook/user 在 `m_callerMutex` 外写，可被并发或重入 probe 覆盖 | 发布移入 caller 临界区，锁覆盖整个握手；apply 线程重入用调用栈上的 hook/user 参数，不写共享 pair。8 个并发投递者各 128 次，每次再嵌套一个 probe，逐投递者计数并校验总分发数。 | + +真实 dispatch 测试还暴露 `SurfaceOpCodec.h` 的 include 顺序问题：`Wire::` 会被既有 +`MG_Remote::Wire` 命名空间遮蔽；已将公开声明限定为 `::MobileGL::Wire::`。 + +**G14**：原测试全部保留，新增 5 条（SurfaceControlFrameTest +2、ServerLoopTest +2、 +ServerLoopEglTest +1）。两目标 `cmake --build build-split --target SurfaceControlFrameTest ServerLoopTest -j8` +通过；`MOBILEGL_ITEST_REQUIRE_GPU=1 ctest --test-dir build-split -R +'^(SurfaceControlFrameTest|ServerLoopTest|ServerLoopEglTest)[.]' --output-on-failure -j8` +**42/42 通过，零 skip**。probe 的另外两组调用者也已覆盖：重建 `RemoteClientTest`， +`RemoteClientControls` **26/26** 与该可执行文件的其余 8 套件 **116/116** 全绿， +合计本次相关 unit **184/184**。 + +**真实 red-once，全部在 WSL 验证副本修改并还原，没有提交故障代码：** + +| 单独回退 | 红证据 | +|---|---| +| seq 恢复无条件 mint | `WireSequenceSurvivesRealDispatchAndFailureReply` 失败：回复 1，预期 4242；ctest rc=8。 | +| 删除四处 `frame.ok=true` | 四种操作均报 `successful void operation encoded failure`；ctest rc=8。 | +| 删除 MetalLayer 解码拒绝 | 解码错误码不符，死亡测试 `failed to die`；ctest rc=8。 | +| probe 恢复锁外共享 pair 发布，并让重入读共享 pair | 并发+nested 用例逐投递者计数不符，报 `another poster or nested probe replaced this hook/user pair`；ctest rc=8。 | +| 额外新增第 15 个指针成员但不更新检查 | 用真实 SurfaceControlFrame.cpp TU 的 compile_commands 编译参数执行 `-fsyntax-only`，编译失败：`binds to 15 elements, but only 14 names were provided`，rc=1。 | +| 将既有 nativeToken 改为 void* | 同样的真实 TU 编译失败，正是 `every surface control frame member must be an integer or enum` 断言，rc=1。 | + +还原后两目标重建通过，相关测试再次 **42/42 绿**。日志在 WSL +`/tmp/p5f-fc-fix-{red-seq,red-ok,red-metal,red-probe,red-added-member,red-pointer-type,restored-green}.log`。 +第一次编译负例的检查脚本预期了另一种编译器措辞，实际已按正确原因编译失败；修正日志匹配词后 +单独重跑两个编译负例并恢复验证，没有重跑无关门。 + +WSL 旧门树 HEAD `7257a372` 与 Windows `d5ed139c` 为兄弟提交，仅报告不同，不能 ff-only。 +本次仅覆盖已确认无本地修改的 7 个相关源/测试文件验证，未 reset WSL 树，未改动其既有子模块状态。