WICG / cookie-store

Asynchronous access to cookies from JavaScript
https://wicg.github.io/cookie-store/
Apache License 2.0
143 stars 35 forks source link

cookieStore throws instead of returning rejected Promise #211

Closed jameshartig closed 1 year ago

jameshartig commented 1 year ago

This might just be theoretical because I haven't been able to get this to happen so I apologize ahead of time if this is not an actual issue but when looking at the source in Chromium I see:

  if (!context->GetSecurityOrigin()->CanAccessCookies()) {
    exception_state.ThrowSecurityError(
        "Access to the CookieStore API is denied in this context.");
    return ScriptPromise();
  }

and

  if (!backend_) {
    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
                                      "CookieStore backend went away");
    return ScriptPromise();
  }

For the first case, the Draft says

  1. If origin is an opaque origin, then return a promise rejected with a "SecurityError" DOMException.

Shouldn't at least the first case be returning a rejected promise?

For the second case, it's a bit unusual if there's some internal error but I'd like to not have to wrap all cookieStore usages in a try/catch. Shouldn't it be returning a rejected Promise?

I'm having trouble testing this since cookieStore is undefined on about:blank and http://example.com.

inexorabletash commented 1 year ago

Those are a quirk of how Chromium's WebIDL bindings are implemented. If a method is defined as returning a promise, the thrown exception is automagically turned into the promise rejection value.

If you try it out you should see the correct behavior - no rejection and a rejected promise.

mkruisselbrink commented 1 year ago

Note that this is not just a quirk of Chromium's WebIDL bindings. The same is also true of specs and the WebIDL spec itself. I.e. in the create an operation function algorithm there is this: "if an exception E was thrown: If op has a return type that is a promise type, then return ! Call(%Promise.reject%, %Promise%, «E»)." So even if a spec says to throw an exception in a promise-returning method, the actual method will return a rejected promise instead.

inexorabletash commented 1 year ago

And yes, those would be difficult to test. The former requires the user to set policy saying storage is disabled, so it can't be tested by WPT. The second would basically signal that something is catastrophically wrong in the implementation.

jameshartig commented 1 year ago

@inexorabletash I appreciate the clarification and apologies for the false report. As I said, I was having a hard time testing it since I can't find an origin where the API exists but would trigger that case. I'll close this. Thanks again!