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

events: prefix events to prevent breaking on known object properties #728

Closed
3rd-Eden opened this issue Feb 5, 2015 · 8 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. events Issues and PRs related to the events subsystem / EventEmitter. repro-exists Issues with reproductions.

Comments

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Feb 5, 2015

In the EventEmitter the events are stored in an plain Object instance. The event names that you use are added directly as property on the object so when you event names such as __proto__ it will break. One solution is to prefix the keys with a char such as ~.

Example case:

var EventEmitter = require('events').EventEmitter;
var e = new EventEmitter();

e.on('__proto__', function (bar) {
  console.log('foo', bar);
});
e.emit('__proto__', 1);

Willing to create pull request if bug requires fixing ;)

@vkurchatkin vkurchatkin added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 5, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Feb 5, 2015

We just solved a similar problem with console timer labels. We ended up going with a Map, but Object.create(null) worked equally as well.

@Fishrock123
Copy link
Contributor

Sounds like a spot to use Map. (Assuming it has decent perf)

@3rd-Eden
Copy link
Contributor Author

3rd-Eden commented Feb 5, 2015

I don't think that the performance of Map and Weakmap are better than Object.create(null)

@meandmycode
Copy link

According to this micro-benchmark (yep) you could expect map to be twice as slow:

http://jsperf.com/map-vs-object-as-hashes/14

Hash access may well not be the/a performance bottleneck in events though.

@petkaantonov
Copy link
Contributor

@meandmycode You are comparing linear array to hash map.. you need to either use string keys or very sparse numeric indices to force the object into a dictionary mode.

Secondly the comparison is not fair unless you also factor in the cost of checking if a key is "__proto__".

So when using an object as a true string hash map (arbitrary string is supported, including proto) vs using Map, the speed is not surprisingly very much equal http://jsperf.com/map-vs-object-as-hashes/24

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2015

One thing to note is that the current version of v8 hasn't required explicit checking for __proto__.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2015

Also, @bnoordhuis just pointed out on another issue that Object.create(null) is 15-30x slower than an object literal. I don't think it's worth it in this case.

@Trott
Copy link
Member

Trott commented Mar 10, 2016

I opened pull request to add a test for this to known_issues: #5649

cjihrig added a commit to cjihrig/node that referenced this issue Mar 12, 2016
This commit adds tests for several known issues.

Refs: nodejs#1901
Refs: nodejs#728
Refs: nodejs#4778
Refs: nodejs#947
Refs: nodejs#2734
PR-URL: nodejs#5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig cjihrig added the repro-exists Issues with reproductions. label Mar 12, 2016
evanlucas pushed a commit that referenced this issue Mar 14, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
rvagg pushed a commit that referenced this issue Mar 16, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
mscdex added a commit to mscdex/io.js that referenced this issue Apr 19, 2016
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: nodejs#728
PR-URL: nodejs#6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: nodejs#728
PR-URL: nodejs#6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: #728
PR-URL: #6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. events Issues and PRs related to the events subsystem / EventEmitter. repro-exists Issues with reproductions.
Projects
None yet
Development

No branches or pull requests

8 participants