[dotnet] [bidi] Do not throw when CallFunction or Evaluate return exceptional result (breaking change)

[RenderMichael]

Description

In an effort for Selenium's raw BiDi to return the exact response from the endpoint, we should not throw exceptions unless the opt-in ThrowOnError() method is called.

Motivation and Context

Fixes #15414

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement, Tests

---

Description

• Updated EvaluateAsync and CallFunctionAsync methods to handle exceptional results without throwing exceptions directly.

  • Introduced ThrowOnError method in EvaluateResult to centralize error handling.
  • Modified return types of EvaluateAsync and CallFunctionAsync to EvaluateResult for consistency.

• Enhanced error handling by refining ScriptEvaluateException to include detailed stack trace information.

• Adjusted test cases to validate the new ThrowOnError behavior and ensure proper handling of exceptional results.

  • Updated assertions in test cases to check for EvaluateResultSuccess and use ThrowOnError for result validation.

• Improved serialization logic in ChannelLocalValue to ensure proper JSON property naming.

---

Changes walkthrough 📝

	Relevant files
Enhancement	5 files BrowsingContextScriptModule.cs Refactored EvaluateAsync and CallFunctionAsync methods for error handling +4/-4      EvaluateCommand.cs Added ThrowOnError method to EvaluateResult for centralized error handling +12/-1    LocalValue.cs Improved JSON serialization for `ChannelLocalValue`            +3/-2      ScriptEvaluateException.cs Enhanced exception message with detailed stack trace          +1/-1      ScriptModule.cs Removed direct exception throwing in EvaluateAsync and CallFunctionAsync +7/-16
Tests	5 files CallFunctionLocalValueTest.cs Updated tests to validate ThrowOnError behavior for CallFunctionAsync +88/-100 CallFunctionParameterTest.cs Adjusted parameter tests to handle ThrowOnError in CallFunctionAsync +23/-22  CallFunctionRemoteValueTest.cs Refined remote value tests to use ThrowOnError for result validation +38/-38  EvaluateParametersTest.cs Enhanced evaluation parameter tests with `ThrowOnError` validation +12/-11  ScriptCommandsTest.cs Updated script command tests to validate `ThrowOnError` behavior +1/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Type Safety The ThrowOnError method assumes that if a result is not EvaluateResultSuccess, it must be EvaluateResultException. This could lead to runtime errors if new result types are added in the future. Exception Message Format The Message property now joins stack trace frames with newlines, but there's no validation that CallFrames is not null or empty, which could lead to unexpected formatting. public override string Message => $"{Text}{Environment.NewLine}{string.Join(Environment.NewLine, _evaluateResultException.ExceptionDetails.StackTrace.CallFrames)}";

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check The code assumes that _evaluateResultException.ExceptionDetails.StackTrace is always non-null, but it could be null in some error scenarios. Add a null check to prevent a potential NullReferenceException when formatting the error message. dotnet/src/webdriver/BiDi/Modules/Script/ScriptEvaluateException.cs [32] -public override string Message => $"{Text}{Environment.NewLine}{string.Join(Environment.NewLine, _evaluateResultException.ExceptionDetails.StackTrace.CallFrames)}"; +public override string Message => $"{Text}{Environment.NewLine}{(_evaluateResultException.ExceptionDetails.StackTrace != null ? string.Join(Environment.NewLine, _evaluateResultException.ExceptionDetails.StackTrace.CallFrames) : "No stack trace available")}"; Apply this suggestion Suggestion importance[1-10]: 7 __ Why: This is a valid defensive programming suggestion that could prevent potential NullReferenceExceptions if StackTrace is null. Adding null checking is a good practice for error handling code, especially in exception formatting.	Medium
General	Fix property visibility issue The Type property is marked as internal which might prevent proper JSON serialization in some contexts. Since this is a property that needs to be included in serialization (as indicated by the comment and JsonInclude attribute), it should be made public to ensure consistent serialization behavior. dotnet/src/webdriver/BiDi/Modules/Script/LocalValue.cs [95-98] public record ChannelLocalValue(ChannelProperties Value) : LocalValue { // AddPreloadScript takes arguments typed as ChannelLocalValue but still requires "type":"channel" [JsonInclude] [JsonPropertyName("type")] - internal string Type => "channel"; + public string Type => "channel"; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that changing the visibility of the Type property from internal to public could improve serialization consistency. This is a reasonable suggestion for better API design, though the current implementation likely works in most scenarios.	Low
Learned best practice	✅ Use safe type checking with the 'as' operator instead of direct casting to prevent potential InvalidCastException Suggestion Impact: The commit addressed the same issue as the suggestion but implemented a different solution. Instead of using 'as' operator with null checking, the method was renamed to 'AsSuccess' and still uses direct casting but with a different approach to handle the exception case. code diff: - public EvaluateResultSuccess ThrowOnError() + public EvaluateResultSuccess AsSuccess() { if (this is EvaluateResultSuccess success) { return success; } - throw new ScriptEvaluateException((EvaluateResultException)this); + var exceptionResult = (EvaluateResultException)this; + + throw new ScriptEvaluateException(exceptionResult.ExceptionDetails); The current implementation of ThrowOnError() uses a direct cast to EvaluateResultException without checking if the object is actually of that type. If this is neither EvaluateResultSuccess nor EvaluateResultException, it will throw an InvalidCastException. Use the as operator with null checking to make the code more robust and prevent potential runtime exceptions. dotnet/src/webdriver/BiDi/Modules/Script/EvaluateCommand.cs [44-52] public EvaluateResultSuccess ThrowOnError() { if (this is EvaluateResultSuccess success) { return success; } - throw new ScriptEvaluateException((EvaluateResultException)this); + var exception = this as EvaluateResultException; + if (exception != null) + { + throw new ScriptEvaluateException(exception); + } + + throw new InvalidOperationException($"Unexpected result type: {this.GetType().Name}"); } [Suggestion has been applied] Suggestion importance[1-10]: 6	Low
Update

[RenderMichael]

For error like this:

Before:

After:

[nvborisenko]

We can even remove ScriptEvaluateException type. And throw general BiDiException which takes already formatted string from EvaluateResultError record type. Then what we need, just improve formatting of EvaluateResultError to string (https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record#built-in-formatting-for-display).

[RenderMichael]

I think a special exception type is necessary, because we can expose the ExceptionDetails directly. User code can be like:

try
{
    var res = await context.Script.EvaluateAsync("maybe throws", false);
}
catch (ScriptEvaluateException ex)
{
    Console.WriteLine(ex.EvaluateResultException);
}

I changed the ScriptEvaluateException to reflect this, let me know what you think.

[RenderMichael]

Good name?

[nvborisenko]

EnsureSuccess() or ToSuccess() or AsSuccess() is better IMHO. As I know EnsureSuccess() should not convert, I have no strong opinion.

[nvborisenko]

If we consider #15521 (comment), then I propose RemoteValue AsSuccessResult().

[nvborisenko]

How I feel it: .To*() should allocate new object, As*() should return a reference.

[RenderMichael]

I agree with the As*() naming style. Going with AsSuccessResult().

I mentioned in the other comment why I think this should still return an EvaluateResultSuccess.

However, I do not feel strongly about it, since this is just a helper method anyway (should this be an extension?). In our code, we call EvaluateResultSuccess.Realm two times, and the remaining 68 times we access .Result immediately.

While typing this message, I have been convinced. Changing the return to RemoteValue.

[nvborisenko]

What if just return RemoteValue at once?

[RenderMichael]

That is too much magic in my opinion. the two methods which have JS evaluation, EvaluateAsync and CallFunctionAsync, both have generic overloads which will de-serialize to the correct type. Otherwise, users have access to the Success type directly. It is trivial to do .Result if they need the RemoteValue.

[RenderMichael]

Done.

[nvborisenko]

Not related to this PR, I added a prefix (breaking change) to reflect this in CHANGELOG. Thus users will be notified about potential breaking changes.

[nvborisenko]

I really propose to remove this exception type.

• If spec introduces new PartiallyException type, we again will be in problematic situation.

How to negotiate: just throw BiDiException with info about something like "expected success but got 'blah blah'".

[nvborisenko]

All right, and this "blah blah" can be just commandResult.ToString() (and here the magic of formatting of record to string helps).

[nvborisenko]

Unnecessary attribute, in my head we will just rename property rather than support backward compatibility. Because if we will not rename properties according spec (even if it is breaking change), we will become to a chaos where the .NET types mess weakly correlated to spec.

[nvborisenko]

Do you know any difference:

internal string Type { get: } = "channel";

Any performance impact?

(minor question, resolve it without answers)

[nvborisenko]

https://sharplab.io/#v2:CYLg1APgAgTAjAWAFBQMwAJboLIE8AquADgKboDey61mGUcADOgILoC8AfOgEQCGARgGNuAbio1x1NJkboAQhXQBzEgBcR6AL7seA4WKQ10k2jIBsMpgGF2XPkNEmT0+hfpMAIopXqt1NroOBppAA===

Static is better? If it is true... and it is friendly with json serialization, I propose to introduce this rule.