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

Suggestion: remove everything mentioned in annex B of the ECMAScript standard #7982

Closed
ghost opened this issue Oct 15, 2020 · 6 comments
Closed
Labels
suggestion suggestions for new features (yet to be agreed)

Comments

@ghost
Copy link

ghost commented Oct 15, 2020

  • Probably a breaking change, since there's so much weird legacy code that's still being written today, even in Deno.
  • Definitely breaks web compatibility, by definition.
  • Likey breaks Node compatibility.

Currently, Deno already doesn't allow access to Object.prototype.__proto__, which was later defined in annex B of the ES spec.

The section starts with the following note:

NOTE: This annex describes various legacy features and other characteristics of web browser-based ECMAScript implementations. All of the language features and behaviors specified in this annex have one or more undesirable characteristics and in the absence of legacy usage, would be removed from this specification. However, the usage of these features by large numbers of existing web pages means that web browsers must continue to support them. The specifications in this annex defined the requirements for interoperable implementations of these legacy features.

These features are not considered part of the core ECMAScript language. Programmers should not use or assume the existence of these features and behaviors when writing new ECMAScript code. ECMAScript implementations are discouraged from implementing these features unless the implementation is part of a web browser or is required to run the same legacy ECMAScript code that web browsers encounter.

Focusing on the last sentence, Deno does not fall under any of the exceptions they mention, so should Deno have these?
Does Deno even classify as an ES implementation, or would that be V8 (with Deno's Rust bindings)?
As mentioned before, __proto__ is deleted before user code is run, so why not remove the rest?

From my personal experience, everything mentioned in annex B is rarely used in modern code, the only potential exception possibly being String.prototype.substr, although, don't take my word for it.

Update

A possible alternative to outright removing them, and potentially forcing users to suffer the unexpected consequences could be that there would be a new flag added that would allow opt-in (or opt-out, as this is quite late, and users may already depend upon it), that would allow or disable annex B in Deno, which would also allow the ability to regain Object.prototype.__proto__, as it is currently considered a security vulnerability, for reasons that I personally cannot agree with.

@kitsonk kitsonk added the suggestion suggestions for new features (yet to be agreed) label Oct 15, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Oct 15, 2020

If v8 implements it, and there isn't a good reason to remove it, we won't remove it. Some of them might be really difficult to remove. Most things in Annex B are being moved out anyways, as TC39 wants to kill off Annex B.

or is required to run the same legacy ECMAScript code that web browsers encounter.

Focusing on the last sentence, Deno does not fall under any of the exceptions they mention, so should Deno have these?

Deno aims for browser compatibility where logical. So yes, it does fall under this exception. If it is implemented in v8 for "free", it is harder and riskier to un-implement it than to just allow it. You mess with some internals like that, you can easily for v8 into de-optimising code, which would be bad for everyone. We had to make with the removal of __proto__ that we weren't accidentally de-optimising everyone's code.

__proto__ is deleted before user code is run, so why not remove the rest?

Because it is a security issue, and in the move of __proto__ of of Annex B (tc39/ecma262#2125) there will be clarity around the prototype and what we did regarding the security concern.

If you have a security reason to remove the other parts of Annex B, we would potentially consider those.

@ghost
Copy link
Author

ghost commented Oct 15, 2020

...Some of them might be really difficult to remove. Most things in Annex B are being moved out anyways, as TC39 wants to kill off Annex B.

I was legitimately expecting that this would merely be a few more delete operations at startup on the Object and String prototypes.
From what I could tell while looking around that proposal, they didn't seem to be at a complete consensus, as most still disliked the existence of the items in annex B, while others went so far as to outright call everything within annex B a "mistake" within ECMAScript itself (and I have to agree).
Ex: tc39/ecma262#2125 (comment)

But, it still appears that they did not want to make anything in that annex required to be implemented, in order to be considered a spec-compliant ES engine. They would remain optional normative, meaning that Deno is not required to implement them, and is still discouraged from doing so.

or is required to run the same legacy ECMAScript code that web browsers encounter.

...
Deno aims for browser compatibility where logical. So yes, it does fall under this exception.

(emphasis mine)

Deno frankly cannot run all legacy code if it tried, as, by default, user code is run as a module. This implies strict mode, which is known to break old code.
I doubt that people have been grabbing their 20-year-old scripts and trying to run them in Node, Deno, or another non-browser runtime.
But I must say, to whoever it was on the Deno team who suggested modules as the default, thank you, it really feels like Deno can be a modern runtime, as most websites are still hesitant to use them because of legacy support (ex: IE).

...You mess with some internals like that, you can easily for v8 into de-optimising code, which would be bad for everyone. We had to make with the removal of __proto__ that we weren't accidentally de-optimising everyone's code.

If you rewrite V8 without it, I could understand the potential to cause an error, but, from my naive understanding of V8, if you were to just delete them as the Deno team had done to __proto__, the worst that could happen would be that reads to the Object.prototype would slow down, thus all object lookups globally would suffer, yet freezing the prototype immediately after should put the object into a sort of "struct" reading mode, allowing the normal "fast write" - "slow read" that is expected of V8 objects.
This type of deoptimization is more impactful after an object has been moved around after its initial creation, so might I suggest, that immediately upon startup, annex B is deleted and the prototypes frozen? I might even go so far as to claim that the benchmarks should improve, as there are now fewer keys to lookup in each read/write in the prototypes, thus allowing for less memory overhead and better runtime performance (although, the benchmarks will have to run in order to confirm this).

__proto__ is deleted before user code is run, so why not remove the rest?

Because it is a security issue, and in the move of __proto__ [out] of Annex B (tc39/ecma262#2125) there will be clarity around the prototype and what we did regarding the security concern.

If you have a security reason to remove the other parts of Annex B, we would potentially consider those.

In my opinion, the example @ry had given was an exceptionally poor choice. JSON.parse on { "__proto__": null } with string concatenation?
If I must ask, why is it that browsers successfully parse that JSON, while still allowing __proto__? Maybe the algorithm used for Deno's JSON.parse just needs to be fixed?
How is that a security vulnerability? Because your code threw an error? Well-written code doesn't have anything along the lines of "foo" + {}, at least last time that I had checked, and as a bonus: TypeScript normally prevents that too.

@ghost
Copy link
Author

ghost commented Oct 16, 2020

Update:
I take back what I had naively claimed:

...yet freezing the prototype immediately after should put the object into a sort of "struct" reading mode, allowing the normal "fast write" - "slow read" that is expected of V8 objects...

It seems like, as of 2019-2020 the V8 developers have yet to fix the bugs regarding their implementation of Object.freeze, as it causes both, their interpreter and JiT, to slow down, due to the objects being put into "dictionary mode," which exists on the opposite end of the spectrum: slow reads, fast writes.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 20, 2020

As I have stated above, our stance is not to proactively remove Annex B features that automatically exist in v8 without a good reason. Closing the issue.

@kitsonk kitsonk closed this as completed Oct 20, 2020
@petamoriken
Copy link
Contributor

Annex B will be annotated as @deprecated in TypeScript 4.5 and later, it will may be possible to prevent it from being used.

microsoft/TypeScript#43709

@kitsonk
Copy link
Contributor

kitsonk commented Sep 1, 2021

I still don't think we would prevent them from being used. People will get feedback in the editor though that they are deprecated and deno_lint should reflect those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

2 participants