Change get() to return null instead of undefined

[inexorabletash]

When get() was implemented in Chrome, we failed to match our own spec and made get() return null instead of undefined if no matching cookie was found. The WPT tests match the implementation, not the spec. Oops.

Someone noticed and filed https://bugs.chromium.org/p/chromium/issues/detail?id=1355228

@ayuishii investigated HTTP Archive and found several sites/libraries that seem to do strict comparisons against null.

Some examples from the the first 1000 mobile results:

• These ones look safe with either null or undefined:
  • controltag.js (a common library - salesforce?) contains return t=this.cookieStore.get(n),(null!=t?t.length:void 0)||(... - this is a sloppy (not strict) comparison, and since undefined == null this code would not break (491/1000)
  • "modern.js" contains var t=this.cookieStore.get(r);return null==t?null:"true"===t - also fine. (284/1000)
  • a library used by many sites contains if ($cookieStore.get('moyasarId') === null || $cookieStore.get('moyasarId') === '' || $cookieStore.get('moyasarId') === undefined) { - this seems like it would handle null and undefined equally (23/1000)
• These look like they'd fail if we changed the return value:
  • library served off of wovn.io: var n=this.cookieStore.get(a.WovnCookies.WOVN_MTM_SHOWED_LANGS);return null!==n?JSON.parse(n):[] (130/1000)

Notably, JSON.parse(undefined) throws while JSON.parse(null) returns null, and JSON-parsing cookies seems to be fairly common (a handful of other pages, e.g. https://www.justfab.se/assets/dist/js/vueapp_m-c5291be04a.js has t=this.cookieStore.get(this.$options.cookie.name),t=JSON.parse(t)

So... I suspect it is not web-compatible to fix and we should instead update the spec to have get() return null. sigh

Thoughts?

[domenic]

I was going to say this would be unfortunate as it would make this fail to match all other maplike get()s on the web platform. But then I found out that Fetch's Headers and URL's URLSearchParams are already using null for their get()s. So, I guess maybe it's fine. /cc @annevk.

[annevk]

Yeah, we spotted the mismatch too late there. Not entirely sure what maplike and such in IDL are doing.

[annevk]

It seems this has been addressed in the specification at some point? I.e., get() returns null. Chromium bug is still open though.

[inexorabletash]

It seems this has been addressed in the specification at some point? I.e., get() returns null. Chromium bug is still open though.

get(name) step 6.3 still says "If list is empty, then resolve p with undefined." - where are you seeing it return null?

[annevk]

Oh, I was just looking at the IDL, which doesn't allow undefined. My bad.