Core - Cancel pending tasks when browser crashed or v8 context gets released #5145
Conversation
WalkthroughRefactors pending JavaScript task management to be frame-scoped, adds per-frame and global cancellation APIs, changes pending task creation to include a frame identifier, returns faulted tasks when the browser host is unavailable, updates task timeout logic, and adds tests for V8 context lifecycle, crashes, and callback execution. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Browser as ChromiumWebBrowser
participant Frame as CefFrameWrapper
participant Repo as PendingTaskRepository
participant Host as BrowserHost
Test->>Browser: EvaluateScriptAsync(...)
Browser->>Frame: EvaluateScriptAsync(...)
Frame->>Host: Check host
alt Host available
Frame->>Repo: CreatePendingTask(frameId, timeout)
Repo-->>Frame: Pending task created (TCS)
Frame->>Host: Send script for eval
Host-->>Frame: Eval result message (includes frameId)
Frame->>Repo: RemovePendingTask(frameId, taskId)
Repo-->>Frame: Complete Task -> Test
else Host not available
Frame-->>Test: Return faulted Task (InvalidOperationException)
end
sequenceDiagram
participant Browser
participant Repo as PendingTaskRepository
Browser->>Repo: CancelPendingTasks(frameId) or CancelPendingTasks()
Repo->>Repo: Dispose FramePendingTaskRepository(frameId) / Dispose all
Repo-->>Browser: Tasks canceled (per-frame or global)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs (1)
68-83: Consider reducing the loop sizes to make the test more practical.While the test correctly validates OOM cancellation behavior, the nested loops with 10M x 10M iterations creating 100M character strings could cause test instability or excessive runtime. Consider smaller values that still reliably trigger OOM.
- let array1 = []; - for (let i = 0; i < 10000000; i++) { - let array2 = []; - for (let j = 0; j < 10000000; j++) { - array2.push('a'.repeat(100000000)); - } - array1.push(array2); - } + let array1 = []; + for (let i = 0; i < 1000; i++) { + let array2 = []; + for (let j = 0; j < 1000; j++) { + array2.push('a'.repeat(10000000)); + } + array1.push(array2); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp(1 hunks)CefSharp.Core.Runtime/Internals/ClientAdapter.cpp(3 hunks)CefSharp.Core.Runtime/Internals/ClientAdapter.h(2 hunks)CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp(1 hunks)CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs(3 hunks)CefSharp/Internals/PendingTaskRepository.cs(2 hunks)CefSharp/Internals/TaskExtensions.cs(1 hunks)
🧠 Learnings (1)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (1)
Learnt from: amaitland
PR: #4475
File: CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp:147-154
Timestamp: 2025-05-10T23:58:24.668Z
Learning: The GetCache method of the IJavaScriptObjectCache implementation should never return null in its current implementation. Both the legacy and per-browser cache implementations guarantee a non-null return value, with the per-browser implementation returning an empty dictionary when no cache exists for a given browser ID.
🧬 Code Graph Analysis (3)
CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp (3)
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h (1)
CefBrowserWrapper(27-75)CefSharp.Core.Runtime/Internals/StringUtils.h (1)
ToNative(74-84)CefSharp.Core.Runtime/Internals/Serialization/Primitives.cpp (2)
SetInt64(41-49)SetInt64(41-41)
CefSharp.Core.Runtime/Internals/ClientAdapter.cpp (2)
CefSharp.Core.Runtime/Internals/CefRequestContextHandlerAdapter.cpp (1)
ToClr(52-52)CefSharp.Core.Runtime/Internals/Serialization/Primitives.cpp (2)
GetInt64(52-60)GetInt64(52-52)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (3)
CefSharp.BrowserSubprocess.Core/Wrapper/Browser.h (1)
Identifier(109-112)CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h (1)
Identifier(117-120)CefSharp.Core.Runtime/Request.h (1)
Identifier(69-69)
🧰 Additional context used
🧠 Learnings (1)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (1)
Learnt from: amaitland
PR: #4475
File: CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp:147-154
Timestamp: 2025-05-10T23:58:24.668Z
Learning: The GetCache method of the IJavaScriptObjectCache implementation should never return null in its current implementation. Both the legacy and per-browser cache implementations guarantee a non-null return value, with the per-browser implementation returning an empty dictionary when no cache exists for a given browser ID.
🧬 Code Graph Analysis (3)
CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp (3)
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h (1)
CefBrowserWrapper(27-75)CefSharp.Core.Runtime/Internals/StringUtils.h (1)
ToNative(74-84)CefSharp.Core.Runtime/Internals/Serialization/Primitives.cpp (2)
SetInt64(41-49)SetInt64(41-41)
CefSharp.Core.Runtime/Internals/ClientAdapter.cpp (2)
CefSharp.Core.Runtime/Internals/CefRequestContextHandlerAdapter.cpp (1)
ToClr(52-52)CefSharp.Core.Runtime/Internals/Serialization/Primitives.cpp (2)
GetInt64(52-60)GetInt64(52-52)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (3)
CefSharp.BrowserSubprocess.Core/Wrapper/Browser.h (1)
Identifier(109-112)CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h (1)
Identifier(117-120)CefSharp.Core.Runtime/Request.h (1)
Identifier(69-69)
🔇 Additional comments (20)
CefSharp.Core.Runtime/Internals/ClientAdapter.h (2)
73-73: Good change to cancel tasks before cleanup.The shift from direct deletion to explicit task cancellation aligns with the PR objective of properly canceling pending JavaScript tasks when the V8 context is released or the browser crashes.
82-82: Appropriate nullification after task cancellation.Setting the pointer to
nullptrafter canceling tasks is consistent with the new lifecycle management approach.CefSharp/Internals/TaskExtensions.cs (1)
23-23: Good simplification of cancellation logic.Relying directly on
TrySetCanceled()return value is cleaner and more robust than the previous status check, as the method already handles the completed task scenario internally.CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (2)
240-240: Improved error handling with faulted Task.Returning a faulted
Taskinstead ofnullptrprovides better error propagation and consistent API behavior when the browser host is unavailable.
248-248: Frame-scoped task management implementation.Adding the
Identifierparameter enables frame-scoped task management, which is essential for the PR objective of canceling pending tasks when V8 contexts are released.CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp (2)
30-30: Consistent error handling pattern.Returning a faulted
Taskinstead of throwing an exception maintains consistency with the error handling pattern established inCefFrameWrapper::EvaluateScriptAsync.
39-54: Frame-scoped task management and improved logic flow.Moving the callback task creation and message construction inside the frame validity check ensures tasks are only created for valid frames, and the addition of
_callback->FrameIdparameter enables proper frame-scoped task tracking.CefSharp.Core.Runtime/Internals/ClientAdapter.cpp (4)
704-704: Essential cleanup on render process termination.Canceling all pending tasks when the render process terminates ensures proper cleanup and prevents dangling tasks, which is a core objective of this PR.
387-389: Frame-scoped task cancellation on context release.Extracting the frame ID and canceling frame-specific pending tasks when the V8 context is released is exactly what the PR aims to achieve - proper cleanup when contexts become invalid.
393-393: Consistent use of extracted frame ID.Using the pre-extracted
frameIdvariable maintains consistency and avoids redundant calls toStringUtils::ToClr(frame->GetIdentifier()).
484-484: Frame-aware task removal operations.Adding the
frameIdparameter to bothRemovePendingTaskandRemoveJavascriptCallbackPendingTaskcalls enables proper frame-scoped task management, ensuring tasks are tracked and removed at the correct granularity.Also applies to: 492-493
CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs (3)
11-11: LGTM!The using statement is appropriately added to support the new test methods.
30-66: Well-structured test for V8 context lifecycle!The test comprehensively validates the frame-scoped task cancellation behavior across different V8 context states, which aligns perfectly with the PR objectives.
323-323: Good formatting consistency fix.CefSharp/Internals/PendingTaskRepository.cs (6)
22-23: Good refactoring to support frame-scoped task management.The new structure properly encapsulates tasks by frame ID, which aligns with the PR objective of canceling tasks when V8 context is released.
33-60: Correctly implements frame-scoped task creation.The method properly handles both creating new frame repositories and adding tasks to existing ones, with appropriate framework-specific implementations.
69-94: Consistent implementation for callback task creation.The method correctly maintains the separation between regular and callback tasks while implementing frame-scoped management.
107-120: Proper implementation of frame-scoped task removal.
147-158: Key implementation for frame-specific task cancellation.This method enables the core PR objective of canceling all pending tasks when a V8 context is released or frame is destroyed.
160-170: Comprehensive global task cancellation.Properly handles cancellation of all pending tasks across all frames, which is essential for browser disposal scenarios.
|
✅ Build CefSharp 138.0.170-CI5314 completed (commit 22ab072d60 by @campersau) |
|
❌ Build CefSharp 138.0.170-CI5315 failed (commit 9f11b8c2ef by @campersau) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (2)
27-71: Comprehensive test coverage for V8 context lifecycle management.This test effectively validates the key scenarios outlined in the PR objectives:
- Script evaluation failure before V8 context creation
- Successful callback execution after context availability
- Task cancellation on context changes and browser disposal
The test logic is sound and the resource management with
usingstatements is appropriate.Consider these improvements for test robustness:
Timing dependency: The 1000ms timeout in promises could cause flakiness in CI environments. Consider using a shorter timeout or mocking time-sensitive operations.
Exception validation: Line 36 only checks the message prefix. Consider validating the specific exception type for better test precision:
-var withoutV8ContextException = await Assert.ThrowsAsync<Exception>(() => browser.EvaluateScriptAsync("(function() { return 1+1; })")); +var withoutV8ContextException = await Assert.ThrowsAsync<InvalidOperationException>(() => browser.EvaluateScriptAsync("(function() { return 1+1; })"));
73-87: Excellent focused test for crash scenario validation.This test cleanly validates the critical crash scenario outlined in the PR objectives. The use of
chrome://crashis the standard approach for simulating browser crashes in Chromium-based testing, and the assertion logic correctly expectsTaskCanceledException.The test structure is well-organized and focused on a single concern.
Minor consideration for test reliability:
Similar to the previous test, the 1000ms timeout could potentially cause timing issues in slower CI environments, though it's likely sufficient for crash testing scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs(3 hunks)CefSharp.Test/Javascript/JavascriptCallbackTests.cs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs
🔇 Additional comments (1)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (1)
9-9: LGTM: Clean namespace addition for test support.The using statement is appropriately added to support the
CefExample.HelloWorldUrlusage in the new test methods.
|
❌ Build CefSharp 138.0.170-CI5316 failed (commit f7a447f0e8 by @campersau) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (1)
27-77: Comprehensive test coverage for V8 context lifecycle scenarios.This test method effectively validates the core functionality described in the PR objectives - canceling pending tasks when V8 context is released or browser is disposed. The test structure is well-organized and covers multiple scenarios:
- ✅ Script evaluation failure before V8 context creation
- ✅ Successful callback execution after context creation
- ✅ Task cancellation on V8 context change (reload)
- ✅ Task cancellation on browser disposal
The async/await patterns and exception assertions are correctly implemented.
However, consider these minor improvements:
- var callbackExecuteWithoutV8ContextException = await Assert.ThrowsAsync<Exception>(() => browser.EvaluateScriptAsync("(function() { return 1+1; })")); + var withoutContextException = await Assert.ThrowsAsync<Exception>(() => browser.EvaluateScriptAsync("(function() { return 1+1; })")); - Assert.StartsWith("Unable to execute javascript at this time", withoutV8ContextException.Message); + Assert.StartsWith("Unable to execute javascript at this time", withoutContextException.Message);The variable names could be more concise while maintaining clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp(1 hunks)CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs(3 hunks)CefSharp.Test/Javascript/JavascriptCallbackTests.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp
- CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs
🔇 Additional comments (3)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (3)
9-9: LGTM: Import addition is appropriate.The
CefSharp.Exampleimport is correctly added and used in the test methods to accessCefExample.HelloWorldUrl.
79-93: Excellent test for browser crash scenario.This test method directly validates one of the core PR objectives - ensuring pending tasks are canceled when the browser crashes. The implementation is clean and effective:
- Uses
chrome://crashto simulate browser crash condition- Properly tests that delayed promise callbacks are canceled
- Correctly expects
TaskCanceledExceptionThe test aligns perfectly with the PR's goal of handling task cancellation during browser crashes.
306-322: Good test coverage for repeated callback execution.This test method ensures that the refactored frame-scoped task management system maintains compatibility with existing callback behavior. The test validates that:
- Callbacks can be executed multiple times successfully
- Results remain consistent across executions
- No issues arise from the new task repository structure
The simple but effective test design provides confidence in the backwards compatibility of the changes.
|
❌ Build CefSharp 138.0.170-CI5317 failed (commit 284fd87c1c by @campersau) |
|
Thanks for the PR 👍 Any ideas what's causing the tests to fail when run on appveyor? |
|
I currently don't know, my run did work: https://ci.appveyor.com/project/campersau/cefsharp/builds/52449336 |
|
❌ Build CefSharp 138.0.330-CI5324 failed (commit 1606c8fea4 by @campersau) |
|
It looks like the same issue happens in main from time to time as well. E.g.: 633a8cd |
|
The GPU crash happens semi regularly, hopefully switching to The
I don't remember seeing this before, looking at a few other failed ones and it has happened prior, so maybe unrelated. |
|
I'd asked about the GCM errors at https://www.magpcss.org/ceforum/viewtopic.php?f=6&t=20318 Might have to follow up to see if there's anything further related to those. |
amaitland
force-pushed
the
cancelpendingtasksoncrashmaster
branch
from
a942fc5 to
774abb1
Compare
|
I should be able to look at this over the weekend. |
|
✅ Build CefSharp 138.0.330-CI5327 completed (commit 6313f2f997 by @campersau) |
| @@ -70,8 +70,7 @@ namespace CefSharp | |||
|
|
|||
| CloseAllPopups(true); | |||
|
|
|||
| //this will dispose the repository and cancel all pending tasks | |||
| delete _pendingTaskRepository; | |||
There was a problem hiding this comment.
delete will call Dispose if it's implemented. Guess it's semantics, might be worth having pending task repository implement IDisposable.
|
❌ Build CefSharp 138.0.330-CI5333 failed (commit a28abc36f2 by @campersau) |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs(3 hunks)CefSharp.Test/Javascript/JavascriptCallbackTests.cs(3 hunks)CefSharp/Internals/FramePendingTaskRepository.cs(1 hunks)CefSharp/Internals/PendingTaskRepository.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CefSharp.Test/Javascript/JavascriptCallbackTests.cs
- CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs
🔇 Additional comments (7)
CefSharp/Internals/FramePendingTaskRepository.cs (2)
11-16: Well-designed frame-scoped task repository.The class design follows good practices with appropriate access modifiers, thread-safe collections, and immutable property references. The generic type parameter enables reuse across different result types.
18-31: Correct disposal pattern with safe task cancellation.The Dispose method properly uses
TrySetCanceled()to avoid exceptions if tasks are already completed, and clears the dictionaries after cancellation. This implementation aligns with best practices for disposing TaskCompletionSource collections.CefSharp/Internals/PendingTaskRepository.cs (5)
22-23: Effective refactoring to frame-scoped task management.The change from separate task dictionaries to a single dictionary of frame repositories enables targeted task cancellation per frame while maintaining thread safety and proper encapsulation.
107-120: Correct implementation of frame-scoped task removal.The method properly handles the two-level lookup (frame then task) with appropriate null checking and maintains thread safety throughout the operation.
132-145: Consistent implementation with proper frame-scoped lookup.The method correctly mirrors the RemovePendingTask implementation pattern, operating on the CallbackPendingTasks dictionary with proper error handling.
147-158: Excellent implementation of frame-scoped task cancellation.This new method enables the core functionality of canceling tasks per frame. The use of
TryRemoveensures atomic removal and the subsequentDispose()call properly cancels all frame-scoped tasks.
163-170: Proper implementation of global task cancellation.The method correctly disposes all frame repositories and clears the main dictionary, providing a clean way to cancel all pending tasks across all frames.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CefSharp/Internals/PendingTaskRepository.cs(2 hunks)
🔇 Additional comments (7)
CefSharp/Internals/PendingTaskRepository.cs (7)
22-23: LGTM! Clean refactoring to frame-scoped architecture.The change from separate dictionaries to a single frame-keyed dictionary aligns well with the PR objective of managing tasks per frame/V8Context for proper cancellation.
65-95: Consistent implementation with CreatePendingTask.The JavaScript callback variant maintains the same pattern and structure as the regular pending task creation, which is good for consistency.
107-120: Proper frame-scoped task removal with safe fallback.The implementation correctly handles the case where the frame doesn't exist and provides safe fallback by returning null. The nested TryGetValue/TryRemove pattern is appropriate for concurrent access.
132-145: Consistent with RemovePendingTask implementation.The callback task removal follows the same safe pattern as the regular task removal, maintaining consistency across the API.
147-158: Frame-specific cancellation implements core PR requirement.This method directly addresses the PR objective of canceling pending tasks when V8Context is released. The TryRemove followed by Dispose ensures proper cleanup.
160-170: FramePendingTaskRepository.Dispose safely uses TrySetCanceled
I’ve verified inCefSharp/Internals/FramePendingTaskRepository.csthat theDispose()implementation iterates over bothPendingTasks.ValuesandCallbackPendingTasks.Values, callingtcs.TrySetCanceled()each time before clearing the dictionaries. There’s no use ofSetCanceled(), so cancellations won’t throw. All good to go.
40-52: Conditional compilation and generic type usage verifiedAll generic class definitions and instantiations are consistent:
- FramePendingTaskRepository defined at
CefSharp/Internals/FramePendingTaskRepository.cs:11- Instantiated with
<TResult>inCefSharp/Internals/PendingTaskRepository.csat lines 42, 49, 77, and 84No non-generic usages detected. Approving these changes.
| /// <param name="frameId">The frame id in which the task is created.</param> | ||
| /// <param name="timeout">The maximum running time of the task.</param> | ||
| /// <returns>The unique id of the newly created pending task and the newly created <see cref="TaskCompletionSource{TResult}"/>.</returns> | ||
| public KeyValuePair<long, TaskCompletionSource<TResult>> CreatePendingTask(TimeSpan? timeout = null) | ||
| public KeyValuePair<long, TaskCompletionSource<TResult>> CreatePendingTask(string frameId, TimeSpan? timeout = null) | ||
| { | ||
| var taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
|
||
| var id = Interlocked.Increment(ref lastId); | ||
| pendingTasks.TryAdd(id, taskCompletionSource); | ||
| var result = new KeyValuePair<long, TaskCompletionSource<TResult>>(id, taskCompletionSource); | ||
| #if NETCOREAPP | ||
| framePendingTasks.AddOrUpdate( | ||
| frameId, | ||
| (key, state) => { var value = new FramePendingTaskRepository<TResult>(); value.PendingTasks.TryAdd(state.Key, state.Value); return value; }, | ||
| (key, value, state) => { value.PendingTasks.TryAdd(state.Key, state.Value); return value; }, | ||
| result | ||
| ); | ||
| #else | ||
| framePendingTasks.AddOrUpdate( | ||
| frameId, | ||
| (key) => { var value = new FramePendingTaskRepository<TResult>(); value.PendingTasks.TryAdd(id, taskCompletionSource); return value; }, | ||
| (key, value) => { value.PendingTasks.TryAdd(id, taskCompletionSource); return value; } | ||
| ); | ||
| #endif | ||
|
|
||
| if (timeout.HasValue) | ||
| { | ||
| taskCompletionSource = taskCompletionSource.WithTimeout(timeout.Value, () => RemovePendingTask(id)); | ||
| taskCompletionSource.WithTimeout(timeout.Value, () => RemovePendingTask(frameId, id)); | ||
| } | ||
|
|
||
| return new KeyValuePair<long, TaskCompletionSource<TResult>>(id, taskCompletionSource); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify frame ID validation and consider null safety.
The method correctly implements frame-scoped task creation, but consider adding validation for the frameId parameter to prevent issues with null or empty frame IDs.
Consider adding validation at the beginning of the method:
public KeyValuePair<long, TaskCompletionSource<TResult>> CreatePendingTask(string frameId, TimeSpan? timeout = null)
{
+ if (string.IsNullOrEmpty(frameId))
+ throw new ArgumentException("Frame ID cannot be null or empty", nameof(frameId));
+
var taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously);🏁 Script executed:
#!/bin/bash
# Search for frameId validation patterns in the codebase
rg -A 3 -B 3 "string.*frameId" --type cs | head -20
# Check if there are any null checks for frameId parameters elsewhere
ast-grep --pattern 'if ($frameId == null || string.IsNullOrEmpty($frameId)) { $$$ }'Length of output: 2364
Add null or empty check for frameId at the start of Create methods
To avoid downstream ArgumentNullException or unintended behavior when frameId is null or empty, validate the parameter in both creation methods:
• CefSharp/Internals/PendingTaskRepository.cs:
– CreatePendingTask
– CreateJavascriptCallbackPendingTask
Proposed diff:
public KeyValuePair<long, TaskCompletionSource<TResult>> CreatePendingTask(string frameId, TimeSpan? timeout = null)
{
+ if (string.IsNullOrEmpty(frameId))
+ throw new ArgumentException("Frame ID cannot be null or empty", nameof(frameId));
var taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously);
var id = Interlocked.Increment(ref lastId);
…
}
public KeyValuePair<long, TaskCompletionSource<TResult>> CreateJavascriptCallbackPendingTask(string frameId, long id, TimeSpan? timeout = null)
{
+ if (string.IsNullOrEmpty(frameId))
+ throw new ArgumentException("Frame ID cannot be null or empty", nameof(frameId));
var taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously);
…
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <param name="frameId">The frame id in which the task is created.</param> | |
| /// <param name="timeout">The maximum running time of the task.</param> | |
| /// <returns>The unique id of the newly created pending task and the newly created <see cref="TaskCompletionSource{TResult}"/>.</returns> | |
| public KeyValuePair<long, TaskCompletionSource<TResult>> CreatePendingTask(TimeSpan? timeout = null) | |
| public KeyValuePair<long, TaskCompletionSource<TResult>> CreatePendingTask(string frameId, TimeSpan? timeout = null) | |
| { | |
| var taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously); | |
| var id = Interlocked.Increment(ref lastId); | |
| pendingTasks.TryAdd(id, taskCompletionSource); | |
| var result = new KeyValuePair<long, TaskCompletionSource<TResult>>(id, taskCompletionSource); | |
| #if NETCOREAPP | |
| framePendingTasks.AddOrUpdate( | |
| frameId, | |
| (key, state) => { var value = new FramePendingTaskRepository<TResult>(); value.PendingTasks.TryAdd(state.Key, state.Value); return value; }, | |
| (key, value, state) => { value.PendingTasks.TryAdd(state.Key, state.Value); return value; }, | |
| result | |
| ); | |
| #else | |
| framePendingTasks.AddOrUpdate( | |
| frameId, | |
| (key) => { var value = new FramePendingTaskRepository<TResult>(); value.PendingTasks.TryAdd(id, taskCompletionSource); return value; }, | |
| (key, value) => { value.PendingTasks.TryAdd(id, taskCompletionSource); return value; } | |
| ); | |
| #endif | |
| if (timeout.HasValue) | |
| { | |
| taskCompletionSource = taskCompletionSource.WithTimeout(timeout.Value, () => RemovePendingTask(id)); | |
| taskCompletionSource.WithTimeout(timeout.Value, () => RemovePendingTask(frameId, id)); | |
| } | |
| return new KeyValuePair<long, TaskCompletionSource<TResult>>(id, taskCompletionSource); | |
| return result; | |
| } | |
| public KeyValuePair<long, TaskCompletionSource<TResult>> CreatePendingTask(string frameId, TimeSpan? timeout = null) | |
| { | |
| if (string.IsNullOrEmpty(frameId)) | |
| throw new ArgumentException("Frame ID cannot be null or empty", nameof(frameId)); | |
| var taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously); | |
| var id = Interlocked.Increment(ref lastId); | |
| var result = new KeyValuePair<long, TaskCompletionSource<TResult>>(id, taskCompletionSource); | |
| #if NETCOREAPP | |
| framePendingTasks.AddOrUpdate( | |
| frameId, | |
| (key, state) => { var value = new FramePendingTaskRepository<TResult>(); value.PendingTasks.TryAdd(state.Key, state.Value); return value; }, | |
| (key, value, state) => { value.PendingTasks.TryAdd(state.Key, state.Value); return value; }, | |
| result | |
| ); | |
| #else | |
| framePendingTasks.AddOrUpdate( | |
| frameId, | |
| (key) => { var value = new FramePendingTaskRepository<TResult>(); value.PendingTasks.TryAdd(id, taskCompletionSource); return value; }, | |
| (key, value) => { value.PendingTasks.TryAdd(id, taskCompletionSource); return value; } | |
| ); | |
| #endif | |
| if (timeout.HasValue) | |
| { | |
| taskCompletionSource.WithTimeout(timeout.Value, () => RemovePendingTask(frameId, id)); | |
| } | |
| return result; | |
| } | |
| public KeyValuePair<long, TaskCompletionSource<TResult>> CreateJavascriptCallbackPendingTask(string frameId, long id, TimeSpan? timeout = null) | |
| { | |
| if (string.IsNullOrEmpty(frameId)) | |
| throw new ArgumentException("Frame ID cannot be null or empty", nameof(frameId)); | |
| var taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously); | |
| … | |
| } |
🤖 Prompt for AI Agents
In CefSharp/Internals/PendingTaskRepository.cs around lines 30 to 60, add a
check at the start of the CreatePendingTask method to validate that the frameId
parameter is not null or empty. If it is, throw an ArgumentNullException or
ArgumentException to prevent downstream errors. Apply the same validation in the
CreateJavascriptCallbackPendingTask method to ensure consistent parameter
validation in both creation methods.
|
❌ Build CefSharp 138.0.330-CI5334 failed (commit 7f17738739 by @campersau) |
|
❌ Build CefSharp 138.0.330-CI5335 failed (commit 7f17738739 by @campersau) |
|
❌ Build CefSharp 138.0.330-CI5336 failed (commit 7f17738739 by @campersau) |
amaitland
force-pushed
the
cancelpendingtasksoncrashmaster
branch
from
7265b22 to
1bf7e15
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (1)
237-242: Return a faulted Task instead of null — good improvement; update the stale commentReturning Task::FromException with a descriptive InvalidOperationException is preferable to nullptr and aligns with earlier review feedback about avoiding NREs.
Please update the comment to reflect the new behavior:
- //If we're unable to get the underlying browser/browserhost then return null + // If we're unable to get the underlying browser/browserhost then return a faulted Task
🧹 Nitpick comments (5)
CefSharp/Internals/TaskExtensions.cs (1)
20-31: Race-safe cancellation via TrySetCanceled — LGTM; consider disposing the timer on early completionSwitching to TrySetCanceled() and invoking the cancelled callback only when cancellation succeeds removes redundant checks and avoids invoking the callback after completion. Looks good.
Optionally, dispose the timer if the task completes before the timeout to free resources sooner (current code disposes on timeout only):
var timer = new Timer(state => { ((Timer)state).Dispose(); if (taskCompletionSource.TrySetCanceled()) { if (cancelled != null) { cancelled(); } } }); timer.Change(timeout, Timeout.InfiniteTimeSpan); + +// Ensure timer is released if the task completes before the timeout +taskCompletionSource.Task.ContinueWith(_ => timer.Dispose(), TaskScheduler.Default); return taskCompletionSource;CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (1)
247-265: Typo: rename idAndComplectionSource to idAndCompletionSource (local rename only)Purely a readability/typo fix within the method:
- auto idAndComplectionSource = pendingTaskRepository->CreatePendingTask(Identifier, timeout); + auto idAndCompletionSource = pendingTaskRepository->CreatePendingTask(Identifier, timeout); - if (useImmediatelyInvokedFuncExpression) + if (useImmediatelyInvokedFuncExpression) { - script = "(function() { let cefSharpInternalCallbackId = " + idAndComplectionSource.Key + "; " + script + " })();"; + script = "(function() { let cefSharpInternalCallbackId = " + idAndCompletionSource.Key + "; " + script + " })();"; } auto message = CefProcessMessage::Create(kEvaluateJavascriptRequest); auto argList = message->GetArgumentList(); - SetInt64(argList, 0, idAndComplectionSource.Key); + SetInt64(argList, 0, idAndCompletionSource.Key); argList->SetString(1, StringUtils::ToNative(script)); argList->SetString(2, StringUtils::ToNative(scriptUrl)); argList->SetInt(3, startLine); _frame->SendProcessMessage(CefProcessId::PID_RENDERER, message); - return idAndComplectionSource.Value->Task; + return idAndCompletionSource.Value->Task;CefSharp.Core.Runtime/Internals/ClientAdapter.cpp (2)
702-706: Cancel pending tasks on render-process termination — good; add a defensive null-checkThe eager cancellation here is appropriate and should reduce dangling awaiters when the renderer dies. For robustness (during late shutdown/disposal windows), guard against a null repository:
- _pendingTaskRepository->CancelPendingTasks(); + if (_pendingTaskRepository != nullptr) + { + _pendingTaskRepository->CancelPendingTasks(); + }
1484-1494: Guard against a potentially invalid frame before accessing GetIdentifier()While CEF typically provides a valid frame for these messages, races during teardown can leave frame invalid. Adding a sanity check avoids dereferencing an invalid frame in those rare cases; cancelled tasks would already have been handled by earlier context-release/crash logic.
- auto frameId = StringUtils::ToClr(frame->GetIdentifier()); + if (!(frame.get() && frame->IsValid())) + { + handled = true; // likely already cancelled + return handled; + } + auto frameId = StringUtils::ToClr(frame->GetIdentifier());CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs (1)
30-64: New test validates cancellation on V8 context change — LGTM; reduce flakiness with a unique reload URLThis is a solid coverage addition. To minimize race conditions where the promise resolves before the reload causes a context change, consider navigating to a unique URL (query param) to guarantee a fresh V8 context:
- // V8 context - await browser.LoadUrlAsync(CefExample.HelloWorldUrl); + // V8 context + await browser.LoadUrlAsync(CefExample.HelloWorldUrl); var evaluateWithoutV8ContextResponse = await evaluateWithoutV8ContextTask; Assert.True(evaluateWithoutV8ContextResponse.Success); Assert.Equal(3, evaluateWithoutV8ContextResponse.Result); var evaluateCancelAfterV8ContextChangeTask = browser.EvaluateScriptAsync("new Promise(resolve => setTimeout(resolve, 1000))"); - // change V8 context - await browser.LoadUrlAsync(CefExample.HelloWorldUrl); + // change V8 context (unique URL to force reload) + var reloadUrl = CefExample.HelloWorldUrl + "?r=" + Guid.NewGuid().ToString("N"); + await browser.LoadUrlAsync(reloadUrl);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp(1 hunks)CefSharp.Core.Runtime/Internals/ClientAdapter.cpp(3 hunks)CefSharp.Core.Runtime/Internals/ClientAdapter.h(2 hunks)CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp(1 hunks)CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs(3 hunks)CefSharp.Test/Javascript/JavascriptCallbackTests.cs(3 hunks)CefSharp/Internals/FramePendingTaskRepository.cs(1 hunks)CefSharp/Internals/PendingTaskRepository.cs(2 hunks)CefSharp/Internals/TaskExtensions.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- CefSharp/Internals/FramePendingTaskRepository.cs
- CefSharp.Core.Runtime/Internals/ClientAdapter.h
- CefSharp.Test/Javascript/JavascriptCallbackTests.cs
- CefSharp.Core.Runtime/Internals/JavascriptCallbackProxy.cpp
- CefSharp/Internals/PendingTaskRepository.cs
🧰 Additional context used
🧬 Code Graph Analysis (3)
CefSharp.Core.Runtime/Internals/ClientAdapter.cpp (2)
CefSharp.Core.Runtime/Internals/CefRequestContextHandlerAdapter.cpp (1)
ToClr(52-52)CefSharp.Core.Runtime/Internals/Serialization/Primitives.cpp (2)
GetInt64(52-60)GetInt64(52-52)
CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs (3)
CefSharp.Test/Javascript/JavascriptCallbackTests.cs (5)
Fact(27-77)Fact(79-93)Fact(259-279)Fact(281-304)Fact(306-322)CefSharp.Example/CefExample.cs (1)
CefExample(15-274)CefSharp.Test/BrowserTests.cs (1)
AssertInitialLoadComplete(17-23)
CefSharp.Core.Runtime/Internals/CefFrameWrapper.cpp (3)
CefSharp.BrowserSubprocess.Core/Wrapper/Browser.h (1)
Identifier(109-112)CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h (1)
Identifier(117-120)CefSharp.Core.Runtime/Request.h (1)
Identifier(69-69)
🔇 Additional comments (4)
CefSharp.Core.Runtime/Internals/ClientAdapter.cpp (1)
1387-1394: Per-frame cancellation on V8 context release — LGTMExtracting frameId once and cancelling only that frame’s pending tasks matches the PR objective and keeps unrelated frames unaffected.
CefSharp.Test/Javascript/EvaluateScriptAsyncTests.cs (3)
11-11: Import for CefExample URLs — LGTMThe using CefSharp.Example; import is required for HelloWorldUrl. Good.
66-75: Crash-triggered cancellation coverage — LGTMAccurately asserts cancellation behavior on chrome://crash (aligned with new CancelPendingTasks on termination).
314-315: Minor formatting fix — LGTMStyle consistency improvement is fine.
|
❌ Build CefSharp 139.0.230-CI5339 failed (commit fa1296ee49 by @campersau) |
|
❌ Build CefSharp 139.0.230-CI5340 failed (commit fa1296ee49 by @campersau) |
|
Looks like |
Fixes: #5144
Summary:
Changes:
How Has This Been Tested?
Unittest
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Refactor