[Bug]: CookiesAsync gives sometimes `Value cannot be null. (Parameter 'source')` - binary breaking changes from 1.51 -> 1.52

[304NotModified]

Version

1.52

Steps to reproduce

1. Call var cookies = await _page.Context.CookiesAsync();

Update: it's a binary breaking change, see #3161 (comment):
So compiled a lib to version <=1.51 and compile your project to >=1.52

Expected behavior

• Don't crash if there are no cookies
• Do throw a ArgumentNullException, we don't send an argument

Actual behavior

a System.ArgumentNullException is thrown

System.ArgumentNullException
  HResult=0x80004003
  Message=Value cannot be null. (Parameter 'source')
  Source=System.Linq
  StackTrace:
   at System.Linq.ThrowHelper.ThrowArgumentNullException(ExceptionArgument argument)
   at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
   at Microsoft.Playwright.Core.BrowserContext.<CookiesAsync>d__106.MoveNext() in /_/src/Playwright/Core/BrowserContext.cs:line 385
   at MyProject.End2EndTests.EndToEndSteps.<LogNavigation>d__19.MoveNext() in /_/src/End2EndTests/EndToEndSteps.cs:line 107

  This exception was originally thrown at this call stack:
    [External Code]
    Microsoft.Playwright.Core.BrowserContext.CookiesAsync(System.Collections.Generic.IEnumerable<string>) in BrowserContext.cs
    MyProject.End2EndTests.EndToEndSteps.LogNavigation(string) in EndToEndSteps.cs

FYI there is one page in the context, and it's about:blank

This is for a generic logging of the used url and used cookies. Unfortunately we cannot check if Context.Page.Count > 0, as there is a page.

Additional context

It seems this bug is introduced in 1.52

Environment

- Operating System: Windows 11
- CPU: Intel Core i7
- Browser: Chromium
- .NET Version (TFM): net8.0
- Other info: on multiple machines local/CI

[304NotModified]

Maybe it's a (binary) breaking change, as I see this:

We call CookiesAsync without arguments, but we end up with CookiesAsync(IEnumerable urls), where url is null

[304NotModified]

Found it, it's indeed a binary breaking change.
From https://github.com/microsoft/playwright-dotnet/pull/3145/files#diff-2cf3606203604824a9e5ffbc050253e353c746ad7a718e8117c24b149a07508b, from #3145

Before: CookiesAsync(IEnumerable<string> urls = null)

After: CookiesAsync(), CookiesAsync(string url) and CookiesAsync(IEnumerable<string> urls)

Of course this is hard to see in the review, could something like the Baseline Package Validator implemented here?

These breaking changes could break a lot and take some analyses time

Also, all users could get issues, even when they migrate from <=1.51 to > 1.51, so we should fix this? (e.g. from 1.40 to 1.60)

[mxschmitt]

I landed a patch with a fix for this specific case: #3162 - I think that should fix it for you?

[304NotModified]

Looks good. Maybe you need also an attribute to mark it as possible nullable (to prevent warnings and regressions)

[mxschmitt]

I filed #3161 for it - I think that should uncover this issue as well in the future if we enable the Nullable checks?

[304NotModified]

I think you mean #3163, that's a good idea, but it would party prevent it.

Would a PR with baseline validation accepted?

[mxschmitt]

Yes sounds great!

[304NotModified]

Yes sounds great!

See #3169

[304NotModified]

fixed by #3162