test(window): make "every window can be reopened" a property the build checks

packages/zig/build.zigmodified+30-0

Changes to packages/zig/build.zig

		⋯705 linesShow the 705 lines above this hunk@@ -706,6 +706,32 @@ pub fn build(b: *std.Build) void {
706	706	}),
707	707	});
708	708
	709	+ // Whether every window craft opens can be reopened. A source conformance
	710	+ // test: constructing a window needs a live NSApplication and a WebContent
	711	+ // process, but the two properties worth defending — every constructor
	712	+ // registers, and nothing infers craft-ness from a window again — are
	713	+ // properties of the source.
	714	+ const window_lifecycle_tests = b.addTest(.{
	715	+ .root_module = b.createModule(.{
	716	+ .root_source_file = b.path("test/window_lifecycle_test.zig"),
	717	+ .target = target,
	718	+ .optimize = optimize,
	719	+ }),
	720	+ });
	721	+ window_lifecycle_tests.root_module.addAnonymousImport("src/macos.zig", .{
	722	+ .root_source_file = b.path("src/macos.zig"),
	723	+ });
	724	+
	725	+ // The window registry itself, free of Objective-C so the whole lifecycle
	726	+ // is provable without AppKit.
	727	+ const window_registry_tests = b.addTest(.{
	728	+ .root_module = b.createModule(.{
	729	+ .root_source_file = b.path("src/window_registry.zig"),
	730	+ .target = target,
	731	+ .optimize = optimize,
	732	+ }),
	733	+ });
	734	+
709	735	// The conformance test: what craft declares, against what it dispatches.
710	736	const capability_conformance_tests = b.addTest(.{
711	737	.root_module = b.createModule(.{
		⋯365 linesShow the 365 lines above this hunk@@ -1077,6 +1103,8 @@ pub fn build(b: *std.Build) void {
1077	1103	const run_local_tls_tests = b.addRunArtifact(local_tls_tests);
1078	1104	const run_menu_roles_tests = b.addRunArtifact(menu_roles_tests);
1079	1105	const run_capabilities_tests = b.addRunArtifact(capabilities_tests);
	1106	+ const run_window_lifecycle_tests = b.addRunArtifact(window_lifecycle_tests);
	1107	+ const run_window_registry_tests = b.addRunArtifact(window_registry_tests);
1080	1108	const run_external_link_tests = b.addRunArtifact(external_link_tests);
1081	1109	const run_webview_recovery_tests = b.addRunArtifact(webview_recovery_tests);
1082	1110	const run_request_context_tests = b.addRunArtifact(request_context_tests);
		⋯113 linesShow the 113 lines above this hunk@@ -1196,6 +1224,8 @@ pub fn build(b: *std.Build) void {
1196	1224	test_step.dependOn(&run_local_tls_tests.step);
1197	1225	test_step.dependOn(&run_menu_roles_tests.step);
1198	1226	test_step.dependOn(&run_capabilities_tests.step);
	1227	+ test_step.dependOn(&run_window_lifecycle_tests.step);
	1228	+ test_step.dependOn(&run_window_registry_tests.step);
1199	1229	test_step.dependOn(&run_external_link_tests.step);
1200	1230	test_step.dependOn(&run_webview_recovery_tests.step);
1201	1231	test_step.dependOn(&run_request_context_tests.step);

packages/zig/src/macos.zigmodified+10-23

Changes to packages/zig/src/macos.zig

		⋯5 linesShow the 5 lines above this hunk@@ -6,6 +6,7 @@ const local_tls = ;
6	6	const menu_roles = ;
7	7	const external_link = ;
8	8	const webview_recovery = ;
	9	+const window_registry = ;
9	10	const request_context = ;
10	11
11	12	// Objective-C runtime types and functions (manual declarations to avoid @cImport issues)
		⋯6,403 linesShow the 6,403 lines above this hunk@@ -6415,25 +6416,15 @@ fn keepWindowAfterClose(window: objc.id) void {
6415	6416	rememberCraftWindow(window);
6416	6417	}
6417	6418
6418		-/// Every window craft opened, so reopen can tell them from AppKit's own.
6419		-///
6420		-/// Craft opens one today; #67 opens more. Sixteen is far past any real app and
6421		-/// bounded so this needs no allocator on a path that runs during window
6422		-/// creation.
6423		-var craft_windows: [16]objc.id = @splat(null);
6424		-
6425	6419	fn rememberCraftWindow(window: objc.id) void {
6426		- if (@intFromPtr(window) == 0) return;
6427		- for (&craft_windows) |*slot| {
6428		- if (slot.* == window) return;
6429		- if (@intFromPtr(slot.*) == 0) {
6430		- slot.* = window;
6431		- return;
6432		- }
6433		- }
6434		- // Full. Reopen will not restore this window, which is a worse outcome than
6435		- // the table being bigger — say so rather than failing silently.
6436		- std.log.warn("more than {d} windows; the newest will not be restored by a Dock click", .{craft_windows.len});
	6420	+ if (window_registry.remember(@intFromPtr(window))) return;
	6421	+ // Full. Reopen will not restore this window, which is worse than the table
	6422	+ // being bigger — say so rather than letting a window quietly stop being
	6423	+ // reopenable, which is the bug this registry exists to prevent.
	6424	+ std.log.warn(
	6425	+ "more than {d} windows open; the newest will not be restored by a Dock click",
	6426	+ .{window_registry.capacity},
	6427	+ );
6437	6428	}
6438	6429
6439	6430	/// Whether the app should quit when its last window closes.
		⋯71 linesShow the 71 lines above this hunk@@ -6511,11 +6502,7 @@ fn appShouldHandleReopen(_: objc.id, _: objc.SEL, app: objc.id, has_visible: boo
6511	6502	/// A test cannot see that. It needs a real reopen event against a sidebar
6512	6503	/// window, so the fix is to stop inferring the answer at all.
6513	6504	fn isCraftWindow(window: objc.id) bool {
6514		- if (@intFromPtr(window) == 0) return false;
6515		- for (craft_windows) |known| {
6516		- if (known == window) return true;
6517		- }
6518		- return false;
	6505	+ return window_registry.isKnown(@intFromPtr(window));
6519	6506	}
6520	6507
6521	6508	var app_delegate_installed = false;

packages/zig/src/window_registry.zigadded+160-0

Changes to packages/zig/src/window_registry.zig

		@@ -0,0 +1,160 @@
	1	+//! Which windows craft opened.
	2	+//!
	3	+//! The Dock's reopen event arrives with no argument: AppKit hands over the
	4	+//! application and craft has to work out, from `[NSApp windows]`, which of them
	5	+//! are its own. That list also holds the offscreen windows AppKit keeps for
	6	+//! menus, tooltips and its own bookkeeping, and ordering one of those front
	7	+//! puts an empty frame on screen.
	8	+//!
	9	+//! ## Why this is a list and not a test
	10	+//!
	11	+//! The first version asked the window: is your content view a `WKWebView`?
	12	+//! That is true of craft's plain window and false of three others —
	13	+//! `--web-sidebar-material` installs a backdrop container, and both sidebar
	14	+//! constructors install a container or a split view controller. All three kept
	15	+//! their closed window alive, so reopen found them, skipped them, and left the
	16	+//! app activated with nothing on screen: exactly the dead end the reopen
	17	+//! handler exists to remove.
	18	+//!
	19	+//! Nothing caught it. It compiles, every test passes, and it fails only against
	20	+//! a real reopen event on a window style no test constructs.
	21	+//!
	22	+//! Recording the answer instead of inferring it fixes that, and it does
	23	+//! something else worth more: it turns an untestable property into a
	24	+//! checkable one. "Does this window look like ours?" can only be answered by
	25	+//! having the window. "Did every constructor register?" is a property of the
	26	+//! source, and `test/window_lifecycle_test.zig` checks it.
	27	+
	28	+const std = ;
	29	+
	30	+/// An opaque window handle — `@intFromPtr(NSWindow)` at the call site. Kept
	31	+/// opaque so this module stays free of Objective-C and can be tested without
	32	+/// AppKit.
	33	+pub const Handle = usize;
	34	+
	35	+/// Craft opens one window today; #67 opens more. Far past any real app, and
	36	+/// fixed so registration needs no allocator on the window-creation path.
	37	+pub const capacity = 16;
	38	+
	39	+var windows: [capacity]Handle = @splat(0);
	40	+
	41	+/// Record a window craft created. Idempotent.
	42	+///
	43	+/// Returns false if the table was full and the window was not recorded — the
	44	+/// caller is expected to say so rather than let a window silently become
	45	+/// unreopenable.
	46	+pub fn remember(handle: Handle) bool {
	47	+ if (handle == 0) return false;
	48	+ for (&windows) |*slot| {
	49	+ if (slot.* == handle) return true;
	50	+ if (slot.* == 0) {
	51	+ slot.* = handle;
	52	+ return true;
	53	+ }
	54	+ }
	55	+ return false;
	56	+}
	57	+
	58	+/// Whether craft opened this window.
	59	+pub fn isKnown(handle: Handle) bool {
	60	+ if (handle == 0) return false;
	61	+ for (windows) |known| {
	62	+ if (known == handle) return true;
	63	+ }
	64	+ return false;
	65	+}
	66	+
	67	+/// Drop a window. For real teardown, whenever #67 introduces some.
	68	+pub fn forget(handle: Handle) void {
	69	+ for (&windows) |*slot| {
	70	+ if (slot.* == handle) slot.* = 0;
	71	+ }
	72	+}
	73	+
	74	+pub fn count() usize {
	75	+ var n: usize = 0;
	76	+ for (windows) |w| {
	77	+ if (w != 0) n += 1;
	78	+ }
	79	+ return n;
	80	+}
	81	+
	82	+pub fn resetForTesting() void {
	83	+ windows = @splat(0);
	84	+}
	85	+
	86	+const testing = std.testing;
	87	+
	88	+test "a window craft opened is recognised" {
	89	+ resetForTesting();
	90	+ try testing.expect(remember(0x1000));
	91	+ try testing.expect(isKnown(0x1000));
	92	+}
	93	+
	94	+test "a window craft did not open is not" {
	95	+ // The offscreen windows AppKit keeps for menus and tooltips land here.
	96	+ // Ordering one of those front puts an empty frame on screen.
	97	+ resetForTesting();
	98	+ _ = remember(0x1000);
	99	+ try testing.expect(!isKnown(0x2000));
	100	+}
	101	+
	102	+test "registering twice does not consume two slots" {
	103	+ resetForTesting();
	104	+ try testing.expect(remember(0x1000));
	105	+ try testing.expect(remember(0x1000));
	106	+ try testing.expectEqual(@as(usize, 1), count());
	107	+}
	108	+
	109	+test "a null window is neither recorded nor recognised" {
	110	+ // `contentView` and friends return nil often enough that zero has to mean
	111	+ // nothing rather than becoming a real entry that matches every other nil.
	112	+ resetForTesting();
	113	+ try testing.expect(!remember(0));
	114	+ try testing.expect(!isKnown(0));
	115	+ try testing.expectEqual(@as(usize, 0), count());
	116	+}
	117	+
	118	+test "every window up to capacity is recorded" {
	119	+ resetForTesting();
	120	+ var i: usize = 1;
	121	+ while (i <= capacity) : (i += 1) {
	122	+ try testing.expect(remember(i * 0x100));
	123	+ }
	124	+ i = 1;
	125	+ while (i <= capacity) : (i += 1) {
	126	+ try testing.expect(isKnown(i * 0x100));
	127	+ }
	128	+ try testing.expectEqual(capacity, count());
	129	+}
	130	+
	131	+test "past capacity the failure is reported, not swallowed" {
	132	+ // The caller logs this. A window that quietly stopped being reopenable is
	133	+ // the bug this whole module exists to prevent, so it must not be the
	134	+ // silent outcome of a full table.
	135	+ resetForTesting();
	136	+ var i: usize = 1;
	137	+ while (i <= capacity) : (i += 1) _ = remember(i * 0x100);
	138	+ try testing.expect(!remember(0xDEAD));
	139	+ try testing.expect(!isKnown(0xDEAD));
	140	+}
	141	+
	142	+test "forgetting frees the slot for the next window" {
	143	+ resetForTesting();
	144	+ var i: usize = 1;
	145	+ while (i <= capacity) : (i += 1) _ = remember(i * 0x100);
	146	+ try testing.expect(!remember(0xDEAD));
	147	+
	148	+ forget(0x100);
	149	+ try testing.expect(!isKnown(0x100));
	150	+ try testing.expect(remember(0xDEAD));
	151	+ try testing.expect(isKnown(0xDEAD));
	152	+}
	153	+
	154	+test "forgetting a window that was never known changes nothing" {
	155	+ resetForTesting();
	156	+ _ = remember(0x1000);
	157	+ forget(0x2000);
	158	+ try testing.expect(isKnown(0x1000));
	159	+ try testing.expectEqual(@as(usize, 1), count());
	160	+}

packages/zig/test/window_lifecycle_test.zigadded+232-0

Changes to packages/zig/test/window_lifecycle_test.zig

		@@ -0,0 +1,232 @@
	1	+//! Every window craft opens must be reopenable.
	2	+//!
	3	+//! This test exists because of a bug that nothing else could have caught.
	4	+//! `isCraftWindow` decided whether a window was craft's by asking whether its
	5	+//! content view was a `WKWebView`. That is true of the plain window and false
	6	+//! of the other three: `--web-sidebar-material` installs a backdrop container,
	7	+//! `createWindowWithSidebar` sets a split view controller, and
	8	+//! `createWindowWithSidebarURL` installs its own container. All three keep
	9	+//! their closed window alive, so the reopen handler found them, skipped them,
	10	+//! and left the app activated with nothing on screen — the exact dead end the
	11	+//! handler was added to remove.
	12	+//!
	13	+//! It compiled. Every test passed. It failed only against a real
	14	+//! `kAEReopenApplication` on a window style no test constructs, and **no test
	15	+//! in this repository constructs a window at all**.
	16	+//!
	17	+//! ## Why this is a source test
	18	+//!
	19	+//! Constructing a window means a live `NSApplication`, a `WKWebView` and the
	20	+//! WebContent process behind it — heavy, and flaky on a runner with no
	21	+//! display. The properties worth defending do not need any of that:
	22	+//!
	23	+//! 1. every window constructor registers its window, and
	24	+//! 2. nothing infers craft-ness from the window's appearance again.
	25	+//!
	26	+//! Both are properties of the source, and they are only checkable *because*
	27	+//! the fix replaced an inference with a registration. "Does this window look
	28	+//! like ours?" can only be answered by holding a window. "Did every
	29	+//! constructor register?" can be answered by reading. Making the property
	30	+//! checkable was worth as much as fixing the bug.
	31	+//!
	32	+//! Each check carries a floor, because a scan that finds nothing passes
	33	+//! vacuously, and a vacuous pass is how this class of bug returns.
	34	+
	35	+const std = ;
	36	+const testing = std.testing;
	37	+
	38	+const macos_source = @embedFile("src/macos.zig");
	39	+
	40	+/// The Objective-C initialiser every `NSWindow` in craft goes through.
	41	+const window_init = "initWithContentRect:styleMask:backing:defer:";
	42	+
	43	+/// What a constructor must call on the window it just made.
	44	+const register_call = "keepWindowAfterClose(";
	45	+
	46	+/// Whether `body` really calls `needle`, ignoring commented-out occurrences.
	47	+///
	48	+/// Substring matching alone cannot tell a call from a mention. Commenting a
	49	+/// registration out would leave the text in place and satisfy a naive scan —
	50	+/// which is exactly how this check was first found to be toothless, by
	51	+/// commenting one out and watching it pass.
	52	+fn callsFunction(body: []const u8, needle: []const u8) bool {
	53	+ var search: usize = 0;
	54	+ while (std.mem.indexOfPos(u8, body, search, needle)) |hit| {
	55	+ search = hit + needle.len;
	56	+ const start = lineStart(body, hit);
	57	+ const before = std.mem.trimStart(u8, body[start..hit], " \t");
	58	+ // `//` anywhere before it on the line makes it a comment, and `///`
	59	+ // makes it documentation.
	60	+ if (std.mem.startsWith(u8, before, "//")) continue;
	61	+ if (std.mem.indexOf(u8, body[start..hit], "//") != null) continue;
	62	+ return true;
	63	+ }
	64	+ return false;
	65	+}
	66	+
	67	+/// Start of the line containing `needle`.
	68	+fn lineStart(source: []const u8, at: usize) usize {
	69	+ var i = at;
	70	+ while (i > 0 and source[i - 1] != '\n') i -= 1;
	71	+ return i;
	72	+}
	73	+
	74	+/// The `fn` declaration enclosing byte `at`.
	75	+///
	76	+/// Zig indents declarations at column zero inside a file, so the nearest
	77	+/// preceding line beginning with `fn ` or `pub fn ` is the enclosing
	78	+/// top-level function. Container-level methods are indented and belong to a
	79	+/// struct whose own declaration is found the same way.
	80	+fn enclosingFn(source: []const u8, at: usize) ?[]const u8 {
	81	+ var i = lineStart(source, at);
	82	+ while (true) {
	83	+ const line_end = std.mem.indexOfScalarPos(u8, source, i, '\n') orelse source.len;
	84	+ const line = source[i..line_end];
	85	+ if (std.mem.startsWith(u8, line, "fn ") or std.mem.startsWith(u8, line, "pub fn ")) return line;
	86	+ if (i == 0) return null;
	87	+ i = lineStart(source, i - 1);
	88	+ }
	89	+}
	90	+
	91	+/// Byte range of the function body enclosing `at`, from its `fn` line to the
	92	+/// next top-level `fn` line (or end of file).
	93	+fn enclosingFnBody(source: []const u8, at: usize) []const u8 {
	94	+ var start = lineStart(source, at);
	95	+ while (start > 0) {
	96	+ const line_end = std.mem.indexOfScalarPos(u8, source, start, '\n') orelse source.len;
	97	+ const line = source[start..line_end];
	98	+ if (std.mem.startsWith(u8, line, "fn ") or std.mem.startsWith(u8, line, "pub fn ")) break;
	99	+ start = lineStart(source, start - 1);
	100	+ }
	101	+
	102	+ var end = at;
	103	+ while (end < source.len) {
	104	+ const line_end = std.mem.indexOfScalarPos(u8, source, end, '\n') orelse source.len;
	105	+ const line = source[end..line_end];
	106	+ if (end != start and (std.mem.startsWith(u8, line, "fn ") or std.mem.startsWith(u8, line, "pub fn "))) break;
	107	+ if (line_end >= source.len) {
	108	+ end = source.len;
	109	+ break;
	110	+ }
	111	+ end = line_end + 1;
	112	+ }
	113	+ return source[start..end];
	114	+}
	115	+
	116	+test "every window craft constructs is registered as one of its own" {
	117	+ // The check that would have caught the bug this file exists for — had the
	118	+ // answer been recorded rather than inferred. It is recorded now, so this
	119	+ // is the property that keeps it recorded.
	120	+ var constructors: usize = 0;
	121	+ var unregistered: usize = 0;
	122	+
	123	+ var search: usize = 0;
	124	+ while (std.mem.indexOfPos(u8, macos_source, search, window_init)) |hit| {
	125	+ search = hit + window_init.len;
	126	+
	127	+ // The name is mentioned in prose as well as called. Only a real
	128	+ // `msgSend4(...)` call constructs a window.
	129	+ const line = macos_source[lineStart(macos_source, hit)..hit];
	130	+ if (std.mem.indexOf(u8, line, "msgSend4") == null) continue;
	131	+
	132	+ constructors += 1;
	133	+ const body = enclosingFnBody(macos_source, hit);
	134	+ if (!callsFunction(body, register_call)) {
	135	+ unregistered += 1;
	136	+ const name = enclosingFn(macos_source, hit) orelse "(unknown)";
	137	+ std.debug.print(
	138	+ "\nwindow constructor does not call {s}: {s}\n",
	139	+ .{ register_call, name },
	140	+ );
	141	+ }
	142	+ }
	143	+
	144	+ // Floor: craft has three window constructors. If a refactor renames the
	145	+ // initialiser this scan finds none and would otherwise pass having checked
	146	+ // nothing at all.
	147	+ try testing.expect(constructors >= 3);
	148	+ try testing.expectEqual(@as(usize, 0), unregistered);
	149	+}
	150	+
	151	+test "whether a window is craft's is recorded, never inferred from the window" {
	152	+ // The specific regression. `isCraftWindow` asked the window what its
	153	+ // content view was; three of craft's four window styles answer with a
	154	+ // container rather than the webview, so those windows were silently
	155	+ // skipped by reopen.
	156	+ //
	157	+ // Any re-introduction reads the window instead of the registry, and every
	158	+ // way of doing that goes through one of these.
	159	+ const start = std.mem.indexOf(u8, macos_source, "fn isCraftWindow(") orelse {
	160	+ // Renamed or removed: this test can no longer defend anything and must
	161	+ // say so rather than pass.
	162	+ return error.IsCraftWindowNotFound;
	163	+ };
	164	+ const body = enclosingFnBody(macos_source, start);
	165	+
	166	+ for ([_][]const u8{
	167	+ "contentView",
	168	+ "isKindOfClass:",
	169	+ "contentViewController",
	170	+ "subviews",
	171	+ }) |inference| {
	172	+ if (std.mem.indexOf(u8, body, inference) != null) {
	173	+ std.debug.print("\nisCraftWindow inspects the window ({s}) instead of the registry\n", .{inference});
	174	+ return error.WindowIdentityInferred;
	175	+ }
	176	+ }
	177	+
	178	+ // And it must actually consult the registry, rather than being stubbed to
	179	+ // a constant that passes the check above.
	180	+ try testing.expect(callsFunction(body, "window_registry.isKnown"));
	181	+}
	182	+
	183	+test "the reopen handler filters by that answer" {
	184	+ // Registration is worth nothing if the reopen loop stops asking. This
	185	+ // pins the one place the two meet.
	186	+ const start = std.mem.indexOf(u8, macos_source, "fn appShouldHandleReopen(") orelse
	187	+ return error.ReopenHandlerNotFound;
	188	+ const body = enclosingFnBody(macos_source, start);
	189	+
	190	+ try testing.expect(callsFunction(body, "isCraftWindow("));
	191	+ try testing.expect(callsFunction(body, "makeKeyAndOrderFront:"));
	192	+}
	193	+
	194	+test "a closed window survives long enough to be reopened" {
	195	+ // `releasedWhenClosed` defaults to YES for a window built with
	196	+ // `initWithContentRect:`, so without this the window is deallocated on
	197	+ // close and reopening it is a use-after-free rather than a missing
	198	+ // feature. It is set in the same helper that registers, so that a
	199	+ // constructor cannot get one without the other.
	200	+ const start = std.mem.indexOf(u8, macos_source, "fn keepWindowAfterClose(") orelse
	201	+ return error.HelperNotFound;
	202	+ const body = enclosingFnBody(macos_source, start);
	203	+
	204	+ try testing.expect(callsFunction(body, "setReleasedWhenClosed:"));
	205	+ try testing.expect(callsFunction(body, "rememberCraftWindow("));
	206	+}
	207	+
	208	+test "the scan can actually find the constructors it claims to check" {
	209	+ // Guards the guard: every test above depends on `enclosingFnBody` locating
	210	+ // real function bodies. If the file's formatting changes such that it
	211	+ // returns the whole file, the checks above pass regardless of the code.
	212	+ const start = std.mem.indexOf(u8, macos_source, "fn isCraftWindow(").?;
	213	+ const body = enclosingFnBody(macos_source, start);
	214	+
	215	+ try testing.expect(body.len > 0);
	216	+ // A body that large means the scan lost its bearings and every check above
	217	+ // is now looking at unrelated code.
	218	+ try testing.expect(body.len < macos_source.len / 10);
	219	+ try testing.expect(std.mem.startsWith(u8, body, "fn isCraftWindow("));
	220	+}
	221	+
	222	+test "a commented-out call does not count as a call" {
	223	+ // The weakness that made the constructor check toothless: substring
	224	+ // matching cannot tell a call from a mention, so commenting a registration
	225	+ // out satisfied it. Found by doing exactly that and watching the test pass.
	226	+ try testing.expect(callsFunction(" keepWindowAfterClose(window);", "keepWindowAfterClose("));
	227	+ try testing.expect(!callsFunction(" // keepWindowAfterClose(window);", "keepWindowAfterClose("));
	228	+ try testing.expect(!callsFunction(" /// keepWindowAfterClose(window);", "keepWindowAfterClose("));
	229	+ try testing.expect(!callsFunction(" _ = x; // keepWindowAfterClose(window);", "keepWindowAfterClose("));
	230	+ // A real call on a line that also carries a trailing comment still counts.
	231	+ try testing.expect(callsFunction(" keepWindowAfterClose(window); // why", "keepWindowAfterClose("));
	232	+}