Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinguish exceptions on .request() #352

Open
marcoscaceres opened this issue Sep 28, 2022 · 15 comments
Open

Distinguish exceptions on .request() #352

marcoscaceres opened this issue Sep 28, 2022 · 15 comments
Assignees

Comments

@marcoscaceres
Copy link
Member

I'm wondering if we should distinguish a few exceptions it more obvious why things failed? (without relying on error messages).

For instance, in request(), when document is null or not fully active or hidden, maybe an InvalidStateError might be good there?

@reillyeon
Copy link
Member

This seems reasonable. Are there other examples where InvalidStateError is used for these kinds of failures?

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Sep 29, 2022

I'm a bit bias, as I edited the following, but web share's .share() uses InvalidStateError for non-fully active. Similarly, Permissions API's query() method also uses InvalidStateError.

Searching webkit's code base, WebLockManager, AudioContext, and XMLHttpRequest also rely on InvalidStateError for non-fully active.

@marcoscaceres
Copy link
Member Author

For visibility state hidden rejections, perhaps we should use AbortError? As the "operation was aborted".

That covers all the cases, I think.

@reillyeon
Copy link
Member

I don't think we need to distinguish between not fully-active and not visible. InvalidStateError seems reasonable for both, though the current NotAllowedError also seems reasonable. The question I ask myself when thinking about this sort of thing is whether there is a different action the developer would take depending on the error. The two situations I can imagine are transient failures (not fully-active and not visible) and permanent errors (permissions policy failure). Using InvalidStateError for transient failures and NotAllowedError for permanent ones seems like a reasonably clear distinction. A developer could try again in the former case while giving up and taking a different approach in the latter.

@marcoscaceres
Copy link
Member Author

I don't think we need to distinguish between not fully-active and not visible. InvalidStateError seems reasonable for both

Yeah, I'd be ok with that.

though the current NotAllowedError also seems reasonable.

The definition of NotAllowedError is so large in scope that it could indeed apply. However, I personally tend to reserve NotAllowedError for when the user doesn't allow something.

The question I ask myself when thinking about this sort of thing is whether there is a different action the developer would take depending on the error.

I'm approaching it more from "why did it fail?" instead of what action can one take (i.e., taking some of the guess work out of it, otherwise it could have failed for any reason in the spec).

Using InvalidStateError for transient failures and NotAllowedError for permanent ones seems like a reasonably clear distinction. A developer could try again in the former case while giving up and taking a different approach in the latter.

Agree.

@reillyeon
Copy link
Member

The question I ask myself when thinking about this sort of thing is whether there is a different action the developer would take depending on the error.

I'm approaching it more from "why did it fail?" instead of what action can one take (i.e., taking some of the guess work out of it, otherwise it could have failed for any reason in the spec).

That's what the message is for. If we are explaining the error to the developer then we have as many words as we want to do that. What I want to avoid is developers running regular expressions over the message string to figure out what step to take next. That's where having well-considered use of the DOMException types is important.

@marcoscaceres
Copy link
Member Author

That's what the message is for. If we are explaining the error to the developer then we have as many words as we want to do that.

Ideally want to avoid that, as the message allows for further UA identification (why I asked in the OP "without relying on error messages").

That's where having well-considered use of the DOMException types is important.

Exactly.

@reillyeon
Copy link
Member

I would like to see some documentation of this threat model before considering it in the design of specifications. I would consider exception messages to be equivalent to the browser engine major version, which is already available to script.

If there is concern about leaking sensitive information to script then I know Blink has a mechanism for providing sanitized and unsanitized error messages, the latter only being available on the Javascript console and not to script. I suspect this feature was inherited from WebKit.

@marcoscaceres
Copy link
Member Author

It's complicated, but see:
whatwg/webidl#1024 (and whatwg/webidl#1024 (comment) in particular)

If there is concern about leaking sensitive information to script then I know Blink has a mechanism for providing sanitized and unsanitized error messages, the latter only being available on the Javascript console and not to script. I suspect this feature was inherited from WebKit.

No, it's not really about that... it's just about reducing differences between browsers (and also for i18n reasons, as described in the links).

@reillyeon
Copy link
Member

I don't see consensus on a resolution to that issue however I'm okay with Anne's recommendation to standardize the message value to reduce differences between browsers. Since you've proposed removing the permission every reason for the API to reject is some kind of developer error and would never be presented to the user. How about we keep using NotAllowedError (so I don't need to try to consider whether this change will break compatibility with what we've shipped in Chromium) and specify the messages for each of the failure types.

@marcoscaceres
Copy link
Member Author

How about we keep using NotAllowedError (so I don't need to try to consider whether this change will break compatibility with what we've shipped in Chromium) and specify the messages for each of the failure types

That still feels kinda not right to me. My preference would be to use the exception types, as there is plenty of precedence (#352 (comment)) at least for InvalidStateError for non-fully active.

@marcoscaceres
Copy link
Member Author

Ping on this one too! Could the Editors please address this bug.

@reillyeon
Copy link
Member

@rakuco, please take a look at the suggestions here and propose a specification change to align the behavior between implementations.

@smaug----
Copy link

I'm not happy with specifying error messages here. If we want to distinguish various error cases, different exception types should be used. So, I agree with #352 (comment)

@reillyeon
Copy link
Member

Change agreed on at TPAC 2024:

  • fully active document checks: NotAllowedError -> InvalidStateError
  • visibility check: NotAllowedError -> InvalidStateError
  • permission policy checks: NotAllowedError
  • permission checks: NotAllowedError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants