Prototype findOrStore considered harmful

Prototype 1.5.0 completely breaks CNN on certain mobile versions of Opera. All the text disappears.

We've dug up the source and it appears to be code from the common prototype.js that causes problems, CNN's prototype.js is here:

Element.extend.cache = {
  findOrStore: function(value) {
    return this[value] = this[value] || function() {
      return value.apply(null, [this].concat($A(arguments)));
    }
  }
};

Problem arises when the "value" argument is a function object. Then this[value] = this[value] requires decompilation, in order to create a string for the property name.

This is a bad idea because

  • Decompilation support is an optional feature. We disable it on many limited platforms for performance
  • Decompiling a function is in any case not a reliable way to create a unique hash key since functions can have exactly the same source code but live in different scopes or different closures, so this code as-is will create obscure bugs that are very hard to track down.

I hope the excellent Prototype devs can find a better approach. Unfortunately this code is still there in the latest version.

(Due to impatient family members I can't bug it in their tracker right now, post a link to the bug in comments if you do.)

Advertisements

7 thoughts on “Prototype findOrStore considered harmful

  1. What would be a good solution to this problem? They can't use a function as the map key unless they implement a map class themselves. There is no way I know of to get an address, ID, hash, etc for an object. Should JavaScript be extended to solve this problem?I guess I'd use something like this:function getID(obj) { if( obj === null ) return 0; if( obj === undefined ) return 1; if( ! obj.__id__ ) obj.__id__ = randNumGreaterThan1(); return obj.__id__;}And call it when I wanted a map key.this[getID(value)] = this[value] || function() {…}

  2. Yes, something like that is probably what I would do too.Regarding your question "should JavaScript be extended..": quoting a somewhat related discussion on whether to add some form of unique object ID to ECMAScript 4 (quote is heavily snipped):

    > What jumps out at me is the potential for developers to assume that a> hashcode is the same as a unique ID (especially if some> implementations enforce uniqueness while others don't), thus leading> to subtle bugs. For that reason, I'd like to weigh in with the> opinion that requiring uniqueness for hashcodes would be really> nice. (Alternatively a separate mechanism for generating unique> object IDs would be nice.)<X>If an implementation is going to guarantee uniqueness then it probablyneeds to maintain a global table of all live objects whose hash codeshave been obtained, to avoid generating duplicate codes when theglobal hash code counter wraps around (intrinsic::hashcode returns auint).<X>IMO we'd be better off following Java here and just spell out (more)clearly that you can't use the hash code as an object ID.

    Posted to ES4-discuss by Lars Thomas Hansen (sorry, listmembers-only archive but AFAIK anyone can join the list.)

  3. How do we detect if decompilation is disabled? What does Function.prototype.toString() return instead? Does it error?

  4. hallvors explained this in bug report:Originally posted by hallvors:

    Decompilation support is an optional feature of ECMAScript. Opera disables it on limited platforms (mobiles, devices) for performance. When the output of Function.prototype.toString() is "[ecmascript code]" this is the case.

  5. Dean, good question – because I don't think we've been completely consistent in the past. It never throws an error but I seem to remember that some versions have returned the mixed case string "[ECMAScript code]" and some the all-lower case form..

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s