| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 5m 9s —— View job Code Review
Overall verdict: ✅ Approve with minor concernsAll four changes address real, well-evidenced CI reliability defects. The root causes are correctly identified, the fixes are narrowly scoped, and the PR description is unusually thorough. No blocking issues. 1 medium finding, 3 minor findings posted as inline comments:
Cross-cutting notesUNONative.m — The null guard is correctly placed only on uno_native_dispose, which is the only entry point callable with a zero handle from C# (via DisposeNativePeer without going through TryGetLiveElement). All other native functions are gated by TryGetLiveElement on the C# side. The asymmetry is intentional and correct. Given_ClientHotReloadProcessor.cs — The RID-to-platform mapping is clean: only recognized prefixes are mapped, the null oracle path adds a non-empty-platform assertion rather than a hard equality check, and the test is correctly void (no await). The removal of the [PlatformCondition(Exclude, SkiaAndroid)] workaround is correctly motivated. Timeout cancellation pattern (UnitTestsControl.cs:1025-1039) — The using var timeoutCts + timeoutCts.Cancel() before the using block exits is the correct pattern: the delay task is cancelled when the test finishes first (no lingering timer per test), and a cancelled Task.Delay becomes TaskStatus.Canceled (not faulted), so no unobserved-exception risk. DisposeNativePeer ordering — Capturing the handle before setting Disposed = true and zeroing NativeHandle is correct: the guard runs first, the state transition is atomic enough on the UI thread, and the native call uses the captured (non-zero) pointer. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 4m 49s —— View job Code Review
Overall verdict: ✅ Approve with minor concernsTwo new commits were added since the previous review. Both are correct and clean. Commit test(hotreload): Assert on page content that exists (Given_HotReloadResilience.cs) — Straightforward fix: the page declares FirstPageTextBlock (Text="First page"), not tb1/tb/Hello. Both tests now reference the element and text that actually exist in HR_Frame_Pages_Page1. No issues. Commit test(hotreload): Fail fast when a file edit matches nothing (HotReloadHelper.cs) — The EnsureOriginalTextIsPresent guard is well-placed (called after both early-return guards, only when a message will actually be sent). The edited = false flag pattern in both UpdateProjectFileAndRevert and UpdateServerFileAndRevert correctly prevents a double-throw when the initial edit failed. The finally-block in When_VisualTree_Paused_Then_ReloadCompleted_StillFires remains safe: the outer try-finally is never entered if UpdateServerFile throws inside the using block, so no unintended revert can fire. One pre-existing issue worth noting (not in this PR's diff, so no inline comment): UpdateServerFile(string filePathInProject, …) at line ~110 has if (message is null) { return; } on a value that can never be null (new UpdateSingleFileRequest { … }). This is a copy-paste artifact from the generic UpdateServerFile<T> overload where CreateUpdateFileMessage CAN return null. Harmless dead code, but worth cleaning up separately. Outstanding findings (from previous review, still open)The four findings from the previous review pass are not addressed in the two new commits:
|
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 6m 20s —— View job Code Review
Overall verdict: ✅ Approve with minor concernsThis review covers the two commits in this synchronize: fix(macos) and test: Bound every runtime test body. Both are correct and address real CI reliability defects with well-evidenced root causes. No new blocking issues. 4 outstanding findings from prior review passes remain unaddressed:
Cross-cutting notes on the new commits: fix(macos) — MacOSNativeElementHostingExtension.cs + UNONative.m The root-cause analysis is correct: Unloaded fires on reparent, the old code called uno_native_dispose and walked away leaving NativeHandle stale, and the subsequent uno_native_attach on the freed pointer caused ARC to retain freed memory. The fix is properly terminal on both sides:
test: Bound every runtime test body — UnitTestsControl.cs The using var timeoutCts + timeoutCts.Cancel() before the WhenAny result is inspected is the correct pattern: the delay task is cancelled as soon as the test body finishes (no lingering timer per test), and a cancelled Task.Delay becomes TaskStatus.Canceled (not faulted), so no unobserved-exception risk. The DEBUG/release split (null vs 5 min) and the UNO_TEST_DEFAULT_TIMEOUT_SECONDS override path are clean. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 228603 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 135 Details
|
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 2m 56s —— View job Code Review
Overall verdict: ⚠️ Fix before mergeThis review covers the current state of the 3 changed files (UnitTestsControl.cs, MacOSNativeElementHostingExtension.cs, UNONative.m). The Hot Reload test files are correctly scoped out to #24125. 1 new medium finding posted as an inline comment. 3 prior findings remain unaddressed. Outstanding findings
What's correct
|
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 228879 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 195 Details
|
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 2m 45s —— View job Code Review
Overall verdict: ⚠️ Fix before mergeThis review covers the current state of the 3 changed files (UnitTestsControl.cs, MacOSNativeElementHostingExtension.cs, UNONative.m). The commits are unchanged since the prior review pass — all 4 outstanding findings remain unaddressed. Outstanding findings (all from prior passes, no new issues found)
What is correct
|
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 229240 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 178 Details
|
Sorry, something went wrong.
|
Claude finished @Xiaoy312's task in 3m 47s —— View job Code Review
Overall verdict: ⚠️ Fix before mergeThis review covers the new commit fix(macos): Drop the stale WebView2 handle copy (new file MacOSNativeWebView.cs) plus the two commits carried from prior passes. The stale-handle fix is correct and clean. 4 findings from prior passes remain unaddressed; 1 new finding and 1 follow-up concern added. Outstanding findings from prior passes (all still open)
New findings (this pass)
Xiaotian Gu's concern — synchronous tests not covered by the timeoutThe new 5-minute timeout in UnitTestsControl.cs only applies to Task-returning test methods (line 1017: if (test.Method.ReturnType == typeof(Task))). Synchronous void tests that deadlock or spin are still unguarded and can hold the macOS CI job for the full 6 hours observed. If the hung test on macOS is synchronous, this PR's timeout won't catch it. A follow-up should either add a thread-abort/process-kill safety valve for sync tests, or require all new tests on the macOS leg to be async. What is correct in the new commit
|
Sorry, something went wrong.
- Fix TaskCompletionSource<string> to TaskCompletionSource<string?> in ExecuteScriptAsync so it matches the callback cast - Guard content?.GetType() in TryGetLiveElement to avoid NullReferenceException when content is null - Return new Size(0, 0) from MeasureNativeElement for disposed peers instead of Size.Empty (-∞, -∞) which caused layout exceptions Co-authored-by: MartinZikmund <1075116+MartinZikmund@users.noreply.github.com>
Head branch was pushed to by a user without write access
Addressed in the latest commit:
|
Sorry, something went wrong.
|
|
Sorry, something went wrong.
`TryGetLiveElement` and `TryGetHandle` are reached on every arrange, measure, attach and detach of a reparented element, so `Error` put a per-layout-pass stream into the channel reserved for genuine faults. The disposed peer is a known degraded path, not an unexpected state. Also from review: - `DisposeNativePeer` skips the P/Invoke when the handle is already zero, which happens when the peer was never created. - `ExecuteScriptAsync` awaits inside the `token.Register` scope, matching `InvokeScriptAsync`. It previously disposed the registration before awaiting, so a cancel arriving after the P/Invoke never reached the pending task. - Records why the disposed-peer measure fallback cannot be `Size.Empty`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEzBHbVj75RX9ENGARb6sL
|
Claude finished @MartinZikmund's task in 2m 33s —— View job Code Review
Overall verdict: ✅ ApproveAll six outstanding findings from prior review passes are resolved in the two new commits (d6462d43 by Copilot and f81eec0b by @MartinZikmund). No new issues found. No inline comments to post. Prior findings — all addressed
Bonus fix (noted in f81eec0b): ExecuteScriptAsync now awaits inside the using (token.Register(...)) block, matching InvokeScriptAsync. The prior code disposed the CancellationTokenRegistration before the await completed, so cancellation arriving after uno_webview_execute_script dispatched could never reach the pending task. This is now correctly wired for the full async duration. What is correct across all three files
|
Sorry, something went wrong.
🤖 Addressed PR comments up to this point21 review threads (12 distinct points — the review bot re-posted four of them across three runs). Code in d6462d4 (Copilot agent) and f81eec0. Fixed
Pushed back
Moved out of this PR
Filed for follow-up — from @Xiaoy312's review notes
@Xiaoy312's thread on the 6-hour macOS run is answered but left open — the retry-convergence question is his and @MartinZikmund's call. @Xiaoy312 your WebView2 commit stays in this PR with authorship intact; it is the direct cause of the second crash signature, so splitting it out would have separated a fix from its evidence. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 229613 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 185 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 229613 has failed on Uno.UI - CI. |
Sorry, something went wrong.
`MacOSNativeElement` destroyed its NSView from `Unloaded`, but `Unloaded` also fires on a reparent and the framework re-enters the very same element through `AttachNativeElement`. Every operation on the way back in was then refused, so a `WebView2` removed from and re-added to the tree came back dead — which is why `When_WebMessageReceived_After_RemoveAdd` failed the macOS runtime-test job. The X11 and Win32 hosting extensions never destroyed their peer on unload either. Root-cause fix: the peer's lifetime follows the managed wrapper. The finalizer releases it, dispatched to the main thread since AppKit is main-thread only, and `uno_native_attach` drops the strong reference `uno_native_detach` took so `transients` holds only genuinely detached views. The disposed-handle guards stay as hardening — they can no longer be reached through a reparent. `addScriptMessageHandler:name:` raises on a name that is already registered, and the WKWebView now survives the round trip, so registration removes the previous handler first and `OnUnloaded` unregisters symmetrically with `OnLoaded`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LKUHFiW2J8nNFy7FmuCsd
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 229653 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 170 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 229653 has failed on Uno.UI - CI. |
Sorry, something went wrong.
`uno_webview_create`, `uno_mediaplayer_create_view` and `uno_native_create_sample` all end in `objc_autoreleaseReturnValue` (confirmed in the shipped dylib), so the handle they hand back is owned by nothing but the current autorelease pool. The native side only took a strong reference at `uno_native_attach`, which adds the view to `elements`. Any element that is never attached, or whose attach lands in a later run-loop turn than its creation, is therefore already deallocated — and both `uno_native_attach` and `uno_native_dispose` begin with an ARC `objc_retain` of their parameter, so the next call takes the process down at `+0x1f`. That is the SIGSEGV the macOS runtime-test job keeps hitting: three CI core dumps from this branch all fault at the same image offset, which symbolizes to `uno_native_dispose` retaining a freed NSView. `uno_native_track` takes the strong reference at creation instead. The peer is now owned continuously — `transients` from creation, `elements` while attached, back to `transients` on detach — until `uno_native_dispose` drops both and deallocates it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LKUHFiW2J8nNFy7FmuCsd
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 229659 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 191 Details
|
Sorry, something went wrong.
`drawInMTKView:` returned without calling managed code when `currentDrawable` was nil. The view runs with `enableSetNeedsDisplay`, so AppKit had already cleared `needsDisplay` to make that call, and the managed side latches its own request: `CompositionTarget.RequestNewFrame` sets `RenderRequested` and only clears it from `OnNativePlatformFrameRequested`, which the dropped frame never reaches. Every later `RequestNewFrame` then coalesced into an invalidation that had already been consumed, so a single nil drawable stopped the window rendering permanently. Anything awaiting a frame after that waits forever — `UITestHelper.WaitForRender` has no timeout of its own — which is how one macOS test takes the whole 60-minute job down with it and publishes no results. The nil case now re-arms `needsDisplay` on a later main-queue turn, so the frame request survives. The software path never had the defect: it calls the managed callback before its own early return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LKUHFiW2J8nNFy7FmuCsd
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24112/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 229660 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 169 Details
|
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
GitHub Issue: closes #24111
PR Type:
🐞 Bugfix
What changed? 🚀
macOS native-peer lifetime defects that take down the whole Tests - Desktop Skia macOS job with a SIGSEGV, losing the results of every other test in the run.
Root cause: native peers were created unowned
uno_webview_create, uno_mediaplayer_create_view and uno_native_create_sample all end in objc_autoreleaseReturnValue — confirmed by disassembling the shipped dylib, not by reading the source. The handle they hand back to managed code is owned by nothing but the current autorelease pool. The native side only took a strong reference later, at uno_native_attach, which adds the view to the elements set.
So any element that is never attached, or whose attach lands in a later run-loop turn than its creation, is already deallocated. Both uno_native_attach and uno_native_dispose begin with an ARC objc_retain of their parameter, so the next call takes the process down at +0x1f.
Three CI core dumps from this branch all fault at the same image offset, which symbolizes to uno_native_dispose retaining a freed NSView.
Fix: uno_native_track takes the strong reference at creation. Ownership is now continuous — transients from birth, elements while attached, back to transients on detach — until uno_native_dispose drops both and deallocates.
Peers no longer die on a reparent
MacOSNativeElement destroyed its NSView from Unloaded, but Unloaded also fires on a reparent and the framework re-enters the very same element through AttachNativeElement. A WebView2 removed from and re-added to the tree came back dead — which is what failed When_WebMessageReceived_After_RemoveAdd. Neither the X11 nor the Win32 hosting extension destroys its peer on unload.
The peer's lifetime now follows the managed wrapper: the finalizer releases it, dispatched to the main thread since AppKit is main-thread only. The disposed-handle guards stay as hardening — they can no longer be reached through a reparent.
addScriptMessageHandler:name: raises on a name that is already registered, and the WKWebView now survives the round trip, so registration removes the previous handler first and OnUnloaded unregisters symmetrically with OnLoaded. The stale private _webview shadow copy of the handle is gone; every call resolves through TryGetHandle.
A dropped frame no longer stops rendering permanently
drawInMTKView: returned without calling managed code when currentDrawable was nil. The view runs with enableSetNeedsDisplay, so AppKit had already cleared needsDisplay to make that call, and the managed side latches its own request: CompositionTarget.RequestNewFrame sets RenderRequested and only clears it from OnNativePlatformFrameRequested, which the dropped frame never reaches. Every later RequestNewFrame then coalesced into an invalidation that had already been consumed, so a single nil drawable stopped that window rendering for good. It now re-arms needsDisplay on a later main-queue turn. The software path never had the defect — it calls the managed callback before its own early return.
Validation
One claim to correct from an earlier revision of this description: the nil-drawable fix was described as the cause of the macOS 60-minute job hangs. That is not established. UITestHelper.WaitForRender does have a 1000 ms bound, and NativeDispatcher.TryGetRenderAction consumes a render action one-shot, so a dead render loop makes a test fail in about a second rather than hang. The latch is a genuine defect worth fixing on its own terms; it is not the hang's explanation. The hang reproduces on feature/breakingchanges without any of this PR's code and is tracked separately.
PR Checklist ✅
🤖 Generated with Claude Code
https://claude.ai/code/session_014LKUHFiW2J8nNFy7FmuCsd