Particularly with IE11 adding support for this, we need to act on this now, else people will start thinking __proto__ is fair game in their code that cares about performance. This patch makes calling the __proto__ setter warn, once per setter. There's one minor problem here: it also causes use of __proto__ in an object initializer to warn, because the way we evaluate JSOP_INITPROP for the name __proto__ is by effectively doing a JSObject::setProperty (which usually finds Object.prototype.__proto__). So that needs to be rewritten somewhat to avoid this warning. Possibly __proto__ setting inside an object literal should be handled with a special JSOp that will do a prototype-set without completely trashing type/shape/property information, in a way that doesn't depend on invoking the __proto__ setter. But this patch is perfectly acceptable if we decided we didn't care about this issue (which I suspect we do), or once that issue's fixed, so it's worth posting in any case.

Comment on attachment 8345031 [details] [diff] [review] Warn once when a global's __proto__ setter is called Review of attachment 8345031 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/GlobalObject.cpp @@ +189,5 @@ > + // use of the __proto__ setter on unacceptable values, where no subsequent > + // use occurs on an acceptable value, will trigger a warning. > + RootedObject callee(cx, &args.callee()); > + if (!GlobalObject::warnOnceAboutPrototypeMutation(cx, callee)) > + return false; Why do you return without calling ProtoSetterImpl if warnOnceAboutPrototypeMutation fails to log the warning?

Comment on attachment 8345031 [details] [diff] [review] Warn once when a global's __proto__ setter is called Review of attachment 8345031 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/js.msg @@ +228,4 @@ > MSG_DEF(JSMSG_NESTING_GENERATOR, 175, 0, JSEXN_TYPEERR, "already executing generator") > MSG_DEF(JSMSG_UNUSED176, 176, 0, JSEXN_NONE, "") > MSG_DEF(JSMSG_UNUSED177, 177, 0, JSEXN_NONE, "") > +MSG_DEF(JSMSG_PROTO_SETTING_SLOW, 178, 0, JSEXN_TYPEERR, "mutating the [[Prototype]] of an object will cause your code to run very slowly; don't do it!") It would be helpful for lazy JavaScript developers if the warning message suggested an alternative (like Object.create()?) to overriding __proto__.

(In reply to Chris Peterson (:cpeterson) from comment #1) > Why do you return without calling ProtoSetterImpl if > warnOnceAboutPrototypeMutation fails to log the warning? The return-false occurs if an error happened when attempting to report the warning. Pending error means failure, so nothing more can be done. This is standard practice. (It really should only happen if the user sets the werror preference -- which they shouldn't, it means by definition JS spec violations -- or hits OOM, which is rare and likely will happen down the line anyway if we chose not to make it happen here.) It appears I uploaded an un-qref'd version of the patch, because locally mine does suggest Object.create. No matter as long as we still have bug 948583 to address first.

Comment on attachment 8347676 [details] [diff] [review] Patch Review of attachment 8347676 [details] [diff] [review]: ----------------------------------------------------------------- Good. Sorry for the atrocious lag time on this review. r=me ::: js/src/vm/GlobalObject.cpp @@ +165,5 @@ > ProtoSetter(JSContext *cx, unsigned argc, Value *vp) > { > CallArgs args = CallArgsFromVp(argc, vp); > + > + // Do this here, rather than in |ProtoSetterImpl|, so even likely-buggy This is fine for now. If we move away from the CallGeneric model (bug 950897), we will have to find a new place for it. ::: js/src/vm/GlobalObject.h @@ +108,4 @@ > static const unsigned DATE_TIME_FORMAT_PROTO = NUMBER_FORMAT_PROTO + 1; > static const unsigned REGEXP_STATICS = DATE_TIME_FORMAT_PROTO + 1; > static const unsigned WARNED_WATCH_DEPRECATED = REGEXP_STATICS + 1; > + static const unsigned WARNED_PROTO_SETTING_SLOW = WARNED_WATCH_DEPRECATED + 1; desperately sad this isn't an enum, but what can you do.

I landed the patch here, with some tweaks to not quite start warning yet: https://hg.mozilla.org/integration/mozilla-inbound/rev/23061213bdd1 Once bug 948583 part two has landed (maybe tomorrow, possibly I'll wait til Monday for peace of mind in bisecting any issues adding JSOP_MUTATEPROTO created), enabling this fully is just a matter of uncommenting a line and removing an adjacent explanatory comment and |return true|, in the obvious place. But this at least cements this in place so I don't have to keep rebasing it all the time. :-)

Final bit to uncomment the warning path, now that { __proto__: ... } doesn't invoke this: https://hg.mozilla.org/integration/mozilla-inbound/rev/be0c71ad1f18

This was backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/b199295c3537 under suspicion of turning mochitest-1 intermittently orange on Windows runs: https://tbpl.mozilla.org/php/getParsedLog.php?id=33841065&tree=Mozilla-Inbound

>Object.getOwnPropertyNames(Object.prototype).indexOf("__proto__") // -1 >Object.getOwnPropertyDescriptor(Object.prototype,"__proto__") ({configurable:true, enumerable:false, get:function () { [native code] }, set:function () { [native code] }}) I don't get this.

The mochitest-1 intermittent orange worries appear unfounded, per https://tbpl.mozilla.org/?tree=Try&rev=b49ebbf3e9ef Repushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b79d5726ac06

This should go in the developers notes for Firefox 30 and should have an example of what people should instead (or a pointer to something on devmo).

Can I see an example of this?

(In reply to Kohei Yoshino [:kohei] from comment #14) > Can I see an example of this? This code, used anywhere in content or chrome script, will trigger the warning: var o = {}; o.__proto__ = {}; and so will this: var o = {}; Object.setPrototypeOf(o, {}); The warning itself is: "TypeError: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create"

(In reply to Till Schneidereit [:till] from comment #15) > and so will this: > > var o = {}; Object.setPrototypeOf(o, {}); Why ES6 guys are going to add setPrototypeOf in the first place?

(In reply to Masatoshi Kimura [:emk] from comment #16) > Why ES6 guys are going to add setPrototypeOf in the first place? IIRC, the reasoning was, roughly, that the ship has sailed on __proto__ and there are some valid use cases that can't be satisfied without it currently. They can with ES6 concepts like classes and the @@create semantics enabling subclassing of exotic builtins (such as Array and Date). Those, however, aren't polyfillable, so setPrototypeOf was introduced as a polyfillable, slightly-better-behaved alternative to __proto__.

(In reply to Till Schneidereit [:till] from comment #15) > This code, used anywhere in content or chrome script, will trigger the > warning: > > var o = {}; o.__proto__ = {}; > > and so will this: > > var o = {}; Object.setPrototypeOf(o, {}); I don't get a warning with those snippets on 30.0a2 (2014-04-28). Was this tested?

The warning only appears if you've turned on strict warnings, or extra warnings, or whatever they're called these days. I expect reasonable web developers and such to flip it, to learn when they're doing dumb things, as here. If they don't, no biggie, they probably didn't care that hard about it anyway. For what it's worth, there's contention on the value at all of prototype mutation. If it were me I would have left all of this out of any spec at all -- it's stupid and you can do just as powerful things without it, with slightly different approaches. But stupid mobile web stupid developers stupid hate aaargh, so here we are. :-)

Hmm, somehow the strict options was disabled. I usually enable the option in my Firefox profile for Web development. Now I get the warning. Thanks!

Updated the site compat doc to include this: https://developer.mozilla.org/en-US/Firefox/Releases/30/Site_Compatibility#JavaScript These documents already have the warnings: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf

Hello all. Are there any examples what to do with __proto__ based code to switch to Object.create? This is what I'm currently using in a few add-ons and I have no idea how to switch to Object.create: var StylesheetManager = { uri: null, stylesheetService: Cc["@mozilla.org/content/style-sheet-service;1"].getService(Ci.nsIStyleSheetService), init: function() { let stylesheet = this.stylesheet; if (this.uri && this.uri.spec == stylesheet) return; this.shutdown(); this.uri = Services.io.newURI(stylesheet, null, null); this.stylesheetService.loadAndRegisterSheet(this.uri, Ci.nsIStyleSheetService.USER_SHEET); }, shutdown: function() { if (!this.uri) return; this.stylesheetService.unregisterSheet(this.uri, Ci.nsIStyleSheetService.USER_SHEET); this.uri = null; } }; var Overlay = { __proto__: StylesheetManager, stylesheet: "chrome://extension/content/overlay.css" };

Alright found a way. var Overlay = Object.create(StylesheetManager); Overlay.stylesheet = "chrome://extension/content/overlay.css"; This would also work: var Overlay = Object.create(StylesheetManager, { stylesheet: { value: "chrome://extension/content/overlay.css", writable: true, enumerable: true, configurable: true } });

Could someone with knowledge on what is going on here actually update the docs on MDN to say *what* is made slower by this? “Unavoidably slows down subsequent execution in modern JavaScript implementations” is completely non-specific and unhelpful regarding the scope of the actual slowdowns that JavaScript authors will encounter if they do this. Thanks!

The nature of JITs, and of how JavaScript JITs work, makes it difficult to be very specific here. The gist of it is that JS JITs go to great lengths to assume things about the prototype chain of objects on which properties are accessed. It is the incredibly, incredibly common case that objects' prototype chains (the entire length) are set in stone at time of object creation. If this is the case, any access that depends upon the prototype chain (missing property accesses, calls to functions on Object.prototype or a more-derived object like String.prototype) can depend on that sequence being fully known. If the sequence can be mutated, however, accesses generally *do* have to do the full [[Prototype]] chain walk even when they know a particular [[Prototype]] chain will be in place. The issue for specificity is that's not really what we do. Instead, we "assume" the chain won't change. And if it *does* change, then we more or less throw out *all* JIT code, everywhere, to invalidate any code that might make bad assumptions about [[Prototype]] sequences. So there's a bad hit at the time of prototype mutation, while things heat up again. It gets worse. We want to completely segregate objects whose prototype chains mutate, from objects whose prototype chains are assumed not to mutate, to cordon off the damage as best as we can. So when you set the [[Prototype]] of an object, we tag that object as having "unknown properties". The JIT will no longer assume pretty much anything at all about the properties of the object, because you screwed with the natural order of the world. And this problem "taints" all performance-sensitive code through which that object flows. Any "hot" loop that accesses object properties, that that mutated-[[Prototype]] object flows through, will deoptimize to account for the possibility of an object with unknown properties passing through. So *any* use of that loop becomes slow, not just uses that pass in the mutated-[[Prototype]] object. I've been thinking about writing a longer MDN article discussing [[Prototype]] mutation, the significant dangers it poses and why you shouldn't do it, and elaborating on the ways this problem can be fixed. A few other things are higher priority right now, for sure, but maybe I can get to it in a few weeks or so.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25) > I've been thinking about writing a longer MDN article discussing > [[Prototype]] mutation, the significant dangers it poses and why you > shouldn't do it, and elaborating on the ways this problem can be fixed. A > few other things are higher priority right now, for sure, but maybe I can > get to it in a few weeks or so. Looking forward to your MDN article!

Funny thing I noticed. While add-on devs see a warning on add-ons validator not to use __proto__ for mutating Prototype, default Firefox modules still use it causing warnings in error console. >Warning: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create >Source file: resource://gre/modules/Preferences.jsm >Line: 378 >Preferences.__proto__ = Preferences.prototype;

(In reply to Aris from comment #27) > Funny thing I noticed. > While add-on devs see a warning on add-ons validator not to use __proto__ > for mutating Prototype, default Firefox modules still use it causing > warnings in error console. Can you please file bugs against the affected modules? Thanks!

The Preferences.jsm warning is bug 982856. If there are others, please do file bugs on them. We filed, and fixed, bugs on many of them. That particular bug, I pushed a certain distance before having to drop it and return to purely JS engine work.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25) > The nature of JITs, and of how JavaScript JITs work, makes it difficult to > be very specific here. > > The gist of it is that JS JITs go to great lengths to assume things about > the prototype chain of objects on which properties are accessed. It is the > incredibly, incredibly common case that objects' prototype chains (the > entire length) are set in stone at time of object creation. If this is the > case, any access that depends upon the prototype chain (missing property > accesses, calls to functions on Object.prototype or a more-derived object > like String.prototype) can depend on that sequence being fully known. > > If the sequence can be mutated, however, accesses generally *do* have to do > the full [[Prototype]] chain walk even when they know a particular > [[Prototype]] chain will be in place. The issue for specificity is that's > not really what we do. Instead, we "assume" the chain won't change. And if > it *does* change, then we more or less throw out *all* JIT code, everywhere, > to invalidate any code that might make bad assumptions about [[Prototype]] > sequences. So there's a bad hit at the time of prototype mutation, while > things heat up again. > > It gets worse. We want to completely segregate objects whose prototype > chains mutate, from objects whose prototype chains are assumed not to > mutate, to cordon off the damage as best as we can. So when you set the > [[Prototype]] of an object, we tag that object as having "unknown > properties". The JIT will no longer assume pretty much anything at all > about the properties of the object, because you screwed with the natural > order of the world. And this problem "taints" all performance-sensitive > code through which that object flows. Any "hot" loop that accesses object > properties, that that mutated-[[Prototype]] object flows through, will > deoptimize to account for the possibility of an object with unknown > properties passing through. So *any* use of that loop becomes slow, not > just uses that pass in the mutated-[[Prototype]] object. > > I've been thinking about writing a longer MDN article discussing > [[Prototype]] mutation, the significant dangers it poses and why you > shouldn't do it, and elaborating on the ways this problem can be fixed. A > few other things are higher priority right now, for sure, but maybe I can > get to it in a few weeks or so. Thanks for the explanation, this is really informative. Does 'ways this problem can be fixed' imply that workarounds are in mind for the two scenarios where __proto__ is required (creating Function and Array instances with prototypes)? Or just that there are potentially ways to avoid the permanent deopt?

Interesting, we have a Class system library which used `__proto__` to create the deep inheritance chain. After Firefox started to drop the warning, I moved the inheritance creation to `Object.create` and after the bunch of perf. tests, it seems there is **NO** difference in performance (Class Creation, Class Initialization, Property Accessors). I would like to stay with the `__proto__` approach, but now our library consumers will be discouraged with the warning. - pity

It would be useful to see your actual tests, to start. It's depressingly easy to not measure the correct thing when writing perf tests. It's quite possible that your tests don't measure the actual performance pitfalls mentioned in comment 25, but rather measure something that happens to be the same for both Object.create usage, and for __proto__ mutation. Another possibility is that we've only lightly optimized Object.create thus far, and so it's closer to __proto__-mutation performance than it could be. Again, seeing the actual code being tested would be helpful here.

I haven’t verified the work, but this group reported testing and seeing the performance impact: http://windward.net/blogs/high-performance-javascript-assign-prototypes/

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #32) > It would be useful to see your actual tests, to start. I have uploaded the tests to jsperf: http://jsperf.com/classjs-core-proto-vs-object-create Tests are complex (integrational performance tests) - we create some classes with the chain inheritance, initialize them, and access all the properties (deep inherited also). The two functions only differs in how the classes are inherited. So the difference in time shows the time for the creation (__proto__ vs `Object.create + extend`) and the prototype chain lookup accessors.