ReviewOS
Reload

also looking at this

craft-native/craft

fix(ios): give getPendingNotifications a deadline, on the shared block table

#247
Merged glennmichael123 wants to merge fix/ios-notification-deadlines into main
2 files +164 -60
packages/zig/src/bridge_mobile_notifications.zigmodified+136-60
@@ -117,6 +117,8 @@ const bridge_error = ;
117117const objc_runtime = ;
118118const ios_async = ;
119119const compat_mutex = ;
120const ios_pending = ;
121const compat = ;
120122const memory = ;
121123
122124const objc = objc_runtime.objc;
@@ -220,19 +222,29 @@ pub const NotificationsBridge = struct {
220222 const sels = try Sels.resolve();
221223
222224 const ticket = ios_async.acquire(A.get_pending_notifications) orelse return poolFull();
225 errdefer ios_async.abandon(ticket);
223226
224 // Published before the framework call, never after: the completion runs
227 // Claimed before the framework call, never after: the completion runs
225228 // on a queue UserNotifications picks and can fire before `msgSend`
226 // returns. A block that arrived at an empty side-table slot would have
227 // no ticket to reply with.
228 publishPendingCall(ticket, sels);
229 // returns. A block that arrived at an empty table entry would have no
230 // ticket to reply with.
231 const block = calls.claim(ticket, sels) orelse {
232 std.log.warn(
233 "getPendingNotifications: all {d} completion blocks are still owed a completion " ++
234 "UserNotifications never delivered; refusing rather than reusing one",
235 .{ios_pending.block_count},
236 );
237 return poolFull();
238 };
239
240 ios_async.scheduleDeadline(ticket, pending_deadline_ms, pendingTimedOut);
229241
230242 // The completion is `void (^)(NSArray<UNNotificationRequest *> *)`,
231243 // which neither `boolBlock` nor `boolErrorBlock` fits — hence this
232 // module's own per-slot global block, feeding `ios_async.deliverJson`.
244 // module's own per-block global block, feeding `ios_async.deliverJson`.
233245 const Fn = *const fn (Id, Id, *anyopaque) callconv(.c) void;
234246 const func: Fn = @ptrCast(&objc.objc_msgSend);
235 func(center, sel_get, arrayBlock(ticket));
247 func(center, sel_get, arrayBlock(block));
236248 }
237249
238250 /// Schedule one local notification, and answer with its identifier.
@@ -499,26 +511,20 @@ const PendingCall = struct {
499511 sels: Sels,
500512};
501513
502var pending_calls: [ios_async.max_in_flight]?PendingCall = @splat(null);
503var pending_mutex: compat_mutex.Mutex = .{};
504
505/// Record the call a slot's block will answer. The slot is leased exclusively
506/// by this ticket, so the entry is ours to overwrite.
507fn publishPendingCall(ticket: ios_async.Ticket, sels: Sels) void {
508 pending_mutex.lock();
509 defer pending_mutex.unlock();
510 pending_calls[ticket.index] = .{ .ticket = ticket, .sels = sels };
511}
512
513/// Read and clear a slot's entry. Clearing is what makes a second fire of the
514/// same completion a no-op rather than a second reply.
515fn takePendingCall(index: u5) ?PendingCall {
516 pending_mutex.lock();
517 defer pending_mutex.unlock();
518 const call = pending_calls[index];
519 pending_calls[index] = null;
520 return call;
521}
514/// One entry per completion block, not per reply slot (#223).
515///
516/// A block knows only its own index. Keyed by reply slot, a deadline would
517/// free slot k, the next call would lease slot k and get the same block, and
518/// the first call's late completion would then answer the second with the
519/// first one's notifications. `ios_pending` keeps a block owed until the
520/// framework calls it; #211 found the same shape in a Siri deletion.
521var calls: ios_pending.Table(Sels) = .{};
522
523/// `getPendingNotificationRequestsWithCompletionHandler:` reads a local store
524/// and involves no person, so nothing justifies waiting on it for ever. Well
525/// under the 30s the page's own `_invoke` allows, so the caller hears this
526/// answer rather than its own timeout.
527const pending_deadline_ms: u32 = 10_000;
522528
523529const BlockDescriptor = extern struct {
524530 reserved: c_ulong = 0,
@@ -544,19 +550,20 @@ const array_block_descriptor = BlockDescriptor{ .size = @sizeOf(ArrayBlock) };
544550
545551extern var _NSConcreteGlobalBlock: anyopaque;
546552
547/// One invoke per slot, comptime-generated so each block knows which slot it is
548/// without capturing anything.
549fn makeArrayInvoke(comptime index: u5) *const anyopaque {
553/// One invoke per block, comptime-generated so each block knows which block it
554/// is without capturing anything. Per block rather than per reply slot: that
555/// separation is what makes a late completion harmless.
556fn makeArrayInvoke(comptime block: u5) *const anyopaque {
550557 const S = struct {
551558 fn invoke(_: *const ArrayBlock, requests: Id) callconv(.c) void {
552 pendingCompletionFired(index, requests);
559 pendingCompletionFired(block, requests);
553560 }
554561 };
555562 return @ptrCast(&S.invoke);
556563}
557564
558fn makeArrayBlocks() [ios_async.max_in_flight]ArrayBlock {
559 var out: [ios_async.max_in_flight]ArrayBlock = undefined;
565fn makeArrayBlocks() [ios_pending.block_count]ArrayBlock {
566 var out: [ios_pending.block_count]ArrayBlock = undefined;
560567 for (&out, 0..) |*b, i| {
561568 b.* = .{
562569 .isa = &_NSConcreteGlobalBlock,
@@ -568,11 +575,11 @@ fn makeArrayBlocks() [ios_async.max_in_flight]ArrayBlock {
568575 return out;
569576}
570577
571var array_blocks: [ios_async.max_in_flight]ArrayBlock =
578var array_blocks: [ios_pending.block_count]ArrayBlock =
572579 if (is_darwin) makeArrayBlocks() else undefined;
573580
574fn arrayBlock(ticket: ios_async.Ticket) *anyopaque {
575 return @ptrCast(&array_blocks[ticket.index]);
581fn arrayBlock(block: u5) *anyopaque {
582 return @ptrCast(&array_blocks[block]);
576583}
577584
578585/// Runs on whatever queue UserNotifications chose. It must not reply from here
@@ -585,19 +592,32 @@ fn arrayBlock(ticket: ios_async.Ticket) *anyopaque {
585592/// `ios_async.deliverError`, which reports NATIVE_CALL_FAILED through this same
586593/// slot and hop — so the caller learns the call failed instead of waiting out a
587594/// promise nothing will settle.
588fn pendingCompletionFired(index: u5, requests: Id) void {
595fn pendingCompletionFired(block: u5, requests: Id) void {
589596 if (!is_darwin) return;
590597
591 const call = takePendingCall(index) orelse {
592 std.log.warn(
593 "getPendingNotifications completion fired for slot {d} with no call recorded; ignored",
594 .{index},
595 );
596 return;
598 const entry = switch (calls.settle(block)) {
599 .reply => |parked| parked,
600 .late => |started_ms| {
601 std.log.warn(
602 "getPendingNotifications: a completion arrived {d} ms after the call, after its " ++
603 "deadline had answered; ignored",
604 .{compat.milliTimestamp() - started_ms},
605 );
606 return;
607 },
608 .stray => {
609 std.log.warn(
610 "getPendingNotifications completion fired for block {d} with no call recorded; ignored",
611 .{block},
612 );
613 return;
614 },
597615 };
616 const call = entry.call;
617 const ticket = entry.ticket;
598618
599619 const allocator = std.heap.c_allocator;
600 const json = shapePendingReply(allocator, requests, call.sels) catch |err| {
620 const json = shapePendingReply(allocator, requests, call) catch |err| {
601621 // Rejects rather than going silent. This abandoned the slot and left
602622 // the caller's untimed promise hanging, on the reasoning that
603623 // `ios_async` could resolve and nothing else, so the choice was
@@ -610,12 +630,27 @@ fn pendingCompletionFired(index: u5, requests: Id) void {
610630 "getPendingNotifications could not shape its reply ({}); rejecting",
611631 .{err},
612632 );
613 ios_async.deliverError(call.ticket);
633 ios_async.deliverError(ticket);
614634 return;
615635 };
616636 defer allocator.free(json);
617637
618 ios_async.deliverJson(call.ticket, json);
638 ios_async.deliverJson(ticket, json);
639}
640
641/// On the main queue, `pending_deadline_ms` after the call. Does nothing when
642/// the completion got there first, which is the usual case: the timer cannot
643/// be cancelled, so it always fires.
644fn pendingTimedOut(context: ?*anyopaque) callconv(.c) void {
645 if (!is_darwin) return;
646
647 const entry = calls.expireIfWaiting(ios_async.ticketFromDeadline(context)) orelse return;
648 std.log.warn(
649 "getPendingNotifications: getPendingNotificationRequestsWithCompletionHandler: did not " ++
650 "call its completion within {d} ms; answering TIMEOUT",
651 .{pending_deadline_ms},
652 );
653 ios_async.deliverErrorCode(entry.ticket, bridge_error.BridgeError.Timeout);
619654}
620655
621656// =============================================================================
@@ -813,6 +848,10 @@ const ScheduleCall = struct {
813848};
814849
815850var schedule_calls: [ios_async.max_in_flight]?ScheduleCall = @splat(null);
851/// Still the old shape: `scheduleNotification` is two stages, and its deadline
852/// has to start after the authorization prompt rather than at dispatch, which
853/// is its own change (#223).
854var pending_mutex: compat_mutex.Mutex = .{};
816855
817856fn publishScheduleCall(ticket: ios_async.Ticket, sels: ScheduleSels, schedule: Schedule) void {
818857 pending_mutex.lock();
@@ -1467,23 +1506,54 @@ test "the side table hands each slot's completion its own ticket, once" {
14671506 .utf8_length = null,
14681507 };
14691508
1470 publishPendingCall(ticket, sels);
1509 const block = calls.claim(ticket, sels) orelse return error.NoFreeBlock;
14711510
1472 const taken = takePendingCall(3) orelse return error.PublishedCallWentMissing;
1511 const taken = switch (calls.settle(block)) {
1512 .reply => |entry| entry,
1513 else => return error.PublishedCallWentMissing,
1514 };
14731515 try testing.expectEqual(@as(u5, 3), taken.ticket.index);
14741516 try testing.expectEqual(@as(u32, 77), taken.ticket.generation);
14751517
1476 try testing.expect(takePendingCall(3) == null);
1518 // And the block is free again, so the same fire cannot answer twice.
1519 try testing.expect(calls.settle(block) == .stray);
1520}
1521
1522test "a deadline answers once, and the completion it beat is ignored" {
1523 // #223: without the table a deadline would free the reply slot, the next
1524 // call would lease it and get the same block, and this late completion
1525 // would answer that call with these notifications.
1526 const ticket: ios_async.Ticket = .{ .index = 5, .generation = 12 };
1527 const sels: Sels = .{
1528 .count = undefined,
1529 .object_at = undefined,
1530 .content = undefined,
1531 .identifier = undefined,
1532 .title = null,
1533 .body = null,
1534 .subtitle = null,
1535 .utf8 = null,
1536 .utf8_length = null,
1537 };
1538 const block = calls.claim(ticket, sels) orelse return error.NoFreeBlock;
1539
1540 const expired = calls.expireIfWaiting(ticket) orelse return error.DeadlineFoundNothing;
1541 try testing.expectEqual(@as(u32, 12), expired.ticket.generation);
1542 // Owed, not free: another call must not be given this block.
1543 const other: ios_async.Ticket = .{ .index = 6, .generation = 1 };
1544 const elsewhere = calls.claim(other, sels) orelse return error.NoFreeBlock;
1545 try testing.expect(elsewhere != block);
1546 try testing.expect(calls.settle(block) == .late);
1547 _ = calls.settle(elsewhere);
14771548}
14781549
1479test "a completion for a slot with no recorded call does nothing" {
1480 // A late or duplicate fire must not reach `deliverJson` at all. With the
1481 // entry cleared there is no ticket to reply with, and the block returns
1482 // without touching the pool — asserted by the neighbouring slots staying
1483 // empty rather than by observing a reply that must not happen.
1484 try testing.expect(takePendingCall(9) == null);
1550test "a completion for a block with no recorded call does nothing" {
1551 // A late or duplicate fire must not reach `deliverJson` at all. With no
1552 // entry there is no ticket to reply with, and the block returns without
1553 // touching the pool.
1554 try testing.expect(calls.settle(9) == .stray);
14851555 pendingCompletionFired(9, null);
1486 try testing.expect(takePendingCall(9) == null);
1556 try testing.expect(calls.settle(9) == .stray);
14871557}
14881558
14891559test "off Darwin the handler refuses rather than fake an empty list" {
@@ -1519,6 +1589,10 @@ test "without a bundle identifier the guard fires before the center is touched"
15191589 var bridge = NotificationsBridge.init(testing.allocator);
15201590 defer bridge.deinit();
15211591
1592 // Drained first, so what follows reads what *this* call did
1593 // rather than whatever the rest of the file left behind.
1594 for (0..ios_pending.block_count) |i| _ = calls.settle(@intCast(i));
1595
15221596 try testing.expectError(
15231597 error.NoBundleIdentifier,
15241598 bridge.handleMessage(A.get_pending_notifications, "{}"),
@@ -1526,7 +1600,9 @@ test "without a bundle identifier the guard fires before the center is touched"
15261600
15271601 // And the refusal must happen before any slot is leased: a lease
15281602 // that is never released narrows the pool for every later call.
1529 for (pending_calls) |entry| try testing.expect(entry == null);
1603 for (0..ios_pending.block_count) |i| {
1604 try testing.expect(calls.settle(@intCast(i)) == .stray);
1605 }
15301606 },
15311607 else => return err,
15321608 }
@@ -1545,9 +1621,9 @@ test "the completion blocks are global, one per slot, and distinct" {
15451621 try testing.expectEqual(@as(c_ulong, @sizeOf(ArrayBlock)), b.descriptor.size);
15461622 }
15471623
1548 // Each slot needs its *own* invoke, or every completion would report the
1549 // same slot index and answer the wrong caller.
1624 // Each block needs its *own* invoke, or every completion would report the
1625 // same block and answer the wrong caller.
15501626 try testing.expect(array_blocks[0].invoke != array_blocks[1].invoke);
1551 try testing.expect(arrayBlock(.{ .index = 0, .generation = 1 }) !=
1552 arrayBlock(.{ .index = 1, .generation = 1 }));
1627 try testing.expect(array_blocks[0].invoke != array_blocks[ios_pending.block_count - 1].invoke);
1628 try testing.expect(arrayBlock(0) != arrayBlock(1));
15531629}
packages/zig/test/ios_conformance_test.zigmodified+28-0
@@ -2013,6 +2013,34 @@ test "no case the page waits on casts its argument without answering the other w
20132013 }
20142014}
20152015
2016test "a module that parks a call on a completion block also gives it a deadline" {
2017 // #223: a call parked on `ios_pending` is one whose only answer comes from
2018 // a framework completion. The table makes a late completion harmless; it
2019 // does not make one that never arrives answer at all. Both halves or
2020 // neither — parking without a deadline is a promise that can wait for
2021 // ever, which is what #211 found on CI twice.
2022 var parked: usize = 0;
2023 for (zig_sources) |source| {
2024 if (std.mem.indexOf(u8, source, "ios_pending.Table(") == null) continue;
2025 parked += 1;
2026 if (std.mem.indexOf(u8, source, "scheduleDeadline(") != null) continue;
2027
2028 // Named by the first action it declares, which is how these files are
2029 // recognisable without embedding their paths.
2030 const at = std.mem.indexOf(u8, source, "pub const") orelse 0;
2031 const line_end = std.mem.indexOfScalarPos(u8, source, at, '\n') orelse at;
2032 std.debug.print(
2033 "a module parks calls on ios_pending.Table and never calls scheduleDeadline.\n" ++
2034 " Near: {s}\n" ++
2035 " A parked call whose completion never comes settles nothing at all.\n",
2036 .{source[at..line_end]},
2037 );
2038 return error.ParkedCallHasNoDeadline;
2039 }
2040 // Non-vacuity: the scan has to be finding the modules that do park.
2041 try testing.expect(parked >= 2);
2042}
2043
20162044test "every call the page waits on has a case that can resolve it" {
20172045 const region = dispatcherRegion();
20182046 var awaited = try collectAwaitedActions(testing.allocator);