fix: fixes for sandbox communication (shared origins and permissions)#36
fix: fixes for sandbox communication (shared origins and permissions)#36ryan-karn wants to merge 5 commits into
Conversation
5eb1133 to
13a98dd
Compare
|
Detailing some the of the behaviour seen prior to this fix; (1) Sandbox → Host messagingiss29_bug1.mp4
(2) Sandbox→Sandbox messagingiss29_bug3.mp4
(3) Error Callbacksiss29_bug5.mp4
|
|
Demonstration of post-fix behaviour: (1) Sandbox → Host messagingiss29_fix1.mp4
(2) Sandbox→Sandbox messagingiss29_fix2.mp4
(3) Error Callbacksiss29_fix3.mp4
(4) Origin permission checksiss29_fix4.mp4
|
|
Also, demonstration of the p2p counter demo post this change - Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-05-14.at.14.53.12.mov |
CAMOBAP
left a comment
There was a problem hiding this comment.
Overall the approach is solid — per-surface routing, error broadcasting, and the access control semantics flip (sender → receiver side) are all the right design choices. A few things before merge.
|
|
||
| val surface = host.createSurface(sandboxContext, componentName, initialProperties) | ||
| val surface = host.createSurface(sandboxContext, componentName, initialProperties.withDelegateId()) | ||
| reactSurface = surface |
There was a problem hiding this comment.
Should we add the same for iOS too?
There was a problem hiding this comment.
Good catch, yeah looks like a miss on the iOS side. I'll adjust and ensure iOS demo works as expected in all scenarios
| * side into initialProperties. If undefined, falls back to broadcast/shared. | ||
| */ | ||
| export function useSurfaceMessaging(delegateId?: string) { | ||
| const postMessage = useCallback( |
There was a problem hiding this comment.
Nit: delegateId / __sandboxDelegateId leaks a native concept/naming into JS. surface fits better — it's already used in useSurfaceMessaging, surfaceMessageCallbacks, and RN's own architecture. What about __sandboxSurfaceId / surfaceId ?
There was a problem hiding this comment.
Yeah, makes sense to me, I'll adjust this
| /> | ||
| ``` | ||
|
|
||
| ### Direct communication Between Sandboxes |
There was a problem hiding this comment.
useSurfaceMessaging and the surface ID prop are not covered here. Bundle authors have no way to discover them without reading the demo source. Should be documented too.
There was a problem hiding this comment.
100% yes good catch - I could have sworn I had added something about that in either this readme or the demo one, but alas it does not seem to be there.
I'll had a distinct section here to cover the approach and nuances of sandbox communication if levergaing a shared or pooled origin
|
@ryan-karn outstanding work! Only minor comments found. Also, if you have a chance to check |
…allstackincubator#30, callstackincubator#28) - Add iOS support for shared-origin factory pooling (same-origin sandboxes reuse a single RCTReactNativeFactory) - Add idleTTL prop to defer ReactHost/factory cleanup after last surface unmounts, enabling warm starts for same-origin remounts - Add origin-pooling demo app demonstrating both features on iOS and Android Ref: callstackincubator#28 Ref: callstackincubator#30
…ittedFrom callstackincubator#35) Fixing issues for handling messages and errors when sandboxes use the same origin. Namely, this approach defines and passes an id that is passed to the sandbox, and provides a hook that allows sandboxes to use this for messaging. This is to distinguish views on the same origin that share the same runtime and globals. This also addresses an issue there allowOrigin checks where checking sender side allows rather than receiver side. ref: callstackincubator#29 ref: callstackincubator#35
|
re: p2p-chat I did quickly check it (I missed calling this out in the PR description), but at any rate I will do a full pass of that p2p chat and counter demo apps once I make some of the adjustments here EDIT: So did some more testing and the basic functionality of the p2p-chat works (no real change as was mostly bi-directional permissions on sandboxes) however, there was an IOS regression introduced that would crash the app when removing a sandbox chat instance. So I'll have that fix included as part of next commit to address the PR. Seems like the scenario was pertaining to clean up when no TTL is set. |
Adds __sandboxSurfaceId injection into iOS initialProperties for per-surface message routing parity with Android. Renames the concept from delegateId to surfaceId across both platforms and JS to align with existing RN surface terminology (PR callstackincubator#36 feedback).
…ry teardown The TTL=0 path in releaseSharedFactory removed the factory from the pool but left the stale delegate wrapper in the C++ registry. When another sandbox then tried to route a message to the removed origin, it found the dangling wrapper and crashed accessing a deallocated RCTInstance. Verified fix in the p2p-chat app. Prior to this, the app would crash on ios after removing a chat sandbox and trying to message it from another chat.
Documents the per-surface messaging API for bundle authors sharing an origin, with both hook and convention-based examples. Adds the origin-pooling demo to the examples list and reorders sections for better flow.
13a98dd to
f2cecdf
Compare
|
Additional commits added to PR in order to:
|
Note this PR is based on changes proposed in #34
Summary
This change fixes cross-origin messaging, error broadcasting, and access control for sandboxes sharing a Hermes VM via origin pooling.
#29
In order to achieve this behaviour, additional changes to the API/approach for sending and handling messages within a bundle were needed. (See details below).
These changes are:
useSurfaceMessaginghookWhy
When multiple sandboxes share an origin (and therefore a single Hermes VM), there is only one
globalThis.postMessageand oneglobalThis.setOnMessagefor the entire runtime. The native side can install these globals, but it cannot know which surface a JS call originates from without cooperation from the JS layer.The fundamental problem:
postMessage({type: 'hello'})arrives at the native host function, but the native side has no way to determine which of the N surfaces sharing this VM made the call. Similarly, when a message arrives from the host, the native side can invoke the JS callback, but if all surfaces registered via the samesetOnMessage, only the last one wins.The solution requires the JS side to include a routing hint (
__sandboxDelegateId) in outgoing messages and to register listeners keyed by delegate ID. This is inherently a JS-side concern because:initialProperty— it's only available to the JS component as a prop.postMessagehost function receives a genericjsi::Value— it can't inspect the JS call stack to determine which React component invoked it.setOnMessageso the native side can maintain a map of callbacks.The hook (or the equivalent convention-based pattern) bridges this gap by attaching the surface identity to messages at the JS layer, enabling the native layer to route correctly.
Break down of changes included
iOS Fixes (
SandboxReactNativeDelegate.mm)1.
setOnMessageargument countThe native
setOnMessageJSI function strictly required 1 argument, but the JSuseSurfaceMessaginghook passes an optionaldelegateIdas the second argument. Updated to accept 1 or 2 arguments.2. JSI global re-installation on warm start
When a same-origin sandbox warm-starts, it needs to swap
postMessage/setOnMessageto point to the new delegate. The original code useddefineReadOnlyGlobalwithconfigurable: false, making the properties permanently locked. Replaced withObject.definePropertyusingconfigurable: trueso warm-start re-installations succeed.3. Per-surface
setOnMessagedispatchPreviously,
_onMessageSandboxwas a single shared callback (last-writer-wins). Added_surfaceMessageCallbacksmap keyed by delegate ID so each surface gets its own listener, matching Android behavior.4. Cross-origin message routing
Changed
routeMessage:toSandbox:to usefindAll()(deliver to all delegates for the target origin) and reordered checks: existence first, then permissions. This ensuresSandboxRoutingErrorfires when the target doesn't exist, andAccessDeniedErroronly fires when the target exists but the sender lacks permission.5. Error broadcasting
Updated
setupErrorHandlerto broadcast errors to all delegates registered for the origin viaregistry.findAll(), matching Android. The error handler capturesoriginby value and uses a weak reference toselfto prevent crashes if the delegate is deallocated.6. Error handler idempotency
setupErrorHandleris now idempotent — it sets a flag (__sandboxErrorHandlerInstalled) onErrorUtilsand skips re-installation on warm start. The cold-start handler remains active and broadcasts via the registry.7. Self-targeting removal
Removed the guard that blocked a sandbox from sending messages to its own origin, matching Android behavior.
8. Registry unregistration fix
Changed
deallocandsetOriginto useunregisterDelegateinstead ofunregister, which was nuking all delegates for the entire origin when only one should be removed.Android Fixes (
SandboxJSIInstaller.cpp,SandboxReactNativeDelegate.kt,SandboxReactNativeViewManager.kt)1. Object mutation in postMessage
The
__sandboxDelegateIdproperty was stripped from the caller's message object without being restored. Now the property is restored after serialization to avoid surprising the caller.2. Thread-safe delegate map
delegateByIdchanged frommutableMapOf(non-thread-safe) toConcurrentHashMapsince it's accessed from both the JS and UI threads.3. Atomic delegate ID generation
nextDelegateIdchanged from a plainLongtoAtomicLongto prevent duplicate IDs under concurrent access.4.
allowedOriginsregistrationAndroid was always registering sandboxes with empty
allowedOriginsin the C++SandboxRegistry. Both registration paths (installSandboxJSIBindingsandnativeRegisterDelegate) now read theallowedOriginsfield from the Kotlin delegate via JNI at registration time. AddednativeUpdateAllowedOriginsJNI method and call it from the view manager when the prop changes.5. Cross-origin routing order
Aligned with iOS: check target existence first (
SandboxRoutingError), then permissions (AccessDeniedError). Previously Android checked permissions first, which produced misleading errors when the target didn't exist.Shared C++ (
SandboxRegistry.cpp,SandboxRegistry.h)#35
1.
registerSandboxalways updatesallowedOriginsThe duplicate-delegate check previously returned early without updating
allowedOrigins_[origin]. Now it skips adding the delegate but always updates the allowed origins map.2.
isPermittedFromsemanticsUpdated to check the target's
allowedOrigins(receiver-side access control): "does the target accept messages from this source?"3.
updateAllowedOriginsmethodNew method that updates allowed origins for an origin without requiring a delegate reference. Used by Android's JNI bridge.
Cleanup function
setOnMessagenow returns a cleanup function that replaces the callback with a no-op. Consumers should use it inuseEffectcleanup to prevent stale callbacks when a surface unmounts while the VM stays alive.Demo App (
apps/origin-pooling)Convention-based messaging (
SandboxAppConvention.tsx)New component demonstrating messaging without importing
@callstack/react-native-sandbox. UsesglobalThis.postMessage/setOnMessagedirectly with the__sandboxDelegateIdconvention.Dual-approach wiring
SandboxAppConvention(convention-based, no library import)SandboxApp(library-based,useSurfaceMessaging)Access control demo
isolated-1AccessDeniedErrorwhen trying to reach alpha/betaDemo App (
apps/p2p-counter)