fix(ios): answer the lease a hop was queued for, not whichever call holds the slot

packages/zig/src/ios_async.zigmodified+163-22

Changes to packages/zig/src/ios_async.zig

		⋯37 linesShow the 37 lines above this hunk@@ -38,9 +38,13 @@ const compat_mutex = ;
38	38	const objc = objc_runtime.objc;
39	39	const is_darwin = builtin.target.os.tag.isDarwin();
40	40
41		-/// How many async calls may be in flight at once. A page that exceeds this
42		-/// gets an explicit Busy error, which is an answer — the alternative was a
43		-/// silently dropped reply.
	41	+/// How many async calls may be in flight at once.
	42	+///
	43	+/// A page that exceeds it gets an answer rather than silence, which was the
	44	+/// point: `acquire` returns null and every caller reports `NativeCallFailed`.
	45	+/// Not the `Busy` this comment used to promise — no such code exists, and
	46	+/// adding one touches every caller's `poolFull`, so it is filed rather than
	47	+/// smuggled in here (#225).
44	48	pub const max_in_flight = 16;
45	49
46	50	const Slot = struct {
		⋯25 linesShow the 25 lines above this hunk@@ -72,8 +76,30 @@ const Slot = struct {
72	76	/// `"denied"` for a caller expecting an array. That is a wrong answer
73	77	/// reported as success, and the page's catch never runs.
74	78	failed: bool = false,
	79	+ /// Whether this lease has handed a pool block to a framework and is still
	80	+ /// waiting for it. Cleared by the first fire, so a framework that calls
	81	+ /// its completion twice queues one hop rather than two.
	82	+ completion_armed: bool = false,
75	83	};
76	84
	85	+/// Clear everything a lease may have written, so the next one starts clean.
	86	+///
	87	+/// Both release paths call it — the hop that delivered, and `abandon` — so a
	88	+/// slot is never handed out carrying the last lease's answer. `abandon` did
	89	+/// not free a stashed payload before, which leaked it whenever a module
	90	+/// abandoned a ticket it had already filled (#225).
	91	+///
	92	+/// Call with the mutex held, and move `json` out first if you are delivering
	93	+/// it: this frees whatever is still there.
	94	+fn resetReply(slot: *Slot) void {
	95	+ if (slot.json) |owned| std.heap.c_allocator.free(owned);
	96	+ slot.json = null;
	97	+ slot.failed = false;
	98	+ slot.fail_code = bridge_error.BridgeError.NativeCallFailed;
	99	+ slot.granted = false;
	100	+ slot.completion_armed = false;
	101	+}
	102	+
77	103	var slots: [max_in_flight]Slot = @splat(.{});
78	104	var slots_mutex: compat_mutex.Mutex = .{};
79	105
		⋯34 linesShow the 34 lines above this hunk@@ -114,6 +140,7 @@ pub fn abandon(ticket: Ticket) void {
114	140	defer slots_mutex.unlock();
115	141	const slot = &slots[ticket.index];
116	142	if (slot.generation != ticket.generation or !slot.in_use) return;
	143	+ resetReply(slot);
117	144	slot.in_use = false;
118	145	slot.generation +%= 1;
119	146	}
		⋯68 linesShow the 68 lines above this hunk@@ -188,14 +215,30 @@ fn makeBlocks(comptime maker: fn (comptime u5) *const anyopaque) [max_in_flight]
188	215	var bool_blocks: [max_in_flight]BoolBlock = if (is_darwin) makeBlocks(makeBoolInvoke) else undefined;
189	216	var bool_error_blocks: [max_in_flight]BoolBlock = if (is_darwin) makeBlocks(makeBoolErrorInvoke) else undefined;
190	217
	218	+/// Mark this lease as waiting for its pool block to fire.
	219	+///
	220	+/// There is one block per *slot*, not per lease, so the block itself cannot
	221	+/// say which call it belongs to. This is what lets the first fire be told
	222	+/// from the second: the fire consumes the arming, and a framework that calls
	223	+/// its completion twice gets one hop.
	224	+fn armCompletion(ticket: Ticket) void {
	225	+ slots_mutex.lock();
	226	+ defer slots_mutex.unlock();
	227	+ const slot = &slots[ticket.index];
	228	+ if (!slot.in_use or slot.generation != ticket.generation) return;
	229	+ slot.completion_armed = true;
	230	+}
	231	+
191	232	/// The block to pass as a `void (^)(BOOL)` completion handler for the call
192	233	/// this ticket was acquired for.
193	234	pub fn boolBlock(ticket: Ticket) *anyopaque {
	235	+ armCompletion(ticket);
194	236	return @ptrCast(&bool_blocks[ticket.index]);
195	237	}
196	238
197	239	/// The block to pass as a `void (^)(BOOL, NSError *)` completion handler.
198	240	pub fn boolErrorBlock(ticket: Ticket) *anyopaque {
	241	+ armCompletion(ticket);
199	242	return @ptrCast(&bool_error_blocks[ticket.index]);
200	243	}
201	244
		⋯10 linesShow the 10 lines above this hunk@@ -212,21 +255,27 @@ fn completionFired(index: u5, granted: bool) void {
212	255	// Record the outcome under the lock, but deliver from the main queue:
213	256	// the reply ends in `evaluateJavaScript`, which is main-thread-only, and
214	257	// this may be running on whatever queue the framework chose.
	258	+ var ticket: Ticket = undefined;
215	259	{
216	260	slots_mutex.lock();
217	261	defer slots_mutex.unlock();
218	262	const slot = &slots[index];
219	263	if (!slot.in_use) return; // stale fire for an abandoned slot
	264	+ // A second fire for the same lease, or a fire for a lease that never
	265	+ // armed a pool block. Either way the answer is not this one's to give.
	266	+ if (!slot.completion_armed) return;
	267	+ slot.completion_armed = false;
220	268	slot.granted = granted;
	269	+ ticket = .{ .index = index, .generation = slot.generation };
221	270	}
222	271
223		- // The context pointer carries only the slot index; everything else stays
224		- // in the slot, guarded by its generation.
225		- dispatch_async_f(&_dispatch_main_q, @ptrFromInt(@as(usize, index) + 1), deliverOnMain);
	272	+ // The lease the outcome belongs to, not just the slot: by the time this
	273	+ // hop runs, the slot may have been answered and re-leased (#225).
	274	+ dispatch_async_f(&_dispatch_main_q, ticketContext(ticket), deliverOnMain);
226	275	}
227	276
228	277	fn deliverOnMain(context: ?*anyopaque) callconv(.c) void {
229		- const index: usize = @intFromPtr(context orelse return) - 1;
	278	+ const ticket = ticketFromContext(context) orelse return;
230	279
231	280	var action_buf: [64]u8 = undefined;
232	281	var action_len: usize = 0;
		⋯5 linesShow the 5 lines above this hunk@@ -238,8 +287,12 @@ fn deliverOnMain(context: ?*anyopaque) callconv(.c) void {
238	287	{
239	288	slots_mutex.lock();
240	289	defer slots_mutex.unlock();
241		- const slot = &slots[index];
242		- if (!slot.in_use) return;
	290	+ const slot = &slots[ticket.index];
	291	+ // The slot being in use is not enough: it may be in use by a *later*
	292	+ // call. Two deliver* calls for one ticket could both pass their own
	293	+ // check before the first hop ran, and the second would then answer
	294	+ // whoever leased the slot next, with the first one's outcome (#225).
	295	+ if (!slot.in_use or slot.generation != ticket.generation) return;
243	296	action_len = slot.action_len;
244	297	@memcpy(action_buf[0..action_len], slot.action_buf[0..action_len]);
245	298	request_id = slot.request_id;
		⋯1 lineShow the 1 line above this hunk@@ -247,9 +300,9 @@ fn deliverOnMain(context: ?*anyopaque) callconv(.c) void {
247	300	json = slot.json;
248	301	failed = slot.failed;
249	302	fail_code = slot.fail_code;
	303	+ // Moved out, so the reset below has nothing left to free.
250	304	slot.json = null;
251		- slot.failed = false;
252		- slot.fail_code = bridge_error.BridgeError.NativeCallFailed;
	305	+ resetReply(slot);
253	306	slot.in_use = false;
254	307	slot.generation +%= 1;
255	308	}
		⋯49 linesShow the 49 lines above this hunk@@ -305,7 +358,7 @@ pub fn deliverJson(ticket: Ticket, json: []const u8) void {
305	358	slots_mutex.unlock();
306	359
307	360	if (!valid) return;
308		- dispatch_async_f(&_dispatch_main_q, @ptrFromInt(@as(usize, ticket.index) + 1), deliverOnMain);
	361	+ dispatch_async_f(&_dispatch_main_q, ticketContext(ticket), deliverOnMain);
309	362	}
310	363
311	364	/// Reply to a captured call with a *specific* error.
		⋯13 linesShow the 13 lines above this hunk@@ -325,7 +378,7 @@ pub fn deliverErrorCode(ticket: Ticket, err: bridge_error.BridgeError) void {
325	378	slots_mutex.unlock();
326	379
327	380	if (!valid) return;
328		- dispatch_async_f(&_dispatch_main_q, @ptrFromInt(@as(usize, ticket.index) + 1), deliverOnMain);
	381	+ dispatch_async_f(&_dispatch_main_q, ticketContext(ticket), deliverOnMain);
329	382	}
330	383
331	384	// ---------------------------------------------------------------------------
		⋯18 linesShow the 18 lines above this hunk@@ -350,25 +403,33 @@ extern "c" fn dispatch_after_f(when: u64, queue: *anyopaque, context: ?*anyopaqu
350	403	pub fn scheduleDeadline(ticket: Ticket, timeout_ms: u32, work: dispatch_function_t) void {
351	404	if (!is_darwin) return;
352	405	const delay_ns: i64 = @as(i64, timeout_ms) * 1_000_000;
353		- dispatch_after_f(dispatch_time(0, delay_ns), &_dispatch_main_q, deadlineContext(ticket), work);
	406	+ dispatch_after_f(dispatch_time(0, delay_ns), &_dispatch_main_q, ticketContext(ticket), work);
354	407	}
355	408
356	409	/// The ticket a deadline's `work` was scheduled for.
357	410	pub fn ticketFromDeadline(context: ?*anyopaque) Ticket {
358		- const encoded = @intFromPtr(context orelse unreachable) - 1;
359		- return .{
360		- .index = @intCast(encoded & 31),
361		- .generation = @intCast(encoded >> 5),
362		- };
	411	+ return ticketFromContext(context) orelse unreachable;
363	412	}
364	413
365	414	/// The whole ticket in the context pointer, plus one so it is never null.
366	415	/// Relies on 64-bit pointers: a `u32` generation shifted past the index bits.
367		-fn deadlineContext(ticket: Ticket) ?*anyopaque {
	416	+///
	417	+/// Used by the main-queue hop as well as the deadline. Both need to say which
	418	+/// *lease* they are for, and a pointer-sized context is the only thing either
	419	+/// `dispatch_async_f` or `dispatch_after_f` will carry.
	420	+fn ticketContext(ticket: Ticket) ?*anyopaque {
368	421	const encoded = (@as(usize, ticket.generation) << 5) | @as(usize, ticket.index);
369	422	return @ptrFromInt(encoded + 1);
370	423	}
371	424
	425	+fn ticketFromContext(context: ?*anyopaque) ?Ticket {
	426	+ const encoded = @intFromPtr(context orelse return null) - 1;
	427	+ return .{
	428	+ .index = @intCast(encoded & 31),
	429	+ .generation = @intCast(encoded >> 5),
	430	+ };
	431	+}
	432	+
372	433	/// Reply to a captured call with an error rather than a result.
373	434	///
374	435	/// A module's own completion can fail to shape its answer — a nil object where
		⋯7 linesShow the 7 lines above this hunk@@ -382,7 +443,7 @@ pub fn deliverError(ticket: Ticket) void {
382	443	slots_mutex.unlock();
383	444
384	445	if (!valid) return;
385		- dispatch_async_f(&_dispatch_main_q, @ptrFromInt(@as(usize, ticket.index) + 1), deliverOnMain);
	446	+ dispatch_async_f(&_dispatch_main_q, ticketContext(ticket), deliverOnMain);
386	447	}
387	448
388	449	const testing = std.testing;
		⋯139 linesShow the 139 lines above this hunk@@ -528,9 +589,89 @@ test "the generic deliverError still means a native failure" {
528	589	slots[ticket.index].generation +%= 1;
529	590	}
530	591
	592	+test "a hop for a lease that has ended cannot answer the call that replaced it" {
	593	+ // The race #225 is about. Two deliver* calls for one ticket can both pass
	594	+ // their own generation check before the first hop runs: the first hop
	595	+ // answers and releases the slot, a new call leases it, and the second hop
	596	+ // then hands that call the first one's outcome.
	597	+ request_context.push(7);
	598	+ const first = acquire("pickImage") orelse return error.PoolUnexpectedlyFull;
	599	+ request_context.pop();
	600	+
	601	+ deliverJson(first, "\"first\"");
	602	+ deliverOnMain(ticketContext(first));
	603	+ try testing.expect(!slots[first.index].in_use);
	604	+
	605	+ const second = acquire("getContacts") orelse return error.PoolUnexpectedlyFull;
	606	+ defer abandon(second);
	607	+ // Not incidental: `acquire` hands out the lowest free slot, so the call
	608	+ // that follows a released one takes its place. That is the whole setup.
	609	+ try testing.expectEqual(first.index, second.index);
	610	+
	611	+ // The stale hop arrives. It must find the lease gone and do nothing.
	612	+ deliverOnMain(ticketContext(first));
	613	+ try testing.expect(slots[second.index].in_use);
	614	+ try testing.expectEqualStrings(
	615	+ "getContacts",
	616	+ slots[second.index].action_buf[0..slots[second.index].action_len],
	617	+ );
	618	+}
	619	+
	620	+test "abandoning a ticket that already has an answer takes the answer with it" {
	621	+ // A module can fill a ticket and then abandon it — a completion that
	622	+ // shaped its reply and then found the call already settled. `abandon`
	623	+ // only flipped the flags, so the payload leaked and the flags stayed for
	624	+ // whoever leased the slot next.
	625	+ const first = acquire("getPendingNotifications") orelse return error.PoolUnexpectedlyFull;
	626	+ deliverJson(first, "[{\"id\":\"orphan\"}]");
	627	+ deliverErrorCode(first, bridge_error.BridgeError.Cancelled);
	628	+ try testing.expect(slots[first.index].json != null);
	629	+
	630	+ abandon(first);
	631	+ try testing.expect(slots[first.index].json == null);
	632	+ try testing.expect(!slots[first.index].failed);
	633	+ try testing.expectEqual(bridge_error.BridgeError.NativeCallFailed, slots[first.index].fail_code);
	634	+
	635	+ // And the next lease of that slot sees none of it.
	636	+ const second = acquire("getContacts") orelse return error.PoolUnexpectedlyFull;
	637	+ defer abandon(second);
	638	+ try testing.expectEqual(first.index, second.index);
	639	+ try testing.expect(slots[second.index].json == null);
	640	+ try testing.expect(!slots[second.index].granted);
	641	+}
	642	+
	643	+test "a completion that fires twice answers with the first outcome, once" {
	644	+ // One block per slot, not per lease, so the block cannot tell its own
	645	+ // second fire from its first. The arming can: it is consumed by the fire.
	646	+ const ticket = acquire("requestPermission") orelse return error.PoolUnexpectedlyFull;
	647	+ defer abandon(ticket);
	648	+
	649	+ _ = boolBlock(ticket);
	650	+ try testing.expect(slots[ticket.index].completion_armed);
	651	+
	652	+ completionFired(ticket.index, true);
	653	+ try testing.expect(!slots[ticket.index].completion_armed);
	654	+ try testing.expect(slots[ticket.index].granted);
	655	+
	656	+ // The framework calls back again, with the opposite answer.
	657	+ completionFired(ticket.index, false);
	658	+ try testing.expect(slots[ticket.index].granted);
	659	+}
	660	+
	661	+test "a fire for a lease that armed no block is not its answer to give" {
	662	+ // A module that shapes its own reply through deliverJson never arms a
	663	+ // pool block. A stray fire on its slot must not resolve it.
	664	+ const ticket = acquire("getContacts") orelse return error.PoolUnexpectedlyFull;
	665	+ defer abandon(ticket);
	666	+
	667	+ try testing.expect(!slots[ticket.index].completion_armed);
	668	+ completionFired(ticket.index, true);
	669	+ try testing.expect(!slots[ticket.index].granted);
	670	+}
	671	+
531	672	test "a deadline's context carries the complete ticket" {
532	673	const original: Ticket = .{ .index = 31, .generation = 0xfedcba98 };
533		- const decoded = ticketFromDeadline(deadlineContext(original));
	674	+ const decoded = ticketFromDeadline(ticketContext(original));
534	675	try testing.expectEqual(original.index, decoded.index);
535	676	try testing.expectEqual(original.generation, decoded.generation);
536	677	}