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

Reject with meaningful exceptions vs undefined. #37

Closed
Brandr0id opened this issue May 16, 2020 · 11 comments
Closed

Reject with meaningful exceptions vs undefined. #37

Brandr0id opened this issue May 16, 2020 · 11 comments
Labels
needs edit This issue is waiting on a PR to be written. resolve before graduation These issues need to be resolved before the spec graduates from the CG

Comments

@Brandr0id
Copy link
Member

It may be nice to reject failed calls to requestStorageAccess with an exception + message.

e.g. if no gesture => reject with a new DomException indicating "The method cannot be executed without a user gesture." and so on.

Any objections to this? I believe currently the promises are just rejected.

@johnwilander
Copy link
Collaborator

We have already seen misuse of the reject signal where the caller times it and if there’s a delay, it concludes that the user was prompted and clicked “Don’t allow.” In this specific case, it’s an embedded video and if the timer tells the embedee that the user explicitly denied them cookie access, they stop video playback. However, if the promise is rejected immediately, playback continues. This means users who are prompted and deny them access are punished. The result is typically that the user tries again, is prompted again, and this time opts in even though they originally didn’t want to.

If we give the caller explicit info on why the promise was rejected, we will likely see more misuse. I have instead considered introducing an artificial delay to make it harder to harass/punish users.

@Brandr0id
Copy link
Member Author

We have already seen misuse of the reject signal where the caller times it and if there’s a delay, it concludes that the user was prompted and clicked “Don’t allow.”

Do you happen to have concrete examples of sites this was seen on?

However, if the promise is rejected immediately, playback continues.

Is this deliberate JS to do this or an accidental race in client side code?

If we give the caller explicit info on why the promise was rejected, we will likely see more misuse. I have instead considered introducing an artificial delay to make it harder to harass/punish users.

If a site wants to be punitive towards users who reject storage access and prevent playback or put up a paywall do we have any reason to believe they would be more inclined to do so for one rejection reason over another? The result is the same that they have no storage access.

@annevk
Copy link
Collaborator

annevk commented May 16, 2020

The exception doesn't have to include information, but we if we reject a promise it must be with an exception: https://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors.

Rejecting with a JavaScript TypeError seems reasonable.

(User agents could of course include relevant information that's visible to the developer console, but not the page.)

@jackfrankland
Copy link

We have already seen misuse of the reject signal where the caller times it and if there’s a delay, it concludes that the user was prompted and clicked “Don’t allow.” In this specific case, it’s an embedded video and if the timer tells the embedee that the user explicitly denied them cookie access, they stop video playback. However, if the promise is rejected immediately, playback continues. This means users who are prompted and deny them access are punished. The result is typically that the user tries again, is prompted again, and this time opts in even though they originally didn’t want to.

If we give the caller explicit info on why the promise was rejected, we will likely see more misuse. I have instead considered introducing an artificial delay to make it harder to harass/punish users.

Just want to mention that a similar thing could be used in relation to the discussion on #25. If an explicit reject consumes the user activation, while an implicit reject does not, the third party could subsequently call a user activation gated api (maybe easiest would be audio.play?) to test the difference.

@Brandr0id
Copy link
Member Author

The exception doesn't have to include information, but we if we reject a promise it must be with an exception: https://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors.

Rejecting with a JavaScript TypeError seems reasonable.

As long as we resolve with some type of error or exception that affords implementers the ability to make a messaging decision for developers that would be great. If we could all agree on common messaging that would be great but it seems reasonable to leave that up to implementers.

@johnwilander @englehardt does it seem reasonable to use TypeErrors for this purpose or are there any other preferences/better fits?

@hober hober assigned johnwilander and hober and unassigned johnwilander Jun 2, 2020
@hober hober added the needs edit This issue is waiting on a PR to be written. label Aug 10, 2020
@johannhof
Copy link
Member

For Firefox we're also not really interested in disclosing a user choice to the site via error type or message. We have recently added more extensive error logging which seems like a good compromise. It is a bit unfortunate for web developers that the Storage Access API has a rather complicated grant algorithm and many implementation specific pieces, so I can understand why you would want a better way to react to rejections. However, looking at potential ways for relaying the rejection to users I can see a lot more potential for abuse than helpful clarification from websites. So it might be preferable to stay unclear here.

But rejecting with a generic error type to follow Promise conventions would certainly be good.

@johannhof
Copy link
Member

To reiterate past comments, we won't provide detailed reasons for rejection but it might be reasonable to discuss throwing a specific error to follow existing conventions, instead of rejecting with undefined, which I think the implementations do right now. The editors consider this future work.

@annevk
Copy link
Collaborator

annevk commented Mar 23, 2022

#37 (comment) has a pretty concrete solution for this. Why can this not be addressed sooner? We don't have any instances of web platform promises that reject with undefined (unless explicitly instructed to do so as is possible with abort signals). They all reject with an exception. This is like having a function throw undefined, it's rather bad.

@annevk annevk added the agenda+ Request to add this issue to the agenda of our next telcon or F2F label Mar 23, 2022
@AramZS
Copy link

AramZS commented Apr 14, 2022

I do think a generic exception is better than undefined. I see no problem from the developer point of view with this, a failure is a failure and the same actions would be taken in response. Presumably logging would allow for detection of a code vs permissions error due to scale. Almost certainly developers testing this would be able to distinguish between a Choice vs code breakage. If that is the issue and there is some reason why that might not be the case additional developer tooling might be the way to go, not more detailed errors.

@erik-anderson erik-anderson removed the agenda+ Request to add this issue to the agenda of our next telcon or F2F label May 9, 2022
@johannhof johannhof removed the future Will consider for a future revision label Aug 18, 2022
@johannhof johannhof self-assigned this Aug 18, 2022
@johannhof johannhof added the resolve before graduation These issues need to be resolved before the spec graduates from the CG label Sep 11, 2022
@johannhof johannhof removed the resolve before graduation These issues need to be resolved before the spec graduates from the CG label Sep 12, 2022
bvandersloot-mozilla added a commit to bvandersloot-mozilla/storage-access that referenced this issue Sep 23, 2022
@johannhof johannhof assigned johannhof and unassigned johannhof Sep 26, 2022
@johannhof
Copy link
Member

I think @bvandersloot-mozilla wanted to take a stab at this but I can't assign you, Ben. We should figure out access rights to the repo with the chairs.

@johannhof johannhof added the resolve before graduation These issues need to be resolved before the spec graduates from the CG label Sep 26, 2022
@johannhof johannhof removed their assignment Sep 27, 2022
@johannhof
Copy link
Member

Fixed by #118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs edit This issue is waiting on a PR to be written. resolve before graduation These issues need to be resolved before the spec graduates from the CG
Projects
None yet
Development

No branches or pull requests

8 participants