Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

console.timeEnd('hasOwnProperty') fails hilariously #9069

Closed
pluma opened this issue Jan 21, 2015 · 13 comments
Closed

console.timeEnd('hasOwnProperty') fails hilariously #9069

pluma opened this issue Jan 21, 2015 · 13 comments
Assignees

Comments

@pluma
Copy link

pluma commented Jan 21, 2015

Because the timers are currently stored as named properties of an object, timers with funny labels will result in console.timeEnd dutifully logging NaNms.

This affects hasOwnProperty, __proto__ and likely anything else that lives on Object.prototype.

A solution would be using ES6 symbols when they land in node.

A workaround would be prefixing the property names with some arbitrary string (except _, which would re-create the problem for timers labelled _proto__) instead of using the label verbatim.

@dougwilson
Copy link
Member

Does changing the line at https://github.com/joyent/node/blob/master/lib/console.js#L43 to prop.value = Object.create(null) fix it?

@pluma
Copy link
Author

pluma commented Jan 21, 2015

Nope, because even Object.create(null) has the magical __proto__:

var x = Object.create(null);
x.__proto__ = 'lol';
console.log(x.__proto__); // null

@dougwilson
Copy link
Member

Sure, but at most, all you'll get is Error: No such label: __proto__. Everything else seems to work fine besides that, right?

@pluma
Copy link
Author

pluma commented Jan 21, 2015

Yes, that should work with that caveat.

cjihrig added a commit to nodejs/node that referenced this issue Jan 27, 2015
Console.prototype.timeEnd() returns NaN if the timer label
corresponds to a property on Object.prototype. This commit
uses a Map to construct the _times object.

Fixes: nodejs/node-v0.x-archive#9069
PR-URL: #563
Reviewed-By: Vladimir Kurchatkin <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@cjihrig
Copy link

cjihrig commented Jan 29, 2015

I have verified that this can be solved with Object.create(null). I'll submit a patch.

@pluma
Copy link
Author

pluma commented Jan 29, 2015

Indeed. The __proto__ issue seems to be absent in newer versions of V8 (as in io.js).

cjihrig added a commit that referenced this issue Feb 13, 2015
Console.prototype.timeEnd() returns NaN if the timer label
corresponds to a property on Object.prototype. This commit
uses Object.create(null) to construct the _times object.

Fixes: #9069
PR-URL: #9116
Reviewed-By: Trevor Norris <[email protected]>
@cjihrig
Copy link

cjihrig commented Feb 13, 2015

Fixed in 6c3647c

@misterdjules
Copy link

@cjihrig Do we want to leave this issue open, or open another issue to fix this in v0.10?

@cjihrig
Copy link

cjihrig commented Feb 13, 2015

@misterdjules I don't think there is a really clean fix for 0.10. I would say either leave as is with the existing few edge cases, or apply this patch, which would leave __proto__ as the only edge case.

@misterdjules
Copy link

@cjihrig We could use a different data structure for _times to store times. For instance, it could be an array of { label: 'someLabel', time: Date.now() }.

I have never used console.time and console.timeEnd before. As a result, I'm not aware of its uses cases and I'm not sure how such a change would impact performance.

What do you think?

@cjihrig
Copy link

cjihrig commented Feb 13, 2015

@misterdjules I think that solution would work. The slight performance hit should be fine since it's a console operation. However, a similar problem exists with event names, and I wouldn't recommend this approach in that code.

I would move the Date.now() call in timeEnd() before the call to findTimeWithLabel() so that you aren't timing the array search.

@misterdjules
Copy link

@cjihrig Good point, thank you! Created #9215.

@misterdjules
Copy link

Fixed in v0.10 by c8239c0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants