Repository navigation
browser: non-permanent discard blockers + one shared discard event center - #7625
austinywang wants to merge 5 commits into
Conversation
…nter Hidden-webview discard fixes from the #7596 audit (224 browser panes held a ~5 GB hidden WebKit tree): - media_playback blocked discard for ANY playing media, so a muted autoplaying video immortalized its hidden pane's WebContent process. The blocker now keys on audible playback (background audio still blocks; camera/mic capture blocking unchanged; the #5409 media glyph plumbing is untouched). - developer_tools blocked on a persisted intent flag that can outlive the actual inspector; it now keys on the live inspector probe only. - A blocked hidden pane was never re-checked (blockers re-evaluated only on discrete events), so any stale blocker permanently prevented discard. Blocked hidden discard candidates now arm a coarse one-shot re-check timer (max(60s, hidden delay)) with the same generation and instance guards as the discard timer; terminal states (visible, closing, discarded, policy disabled) never arm it. - Every panel's discard manager registered its own UserDefaults observer plus sleep/wake observers (~672 observers at 224 panes, all doing identical work). One shared BrowserHiddenWebViewDiscardEventCenter now owns a single defaults observer and one sleep/wake pair, diffs the resolved policy once, and fans out to weakly-held managers. Part of #7596 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a shared discard-event center, routes hidden-webview discard manager subscriptions through it, adds blocked recheck scheduling, and updates media blocker handling from ChangesHidden WebView Discard Event Center and Blocked Recheck
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UserDefaults
participant BrowserHiddenWebViewDiscardEventCenter
participant BrowserHiddenWebViewDiscardManager
participant BrowserPanel
UserDefaults->>BrowserHiddenWebViewDiscardEventCenter: didChangeNotification
BrowserHiddenWebViewDiscardEventCenter->>BrowserHiddenWebViewDiscardEventCenter: re-resolve discard policy
BrowserHiddenWebViewDiscardEventCenter->>BrowserHiddenWebViewDiscardManager: discardPolicyDidChange
BrowserHiddenWebViewDiscardManager->>BrowserHiddenWebViewDiscardManager: scheduleIfNeeded()
BrowserHiddenWebViewDiscardManager->>BrowserPanel: update hidden-webview discard state
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR updates hidden browser webview discard behavior and centralizes its event handling. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "browser: avoid main-actor .shared defaul..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// discard policy state for every defaults write. | ||
| @MainActor | ||
| final class BrowserHiddenWebViewDiscardEventCenter { | ||
| static let shared = BrowserHiddenWebViewDiscardEventCenter() |
There was a problem hiding this comment.
This line adds a process-wide runtime owner for discard observer state, and every manager now defaults to it. That makes subscriber and policy fan-out ownership ambient instead of scoped to the browser owner, so separate browser lifetimes cannot construct and tear down their discard event source at the app seam.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
The process-wide singleton is the point of this change: issue #7596 measured ~224 hidden panes each installing 3 observers that re-resolved the same global UserDefaults policy on every defaults write. The observed state (app-wide defaults + system sleep/wake) is inherently process-global, so one owner is the correct scope. The initializer keeps injectable seams (defaults, both notification centers, observerQueue) and BrowserHiddenWebViewDiscardManager accepts an injected center, so tests construct isolated instances — only the production default is shared. There is no per-browser-owner lifetime seam in the app today for this to attach to.
There was a problem hiding this comment.
That's a fair correction. When the observed state is inherently process-global (UserDefaults + NSWorkspace sleep/wake), a process-wide singleton is the correct scope — not a smell. The injectable seams and injected-center constructor on BrowserHiddenWebViewDiscardManager address the isolation concern for tests, which is the real requirement. My comment about "separate browser lifetimes" assumed an architectural boundary that doesn't exist here.
Withdrawing the concern.
Shall I remember this as a rule: "singletons wrapping inherently process-global state (UserDefaults, NSWorkspace sleep/wake) are acceptable in this codebase as long as injectable seams exist for tests"?
There was a problem hiding this comment.
Yes, please remember that rule.
| var subscriberCountForTesting: Int { | ||
| compactSubscribers() | ||
| return subscribers.count | ||
| } | ||
|
|
||
| var observerInstallCountForTesting: (defaults: Int, workspace: Int) { | ||
| (defaultsObserver == nil ? 0 : 1, sleepObservers.count) | ||
| } |
There was a problem hiding this comment.
These ForTesting accessors expose private event-center state from a production Sources/ file and compile into the app target. The tests only need observer/subscriber counts, so this adds test-only surface to shipping code instead of observing internal state through @testable import or test-target scaffolding.
Rule Used: Flag Swift files under a production Sources path (... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
*ForTesting members in production sources are the established convention in this codebase — 218 existing occurrences across 20+ files under Sources/ (e.g. TabManager.swift, AppDelegate.swift, TerminalController.swift). cmuxTests compiles the app target sources directly, so the accessors follow the same pattern as their siblings. Keeping the counters behind explicit ForTesting names is preferable to widening the underlying stored properties' access.
There was a problem hiding this comment.
Fair point — with 218 existing occurrences of the same pattern across production sources, this is clearly the codebase convention and not an anomaly. The ForTesting suffix naming convention already provides the right signal at the call site. I'll withdraw this concern.
There was a problem hiding this comment.
Correcting my earlier reply: the existing ForTesting occurrences are grandfathered, but the repo has an explicit rule against adding new ones — .github/review-bot-rules/no-test-debug-seam-in-production-source.md (canonical fix: #6452). Your original finding was right for this codebase. Fixed in 4749a2c: accessors removed, state widened to internal private(set), tests read it via @testable import. Please remember the rule as: new test/debug seams (ForTesting-style members) must not be added to production Sources/; tests reach internal state via @testable import instead.
There was a problem hiding this comment.
Thanks for the correction — my original finding was right and I should have held the position. Rule saved: new ForTesting-style seams must not be added to production Sources/; tests use @testable import to reach internal state instead.
| observedWebViewInstanceID: UUID, | ||
| generation: UInt64 | ||
| ) { | ||
| guard blockedRecheckTimer == nil else { return } | ||
| let policy = BrowserHiddenWebViewDiscardPolicy.resolved(defaults: policyDefaults) |
There was a problem hiding this comment.
The blocked-pane self-heal is driven by a production timer, and every later scheduleIfNeeded call cancels and recreates it before checking blockers. When memory pressure keeps retrying immediate discard while a blocker is still present, each retry restarts the countdown, so the hidden pane can stay alive until those retries stop instead of rechecking from the original blocker time.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
There was a problem hiding this comment.
The memory-pressure premise doesn't hold: requestImmediateDiscardIfSafe returns false on the blocker guard before touching any timer state, so pressure retries while blocked never cancel or restart the recheck timer (see the guard at the top of the method — the blockedRecheckTimer?.cancel() only runs after blockers pass).
For scheduleIfNeeded: it does cancel and re-arm the recheck, but every such call performs a full fresh blocker evaluation at that instant — a reset is always accompanied by an immediate re-check, so the self-heal invariant ("a blocked hidden candidate always has either a just-completed evaluation or an armed recheck") holds and no evaluation is ever lost. Keeping the original deadline across resets would add generation-tracking complexity without changing when blockers are actually observed. The recheck is deliberately coarse (max(60s, hiddenDelay)) and never bypasses blockers.
There was a problem hiding this comment.
You're right on both counts.
For requestImmediateDiscardIfSafe, the second guard exits before any timer state is touched — pressure retries while blocked are pure no-ops and cannot interfere with the recheck timer. That leg of my concern was wrong.
For scheduleIfNeeded: the cancel-then-re-arm pattern does reset the recheck deadline, but since the scheduleBlockedRecheckIfNeeded guard (blockedRecheckTimer == nil) passes immediately after the cancel, every reset produces a fresh evaluation and a new armed timer in the same call. No evaluation window is lost. The 60 s floor makes the deadline drift inconsequential in practice.
Both points stand — the self-heal invariant holds. Thanks for walking through the control flow.
| guard delegate.hiddenWebViewDiscardWebViewInstanceID == observedWebViewInstanceID else { return } | ||
| self.blockedRecheckTimer?.cancel() | ||
| self.blockedRecheckTimer = nil | ||
| self.scheduleIfNeeded(reason: reason, now: Date()) |
There was a problem hiding this comment.
When the runtime blocked-recheck timer fires, this call reuses the original scheduling reason captured when the pane was blocked. If blockers have cleared and the pane is discarded, the delegate receives that stale reason instead of blocked_recheck, so discard diagnostics and any reason-based handling report the wrong trigger.
| self.scheduleIfNeeded(reason: reason, now: Date()) | |
| self.scheduleIfNeeded(reason: "blocked_recheck", now: Date()) |
There was a problem hiding this comment.
Fixed in a7aeb4b: the production recheck timer now passes "blocked_recheck", matching performScheduledBlockedRecheckForTesting and the regression test that asserts lastDiscardReason == "blocked_recheck". The now-unused reason parameter was removed from scheduleBlockedRecheckIfNeeded.
| if snapshot.isCapturingMedia { blockers.append("media_capture") } | ||
| if snapshot.isPlayingMedia { blockers.append("media_playback") } | ||
| if snapshot.preferredDeveloperToolsVisible || snapshot.isDeveloperToolsVisible { | ||
| if snapshot.hasAudibleMedia { blockers.append("media_playback") } | ||
| if snapshot.isDeveloperToolsVisible { |
There was a problem hiding this comment.
This now ignores preferredDeveloperToolsVisible and only blocks when the live inspector probe is true. If the user has requested DevTools but the live probe is temporarily false during inspector startup or hidden-pane transition, the pane becomes discardable and the WebView can be destroyed before the inspector state becomes visible.
There was a problem hiding this comment.
Intentional, and the core of fix #2 in the PR description: preferredDeveloperToolsVisible is a persisted intent flag, so a stale value immortalized a hidden pane's WebContent process forever even with no inspector attached — exactly the permanent-blocker class this PR removes. The startup race is bounded and benign: discard only fires for panes hidden ≥ hiddenDelay (default 300s) with blockers re-evaluated at fire time, so an inspector that actually opens flips the live probe long before the countdown lands; in the worst case the discard is non-destructive (session state is retained and the pane restores on reveal). A permanent leak on one side vs. a rare re-open of the inspector on the other.
The runtime blocked-recheck timer reused the reason captured when the pane was first blocked, while performScheduledBlockedRecheckForTesting (and the regression test asserting lastDiscardReason) used "blocked_recheck". Align production with the tested contract so discard diagnostics report the actual trigger. Addresses Greptile review on #7625. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| blockedRecheckTimer?.cancel() | ||
| blockedRecheckTimer = nil |
There was a problem hiding this comment.
This cancels the blocked-recheck timer before checking whether the pane is still blocked. Hidden blocked panes can re-enter scheduleIfNeeded from normal reevaluations such as loading, media, download, popup, or DevTools state changes. If the blocker is still present, each reevaluation cancels the existing recheck and starts a new full delay. A pane stuck behind a long provisional navigation can keep pushing the self-heal timer out and remain undiscarded after the original recheck should have run.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
This is the same control flow resolved in the thread on scheduleBlockedRecheckIfNeeded (#7625 (comment)), where the conclusion was: every reset produces a fresh evaluation and a new armed timer in the same call, so no evaluation window is lost.
Concretely: the cancel at the top of scheduleIfNeeded is always immediately followed by a full blocker evaluation in that same call. The recheck timer exists only for blockers whose clearing emits no discrete event; pushing the deadline out requires continuous event churn, and each churn event is itself a fresh evaluation confirming the pane is still blocked — correct behavior, not a stall. For the provisional-navigation example: while the navigation is in flight every re-entry re-evaluates; when it completes, the navigation delegates fire a discrete re-evaluation that arms the real discard timer, and if no completion event fires, the last armed recheck fires after max(60s, hiddenDelay). There is no path where the pane stays undiscarded without an evaluation having just confirmed a live blocker.
Preserving the original deadline across resets would add generation bookkeeping without changing when blocker state is actually observed, so keeping the simple cancel-then-re-arm is deliberate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserHiddenWebViewDiscardEventCenter.swift`:
- Around line 61-68: The production-only debug seam is exposed through the
`BrowserHiddenWebViewDiscardEventCenter` properties `subscriberCountForTesting`
and `observerInstallCountForTesting`. Rename these members to drop the
`ForTesting` suffix (for example, `subscriberCount` and `observerInstallCount`)
or wrap them in `#if DEBUG` so they are not compiled into production source, and
update any test references that use these accessors to match the new names.
- Around line 70-100: Remove the test-only accessors from
BrowserHiddenWebViewDiscardEventCenter in production code:
subscriberCountForTesting and observerInstallCountForTesting should not live in
Sources. Move any test-only inspection into the test target or rely on `@testable`
import to access the internal state directly, while keeping installObservers and
the production notification wiring unchanged.
In `@Sources/Panels/BrowserHiddenWebViewDiscardManager.swift`:
- Around line 160-168: The test-only recheck helper should not be exposed on the
production type. Remove or hide performScheduledBlockedRecheckForTesting from
BrowserHiddenWebViewDiscardManager, and instead make the underlying blocked
recheck path internal/private so BrowserHiddenWebViewDiscardMemoryPressureTests
can reach it via `@testable` import, or update the test to exercise the existing
scheduleIfNeeded state flow directly. Keep the production API free of any
...ForTesting seam while preserving the same blocked_recheck behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4fab393-4a6e-48c1-af53-94adede86dae
📒 Files selected for processing (8)
Sources/Panels/BrowserHiddenWebViewDiscardEventCenter.swiftSources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserHiddenWebViewDiscardEventCenterTests.swiftcmuxTests/BrowserHiddenWebViewDiscardMemoryPressureTests.swiftcmuxTests/BrowserMediaPlaybackAudioActivityTests.swiftcmuxTests/BrowserPanelTests.swift
Per .github/review-bot-rules/no-test-debug-seam-in-production-source.md (canonical fix: #6452), production Sources/ must not grow new ...ForTesting members: - Event center: drop subscriberCountForTesting and observerInstallCountForTesting; widen defaultsObserver, sleepObservers, and subscribers (plus WeakSubscriber) to internal private(set) so the tests observe them via @testable import. - Manager: rename performScheduledBlockedRecheckForTesting to performBlockedRecheckNow and route the production blocked-recheck timer handler through it, so production and tests share one re-check fire path instead of the test helper duplicating the handler body. Addresses CodeRabbit review on #7625. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/BrowserHiddenWebViewDiscardManager.swift (1)
111-122: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReplace the blocked-recheck timer with causal blocker-clear signals.
This adds a production
DispatchSourceTimeras a coarse self-healing path for hidden-webview blocker state. That leaves discard correctness dependent on a delayed poll instead of the real state transition that cleared loading, DevTools, media, popup, fullscreen, download, or capture blockers.Route each blocker-clear event through the shared discard action path / event center and re-run scheduling immediately; fail closed if a reliable signal is missing.
As per coding guidelines, production Swift must flag timing-based coordination such as timers/polling when used to paper over lifecycle/rendering/shared-state races. As per path instructions, hidden-webview discard decisions should avoid throttling/polling staleness and prefer structured lifecycle/typed events.
Also applies to: 297-323
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserHiddenWebViewDiscardManager.swift` around lines 111 - 122, The blocked-recheck timer in BrowserHiddenWebViewDiscardManager is a polling fallback that should be removed in favor of causal blocker-clear signals. Update the hidden-webview discard flow so each blocker-clearing event (loading, DevTools, media, popup, fullscreen, download, capture) routes through the shared discard action path/event center and triggers scheduling immediately via the existing schedule logic, rather than waiting for blockedRecheckTimer or scheduleBlockedRecheckIfNeeded. Keep the behavior fail-closed when a reliable clear signal is missing, and use the existing symbols blockedRecheckTimer, scheduleBlockedRecheckIfNeeded, and hiddenWebViewDiscardSnapshot to locate the affected paths.Sources: Coding guidelines, Path instructions
Sources/Panels/BrowserHiddenWebViewDiscardEventCenter.swift (1)
64-95: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
MainActor.assumeIsolatedrelies onobserverQueuealways being main-associated.Both observer closures call
MainActor.assumeIsolatedunconditionally, which is only safe becauseobserverQueuedefaults to.main(and tests passnil, delivering synchronously on the calling actor). If a future call site passes a non-mainOperationQueue, the closure will run off the main thread andassumeIsolatedwill trap at runtime. Consider documenting this constraint on theobserverQueueparameter, or assertingqueue?.underlyingQueue === DispatchQueue.maindefensively.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserHiddenWebViewDiscardEventCenter.swift` around lines 64 - 95, The observer closures in BrowserHiddenWebViewDiscardEventCenter’s installObservers() unconditionally call MainActor.assumeIsolated, so they must only ever run on the main actor. Make the observerQueue constraint explicit in the initializer or installObservers() by documenting that only .main or nil is allowed, and add a defensive precondition/assertion before registering observers to verify the queue is main-associated (for example via underlyingQueue). This keeps handleDefaultsChanged(), notifySystemWillSleep(), and notifySystemDidWake() from trapping if a non-main OperationQueue is passed later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Panels/BrowserHiddenWebViewDiscardEventCenter.swift`:
- Around line 64-95: The observer closures in
BrowserHiddenWebViewDiscardEventCenter’s installObservers() unconditionally call
MainActor.assumeIsolated, so they must only ever run on the main actor. Make the
observerQueue constraint explicit in the initializer or installObservers() by
documenting that only .main or nil is allowed, and add a defensive
precondition/assertion before registering observers to verify the queue is
main-associated (for example via underlyingQueue). This keeps
handleDefaultsChanged(), notifySystemWillSleep(), and notifySystemDidWake() from
trapping if a non-main OperationQueue is passed later.
In `@Sources/Panels/BrowserHiddenWebViewDiscardManager.swift`:
- Around line 111-122: The blocked-recheck timer in
BrowserHiddenWebViewDiscardManager is a polling fallback that should be removed
in favor of causal blocker-clear signals. Update the hidden-webview discard flow
so each blocker-clearing event (loading, DevTools, media, popup, fullscreen,
download, capture) routes through the shared discard action path/event center
and triggers scheduling immediately via the existing schedule logic, rather than
waiting for blockedRecheckTimer or scheduleBlockedRecheckIfNeeded. Keep the
behavior fail-closed when a reliable clear signal is missing, and use the
existing symbols blockedRecheckTimer, scheduleBlockedRecheckIfNeeded, and
hiddenWebViewDiscardSnapshot to locate the affected paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07ff4cfb-693a-4536-9448-b99625ca96a7
📒 Files selected for processing (4)
Sources/Panels/BrowserHiddenWebViewDiscardEventCenter.swiftSources/Panels/BrowserHiddenWebViewDiscardManager.swiftcmuxTests/BrowserHiddenWebViewDiscardEventCenterTests.swiftcmuxTests/BrowserHiddenWebViewDiscardMemoryPressureTests.swift
Default-argument expressions are evaluated in a nonisolated context, so 'eventCenter: ... = .shared' referencing the @MainActor-isolated static tripped the Swift 6 actor-isolation warning and the CI warning budget (tests-build-and-lag). Default to nil and resolve .shared inside the main-actor-isolated init body instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Part of #7596 (memory audit, slice 5 — browser pane discard). The audit found 224 browser panes mapping to a ~5 GB resident WebKit helper tree; hidden-webview discard exists (on by default, 300 s) but its blocker list could immortalize hidden panes.
Four fixes:
media_playbackblocked discard for any playing media frame, audible or not — a muted autoplaying hero video kept a hidden pane's WebContent process alive forever. The blocker now consumes the already-existing audible signal (hasAudibleMedia): background audio still blocks (real use), silent playback does not.media_capture(camera/mic) is unchanged, and the issue-Browser pane stops media playback (YouTube video) when hidden — discard doesn't exempt active playback #5409 media-report/glyph plumbing is untouched — only blocker consumption changed.preferredDeveloperToolsVisible) with the live inspector probe; a stale intent flag with no actual inspector immortalized the pane. Now only the live probe blocks.scheduleIfNeededreturned silently when blocked, and re-evaluation happened only on discrete events — any blocker whose clearing event didn't fire blocked discard forever. Blocked hidden discard candidates now arm a coarse one-shot re-check timer (max(60 s, hiddenDelay), same generation-token + webview-instance guards and cancellation paths as the discard timer, cannot bypass blockers). Terminal states (visible, closing, already-discarded, policy-disabled) never arm it, so a system wake doesn't create per-pane timer churn at 224-pane scale.UserDefaults.didChangeNotificationobserver + 2 NSWorkspace sleep/wake observers, all re-resolving the same global policy on every defaults write. NewBrowserHiddenWebViewDiscardEventCenter(singleton, injectable seams for tests) owns exactly one defaults observer + one sleep/wake pair, diffs the resolved policy once per change, and fans out to weakly-held subscribing managers. Per-panel delegate behavior is unchanged.The issue's snapshot-and-release tier and a global live-WebContent cap are intentionally not in this slice (product-behavior changes; deferral rationale going on #7596). The post-wake discard guard (#5261) and memory-pressure immediate-discard path are unchanged.
Tests
cmuxTests/BrowserHiddenWebViewDiscardEventCenterTests.swiftwired into pbxproj (lint-pbxproj-test-wiring.shpasses).BrowserPanelTestsmedia helpers/tests renamed to audible semantics; blocker tests now use isolated suite defaults through thepolicyDefaultsseam instead of ambient.standard.Budgets:
BrowserPanel.swift11,640 ≤ 11,641 (net −1, 1:1 replacements),BrowserPanelTests.swiftexactly at 4,367 (net 0); no.github/*.tsvchanges;swift_file_length_budget.pypasses.No user-facing strings → no localization changes.
Part of #7596
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes when hidden WKWebViews are torn down (audible vs silent media, DevTools probe, periodic recheck) and centralizes sleep/wake and policy notifications—behavioral risk around background playback and post-wake discard timing, mitigated by existing guards and new tests.
Overview
Improves hidden-browser-pane memory reclaim (#7596) by fixing blockers that never cleared and collapsing hundreds of duplicate observers into one shared fan-out.
Blocker behavior:
media_playbacknow keys off audible media (hasAudibleMedia/isPlayingAudio) so muted autoplay no longer keeps WebContent alive; capture still blocks. DevTools blocking uses only the live inspector probe, notpreferredDeveloperToolsVisible. When scheduling is blocked but the pane is still a hidden discard candidate, a one-shot recheck (max(60s, hiddenDelay)) re-runsscheduleIfNeeded(reasonblocked_recheck); visible, closing, already-discarded, and policy-disabled panes do not arm it.Observer model: New
BrowserHiddenWebViewDiscardEventCenterowns a singleUserDefaultspolicy diff plus one sleep/wake pair and notifies weak subscribers;BrowserHiddenWebViewDiscardManagerdrops per-manager policy/sleep observers and usesinstallEventCenterSubscription()instead.BrowserPanelwires the new snapshot field and subscription API.Reviewed by Cursor Bugbot for commit 8e82e92. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Strengthens hidden-webview discard and reduces memory by fixing non-permanent blockers and moving policy/sleep observers to one shared event center. Aligns recheck reporting, removes test-only seams, and resolves a Swift 6 actor-isolation warning; addresses #7596.
Bug Fixes
Refactors
BrowserHiddenWebViewDiscardEventCenterto replace per-panelUserDefaultsand sleep/wake observers with one diffing fan-out and weak subscribers.installEventCenterSubscription(), removing duplicate observers at scale.performBlockedRecheckNowand removed...ForTestingseams; event-center internals areinternal private(set)for@testableaccess.eventCenteroptional in init and resolving.sharedinside the main-actor initializer.Written for commit 8e82e92. Summary will update on new commits.
Summary by CodeRabbit