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: don't inherit from Object.prototype #6092

Merged
merged 1 commit into from Apr 19, 2016

Conversation

@mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 7, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • events
Description of change

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

@mscdex mscdex force-pushed the mscdex:events-empty-prototype branch Apr 7, 2016
@mscdex
Copy link
Contributor Author

@mscdex mscdex commented Apr 7, 2016

Also, there is no performance regression with this change 💃

@mscdex mscdex added this to the 6.0.0 milestone Apr 7, 2016
@thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Apr 7, 2016

Can we make use of the test changes in #2350?

@mscdex
Copy link
Contributor Author

@mscdex mscdex commented Apr 7, 2016

@thefourtheye The included tests should cover it I think

@evanlucas
Copy link
Member

@evanlucas evanlucas commented Apr 7, 2016

@mscdex will this have any affect on process since it sets _events to a regular object?

@vkurchatkin
Copy link
Contributor

@vkurchatkin vkurchatkin commented Apr 7, 2016

@evanlucas it makes sense to simply call EventEmitter on process

@mscdex mscdex force-pushed the mscdex:events-empty-prototype branch Apr 7, 2016
@mscdex
Copy link
Contributor Author

@mscdex mscdex commented Apr 7, 2016

@evanlucas
Copy link
Member

@evanlucas evanlucas commented Apr 7, 2016

LGTM

process->Set(env->events_string(), Object::New(env->isolate()));
Local<Object> events_obj = Object::New(env->isolate());
events_obj->SetPrototype(env->context(), Null(env->isolate()));
process->Set(env->events_string(), events_obj);

This comment has been minimized.

@Fishrock123

Fishrock123 Apr 7, 2016
Member

Is this process._events?

This comment has been minimized.

@mscdex

mscdex Apr 7, 2016
Author Contributor

Yes.

@@ -21,7 +21,6 @@ assert(fl.length === 1);
assert(fl[0] === assert.fail);

e.listeners('bar');
assert(!e._events.hasOwnProperty('bar'));

This comment has been minimized.

@Fishrock123

Fishrock123 Apr 7, 2016
Member

won't this still be true?

This comment has been minimized.

@addaleax

addaleax Apr 7, 2016
Member

There is no _events.hasOwnProperty anymore – you could do something like Object.prototype.hasOwnProperty.call(e._events, 'bar') here, I guess?

This comment has been minimized.

@Fishrock123

Fishrock123 Apr 7, 2016
Member

Oh good catch. Guess that makes it semver-major, We should probably use what you suggested to check it though.

This comment has been minimized.

@jasnell

jasnell Apr 7, 2016
Member

Yep, definitely semver major. There are other side effects of this. For instance, there is no toString or valueOf so doing a console.log or util.inspect will yield different results and many of the Object.{method} static methods may croak a bit on it (e.g. Object.keys will still work but I believe some of the others fail).

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Apr 7, 2016

Hmmm, can this be back-ported? Theoretically, this could be considered a security issue for unvalidated input.

@Fishrock123 Fishrock123 mentioned this pull request Apr 7, 2016
3 of 3 tasks complete
@mscdex
Copy link
Contributor Author

@mscdex mscdex commented Apr 7, 2016

@Fishrock123 It's definitely a semver-major change, but I don't recall how that plays out with backporting/LTS/etc.

@mscdex mscdex mentioned this pull request Apr 7, 2016
3 of 3 tasks complete
@mscdex
Copy link
Contributor Author

@mscdex mscdex commented Apr 7, 2016

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 7, 2016

I don't believe back porting would be possible to v4. It might be safe to get into v5 tho.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 7, 2016

LGTM

@mscdex
Copy link
Contributor Author

@mscdex mscdex commented Apr 7, 2016

CI is green except for a flaky test and citgm is green.

@mscdex
Copy link
Contributor Author

@mscdex mscdex commented Apr 18, 2016

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 18, 2016

Still LGTM

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>
@mscdex mscdex force-pushed the mscdex:events-empty-prototype branch to e38bade Apr 19, 2016
@mscdex mscdex merged commit e38bade into nodejs:master Apr 19, 2016
@mscdex mscdex deleted the mscdex:events-empty-prototype branch Apr 19, 2016
@jasnell

This comment has been minimized.

Copy link
Member

@jasnell jasnell commented on src/node.cc in e38bade Apr 19, 2016

@mscdex ... fyi... this is causing a compiler warning when building

../src/node.cc:3232:3: warning: ignoring return value of function declared with
      warn_unused_result attribute [-Wunused-result]
  events_obj->SetPrototype(env->context(), Null(env->isolate()));
  ^~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Apr 19, 2016

iirc usually we don't care about warnings

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 19, 2016

Typically, but it's nice to avoid them when possible to do so, no?

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Apr 19, 2016

I guess, just consider though, that there are tons of warnings of the same and there isn't necessarily easy ways to avoid it.

@thefourtheye thefourtheye mentioned this pull request Apr 19, 2016
2 of 2 tasks complete
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Apr 19, 2016
This patch fixes the warning introduced by the changes in e38bade.

Ref: nodejs#6092
thefourtheye added a commit that referenced this pull request Apr 19, 2016
This patch fixes the warning introduced by the changes in e38bade.

Ref: #6092
PR-URL: #6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
joelostrowski added a commit to joelostrowski/node that referenced this pull request 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>
joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
This patch fixes the warning introduced by the changes in e38bade.

Ref: nodejs#6092
PR-URL: nodejs#6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell added a commit that referenced this pull request 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>
jasnell added a commit that referenced this pull request Apr 26, 2016
This patch fixes the warning introduced by the changes in e38bade.

Ref: #6092
PR-URL: #6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@BridgeAR BridgeAR mentioned this pull request Apr 1, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants