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

Ignore unsupported url types in setConfiguration #1346

Closed
stefhak opened this Issue Jun 6, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@stefhak
Contributor

stefhak commented Jun 6, 2017

From EKR: https://lists.w3.org/Archives/Public/public-webrtc/2017May/0166.html

(setConfiguration)

3. For each url in server.urls parse url and obtain scheme
name. If the scheme name is not implemented by the browser, or if
parsing based on the syntax defined in [ RFC7064] and [RFC7065]
fails, throw a SyntaxError.

This seems unwise as it makes it very hard to introduce new schemes.
Why aren't they just ignored?

See #1278 for context.

@fippo

This comment has been minimized.

Contributor

fippo commented Jun 6, 2017

+1 on silently ignoring / filtering unsupported schemes (and unsupported transport protocols; we might need text for that). This would be consistent with Firefox ignoring (with a warning) turns and Edge ignoring stun even though both are implementation bugs.

We have getConfiguration that allows detecting what urls were not supported.

@stefhak

This comment has been minimized.

Contributor

stefhak commented Jun 10, 2017

@fluffy do you have a comment (I'm thinking about that you have been pushing for more error reporting, and the proposal here is to remove an error and silently move on)?

@stefhak

This comment has been minimized.

Contributor

stefhak commented Jun 14, 2017

Assigning to @fluffy. Perhaps we should add this to an interim agenda in the future?

@fluffy

This comment has been minimized.

Contributor

fluffy commented Jun 22, 2017

How does the JS code know if the new scheme worked or not if we add a new one in the future?

I'm against the idea that I put a typo in a turn server name syntax and gets silently ignored. That is really hard to debug because it will be much later when ICE does not work that the impact of this comes.

@fippo

This comment has been minimized.

Contributor

fippo commented Jun 22, 2017

How about

  1. If the scheme name is implemented and there is a parsing error throw a SyntaxError
  2. If th scheme name is not implemented throw a NotSupportedError (consistent with mux policy)
    Throwing a SyntaxError on data that is generally valid seems bad.

I think silently ignoring makes developers life a lot easier but that can be considered the job of a library.

@fluffy

This comment has been minimized.

Contributor

fluffy commented Jun 22, 2017

That sounds good to me.

@fippo

This comment has been minimized.

Contributor

fippo commented Jun 22, 2017

For each url in server.urls parse url and obtain scheme name. 
If parsing based on the syntax defined in RFC7064 and RFC7065 fails, throw a SyntaxError.
If the scheme name is not implemented by the browser, throw a NotSupportedError.

I can make a PR once #1233 is done.

Referencing RFC 3986 for 'obtain scheme' seemed overkill at first but... do we have an extensibility issue here since we only reference 7064 and 7065?

@fippo

This comment has been minimized.

Contributor

fippo commented Jun 26, 2017

We might also have the same extensibility issue further down

@fluffy

This comment has been minimized.

Contributor

fluffy commented Jun 28, 2017

Wrote a PR for this at #1433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment