IRequest.Headers uses copy semantics instead of reference semantics

[ike86]

• What version of the product are you using?

  • CefSharp.Common 71.0.0 from NuGet

• What architecture x86 or x64?

  • x64

• On what operating system?

  • Win 8, Win10

• I am using WPF

• What steps will reproduce the problem?

  • Inherit from DefaultRequestHandler
  • Override OnBeforeResourceLoad

public override CefReturnValue OnBeforeResourceLoad(
    IWebBrowser browserControl,
    IBrowser browser,
    IFrame frame,
    IRequest request,
    IRequestCallback callback)
{
    var headers = request.Headers;
    headers["Authorization"] = $"Bearer {this.bearerToken}";
    // request.Headers = headers;

    return CefReturnValue.Continue;
}

• What is the expected output? What do you see instead?

  • Expected: "Authorization" header is set in request.Headers
  • Actual: request.Headers is unchanged

• Please provide any additional information below.

  • If you uncomment request.Headers = headers;, the Expected outcome will happen.
  • The Headers getter and setter has copy semantics. This is unexpected in C#, as classes use reference semantics, so one would expect, that the code above will just work.
  • See https://github.com/cefsharp/CefSharp/blob/master/CefSharp.Core/Internals/CefRequestWrapper.cpp#L103

• Does this problem also occur in the CEF Sample Application from http://opensource.spotify.com/cefbuilds/index.html?

  • n/a

[amaitland]

Header Collection NOTE: This collection is a copy of the underlying type, to make changes, take a reference to the collection, make your changes, then reassign the collection. At some point this will be replaced with a proper wrapper.

http://cefsharp.github.io/api/71.0.0/html/P_CefSharp_IRequest_Headers.htm

For anyone else reading this, I'll point out that this is the current expected behaviour.

• The Headers getter and setter has copy semantics. This is unexpected in C#, as classes use reference semantics, so one would expect, that the code above will just work.

It's this expectation that makes is so hard to map/wrap. Exposing the Get/Set methods might give a clue to developers with a C++ background, it's not clear that it will improve the user experience dramatically enough to warrant the breaking change (and the extra support that comes with it, all the old documentation/examples becoming invalidated). The number of support requests I get for this as it stands are very low.

If someone wants to submit a PR that adds the Get/Set methods as well then by all means 👍

It maybe possible to improve the user experience by throwing an exception if you modify the collection and don't call the additionally required Set. This would require changing ClientAdapter::OnBeforeResourceLoad to pass CefRequestWrapper so it's disposed when it goes out of scope, this would be a breaking change and may not make sense as it would make the callback harder to use.

If anyone has other ideas, including how to better document the current behaviour then I'm open to suggestions.

[ike86]

Ok, first of all, I haven't read the documentation of the property 🤦‍♂️

Introducing any breaking change is not worth the effort IMO.

Another solution could be to write a code analyzer rule for these cases, which provides a warning, if the user doesn't set the property after getting. I'm not sure, how much effort would that take. It might be the easiest to assume, that the user just reads the documentation.

[amaitland]

It isn't the best documentation I've ever written, at a minimum let's add a code example.

For my own reference https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/xmldoc/example

[amaitland]

I've added a xml doc code example in f77c6d2

There's also now SetHeaderByName and GetHeaderByName methods that can be used in cases where the header has a single value, these greatly simplify say modifying the User-Agent.