Fixed properties defined on prototype in Safari background page proxy

safari/content.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 10 matching lines @@
 
 
   /* Background page proxy */
   var proxy = {
     objects: [],
     callbacks: [],
 
     send: function(message)
     {
       var evt = document.createEvent("Event");
       evt.initEvent("beforeload");

[Wladimir Palant, 2013/11/21 08:15:58]

Can we reuse the same event instead of creating a new one every time?

[Sebastian Noack, 2014/02/26 11:27:19]

This was addressed by http://codereview.adblockplus.org/5464830253203456/

       return safari.self.tab.canLoad(evt, {type: "proxy", payload: message});
     },
     checkResult: function(result)
     {
       if (!result.succeed)
         throw result.error;
     },
     deserializeResult: function(result)
     {
       this.checkResult(result);
       return this.deserialize(result.result);
     },
     serialize: function(obj, memo)
     {
       var objectId = this.objects.indexOf(obj);

[Wladimir Palant, 2013/11/21 08:15:58]

this.getObjectId() should be used here.

[Sebastian Noack, 2014/02/26 11:27:19]

getObjectId() is gone now.

       if (objectId != -1)
         return {type: "hosted", objectId: objectId};
 
       if (typeof obj == "function")
       {
         var callbackId = this.callbacks.indexOf(obj);

[Wladimir Palant, 2013/11/21 08:15:58]

As with objects, it is better to put the callback ID into an expando property of
the callback rather than going through the array every time.

[Sebastian Noack, 2013/11/21 21:17:08]

But this would make the ID visible externally and might theoretically clash with
other properties. I would prefer to keep it hidden from the calling code.

[Wladimir Palant, 2014/01/15 16:21:01]

It's not like we are messing with some website here - this is our own code, our
own properties. If you are afraid of naming conflicts you can use
$safariCompat$callbackId as property name. However, looping through arrays to
find object/callback IDs isn't really an option as it will degrade performance
severely. This will become especially visible if we expose individual filters in
the preferences UI - we will be dealing with thousands of objects then.

[Sebastian Noack, 2014/02/26 11:27:19]

Done.

 
         if (callbackId == -1)
         {
           callbackId = this.callbacks.push(obj) - 1;
 
           safari.self.addEventListener("message", function(event)
           {
             if (event.name == "proxyCallback")
             if (event.message.callbackId == callbackId)

[Wladimir Palant, 2013/11/21 08:15:58]

You should use && here and put brackets around the body of the block.

[Sebastian Noack, 2014/02/26 11:27:19]

This was addressed by http://codereview.adblockplus.org/5464830253203456/

               obj.apply(
                 this.getObject(event.message.contextId),
                 this.deserializeSequence(event.message.args)
               );
           }.bind(this));

[Wladimir Palant, 2013/11/21 08:15:58]

Why add a new listener for each single callback? There should be only one
listener:

function(event)
{
  if (event.name == "proxyCallback")
  {
    var callback = this.callbacks[event.message.callbackId];
    callback.apply(
      this.getObject(event.message.contextId),
      this.deserializeSequence(event.message.args)
    );
  }
}

This listener can be added when the first callback is created.

[Sebastian Noack, 2014/02/26 11:27:19]

This was addressed by http://codereview.adblockplus.org/5464830253203456/

         }
 
         return {type: "callback", callbackId: callbackId};
       }
 
       if (typeof obj == "object" &&
           obj != null &&
           obj.constructor != Date &&
           obj.constructor != RegExp)
       {
@@ skipping 59 matching lines @@
           if (idx != -1)
             return memo.arrays[idx];
 
           var array = [];
           memo.specs.push(spec);
           memo.arrays.push(array);
 
           return this.deserializeSequence(spec.items, array, memo);
       }
     },
+    getObjectId: function(obj)
+    {
+      return this.objects.indexOf(obj);

[Wladimir Palant, 2013/11/21 08:15:58]

Why iterate through the entire array when we can cache the object ID as an
object property (e.g. __objectID, not enumerable) when we are creating the
object?

[Sebastian Noack, 2014/02/26 11:27:19]

Done.

+    },
     getProperty: function(objectId, property)
     {
       return this.deserializeResult(
         this.send(
         {
           type: "getProperty",
           objectId: objectId,
           property: property
         })
       );
     },
-    createProperty: function(objectId, property, enumerable)
+    createProperty: function(property, enumerable)
     {
+      var proxy = this;
       return {
         get: function()
         {
-          return this.getProperty(objectId, property);
-        }.bind(this),
+          return proxy.getProperty(proxy.getObjectId(this), property);
+        },
         set: function(value)
         {
-          this.checkResult(
-            this.send(
+          proxy.checkResult(
+            proxy.send(
             {
               type: "setProperty",
-              objectId: objectId,
+              objectId: proxy.getObjectId(this),
               property: property,
-              value: this.serialize(value)
+              value: proxy.serialize(value)
             })
           );
-        }.bind(this),
+        },
         enumerable: enumerable,
         configurable: true
       };
     },
     createFunction: function(objectId)
     {
       var proxy = this;
       return function()
       {
         return proxy.deserializeResult(
           proxy.send(
           {
             type: "callFunction",
             functionId: objectId,
-            contextId: proxy.objects.indexOf(this),
+            contextId: proxy.getObjectId(this),
             args: Array.prototype.map.call(
               arguments,
               proxy.serialize.bind(proxy)
             )
           })
         );
       };
     },
     getObject: function(objectId) {

[Sebastian Noack, 2014/02/26 11:27:19]

This was addressed by http://codereview.adblockplus.org/5464830253203456/

       var objectInfo = this.send({
         type: "inspectObject",
         objectId: objectId
       });
 
       var obj = this.objects[objectId];
       if (obj)

[Wladimir Palant, 2013/11/21 08:15:58]

Nit: I generally dislike blindly requesting properties that might not exist.
This should really be |if (objectId in this.objects)|.

[Sebastian Noack, 2013/11/21 21:17:08]

First using the in opreator and then getting the actual property from the
object, duplicates the logic, and since the lookup is done twice it adds an
unneeded overhead.

[Wladimir Palant, 2014/01/15 16:21:01]

Very Python-like logic. Except that property lookups are generally cached so the
performance difference should be negligible even if this were a hot spot.

if (foo.bar) is not quite the same as if ("bar" in foo) as you certainly know. I
prefer to show clearly that we are checking existence here, not the value - and
this doesn't really count as "duplicating logic". But I will accept this as is
if you insist.

[Sebastian Noack, 2014/02/26 11:27:19]

I've benchmarked this, and on Firefox using the in operator before the actual
lookup seems to be even faster. But on Chrome and Safari (where this code is
used) it is faster if you skip the in operator check and do only one lookup.
http://jsperf.com/in-operator-performance
Yes, I know but since we have only objects here, which always evaluate to true,
it doesn't make a difference.

         Object.getOwnPropertyNames(obj).forEach(function(prop) { delete obj[prop]; });
       else
       {
         if (objectInfo.isFunction)
           obj = this.createFunction(objectId);
         else
           obj = {};
 
         this.objects[objectId] = obj;
       }
 
       var ignored = [];
       if ("prototypeOf" in objectInfo)
       {
         var prototype = window[objectInfo.prototypeOf].prototype;
 
         ignored = Object.getOwnPropertyNames(prototype);
         ignored.splice(ignored.indexOf("constructor"), 1);

[Wladimir Palant, 2013/11/21 08:15:58]

I think there is something conceptually wrong here. Why is the sender sending
all properties rather than own properties only? ignored shouldn't normally be
necessary.

[Sebastian Noack, 2013/11/21 21:17:08]

Only own properties are sent. This code only handles the special case of
Object.prototype and Function.prototype. The proxy objects for Object.prototype
and Function.prototype are going to inherit from the builtin Object.prototype
and Function.prototype and we must not override functions like toString(),
__lookupGetter__(), etc with proxy functions. However we want to proxy the
constructor (in order that "bg.someObj instanceof bg.Object == true"), and we
also want to proxy properties that were monkey-patched to Object.prototype or
Function.prototype in the bg page, but not in our current execution environment.

 
         obj.__proto__ = prototype;
       }
       else
       {
         if (objectInfo.isFunction)
           ignored = Object.getOwnPropertyNames(function() {});
         else
           ignored = [];
 
         if ("prototypeId" in objectInfo)
           obj.__proto__ = this.getObject(objectInfo.prototypeId);
         else
           obj.__proto__ = null;

[Wladimir Palant, 2013/11/21 08:15:58]

Note that __proto__ is deprecated. It would be better to use Object.create()
here to create the object with a specific prototype. Object.getPrototypeOf() can
be used on the sender's side.

We use __proto__ in our Firefox code because that code is older than the two
functions mentioned above.

       }
 
       for (var property in objectInfo.properties)
         if (ignored.indexOf(property) == -1)
           Object.defineProperty(obj, property, this.createProperty(
-            objectId, property,
-            objectInfo.properties[property].enumerable
+            property, objectInfo.properties[property].enumerable
           ));

[Wladimir Palant, 2013/11/21 08:15:58]

Style nit: please always put brackets around multi-line blocks, even if
syntactically they aren't required.

[Sebastian Noack, 2014/02/26 11:27:19]

Done.

 
       if (objectInfo.isFunction)
         obj.prototype = this.getProperty(objectId, "prototype");

[Wladimir Palant, 2013/11/21 08:15:58]

I'm not sure whether this special case is the best solution. The loop above
could instead check whether the property already exists and is non-configurable
- the property value should be set then rather than defining the property.

[Sebastian Noack, 2014/02/26 11:27:19]

Done.

 
       return obj;
     }
   };
 
 
   /* Web request blocking */
 
   document.addEventListener("beforeload", function(event)
   {
     var type;
 
     switch(event.target.nodeName)

[Wladimir Palant, 2013/11/21 08:15:58]

Please use localName and check for lower-case tag names below - then this will
work properly with XHTML documents as well.

[Sebastian Noack, 2014/02/26 11:27:19]

This was addressed by http://codereview.adblockplus.org/4526994515558400/

     {
       case "FRAME":
       case "IFRAME":
         type = "frame";
         break;
       case "IMG":
         type = "image";
         break;
       case "OBJECT":
       case "EMBED":
         type = "object";
         break;
       case "SCRIPT":
         type = "script";
         break;
       case "LINK":
         if (/(^|\s)stylesheet($|\s)/i.test(event.target.rel))

[Wladimir Palant, 2013/11/21 08:15:58]

/\bstylesheet\b/i should do here.

[Sebastian Noack, 2013/11/21 21:17:08]

There is no \b in regular expressions in JS (at least in Webkit).

[Sebastian Noack, 2013/11/27 17:43:16]

I'm wrong. Nevermind.

[Sebastian Noack, 2014/02/26 11:27:19]

This was addressed by http://codereview.adblockplus.org/4526994515558400/

         {
           type = "stylesheet";
           break;
         }
       default:
         type = "other";
     }
 
     if (!safari.self.tab.canLoad(event, {type: "webRequest", payload: {url: event.url, type: type}}))
       event.preventDefault();
   }, true);
 
 
   /* API */
 
   ext.backgroundPage = {
     _eventTarget: safari.self,
     _messageDispatcher: safari.self.tab,
 
     sendMessage: sendMessage,
     getWindow: function() { return proxy.getObject(0); }
   };
 
   ext.onMessage = new MessageEventTarget(safari.self);
 })();