Remove chrome.* Function Instrumentation Inlining and Use a Separate Instrumented Object

chrome/browser/resources/google_now/utility.js

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
 /**
  * @fileoverview Utility objects and functions for Google Now extension.
  */
 
@@ skipping 37 matching lines @@
 function buildServerRequest(handlerName, contentType) {
   var request = new XMLHttpRequest();
 
   request.responseType = 'text';
   request.open('POST', NOTIFICATION_CARDS_URL + '/' + handlerName, true);
   request.setRequestHeader('Content-type', contentType);
 
   return request;
 }
 
+// Partial mirror of chrome.* for all instrumented functions.
+var instrumented = {};
+
 /**
  * Builds the object to manage tasks (mutually exclusive chains of events).
  * @param {function(string, string): boolean} areConflicting Function that
  *     checks if a new task can't be added to a task queue that contains an
  *     existing task.
  * @return {Object} Task manager interface.
  */
 function buildTaskManager(areConflicting) {
   /**
    * Queue of scheduled tasks. The first element, if present, corresponds to the
@@ skipping 223 matching lines @@
           });
           debugAlert(message);
         }
       }
     };
   }
 
   /**
    * Instruments an API function to add error processing to its user
    * code-provided callback.
-   * @param {Object} namespace Namespace of the API function.
-   * @param {string} functionName Name of the API function.
+   * @param {string} functionIdentifier Full identifier of the function without
+   *     the 'chrome.' portion.
    * @param {number} callbackParameter Index of the callback parameter to this
    *     API function.
    */
-  function instrumentApiFunction(namespace, functionName, callbackParameter) {
-    var originalFunction = namespace[functionName];
+  function instrumentChromeApiFunction(functionIdentifier, callbackParameter) {
+    var functionIdentifierParts = functionIdentifier.split('.');
+    var functionName = functionIdentifierParts.pop();
+    var functioncontainer = chrome;

[vadimt, 2013/08/08 01:15:10]

chromeContainer?

[robliao, 2013/08/08 17:43:31]

Done.

+    var instrumentedcontainer = instrumented;

[vadimt, 2013/08/08 01:15:10]

camelCased

[robliao, 2013/08/08 17:43:31]

Done.

+    functionIdentifierParts.map(function(fragment) {
+      functioncontainer = functioncontainer[fragment];
+      if (!(fragment in instrumentedcontainer))
+        instrumentedcontainer[fragment] = {};
 
-    if (!originalFunction)
+      instrumentedcontainer = instrumentedcontainer[fragment];
+    });
+
+    var targetFunction = functioncontainer[functionName];
+
+    if (!targetFunction)
       debugAlert('Cannot instrument ' + functionName);
 
-    namespace[functionName] = function() {
+    // After we've verified that the target is indeed a function, throw
+    // it away so that we can't ever use it again.
+    // Chrome can rebind the chrome.* APIs, which may invalidate
+    // any cached function we have.
+    targetFunction = null;

[vadimt, 2013/08/08 01:15:10]

It could be better to move the above piece of code into a separate function
where targetFunction would be a local variable. This would also solve the
problem of accidentally using it in the callback and the problem of the growing
side of instrumentChromeApiFunction.

[robliao, 2013/08/08 17:43:31]

Done.

+
+    instrumentedcontainer[functionName] = function() {
       // This is the wrapper for the API function. Pass the wrapped callback to
       // the original function.
       var callback = arguments[callbackParameter];
       if (typeof callback != 'function') {
         debugAlert('Argument ' + callbackParameter + ' of ' + functionName +

[vadimt, 2013/08/08 01:15:10]

functionIdentifier would give more information than functionName.

[robliao, 2013/08/08 17:43:31]

Done.

               ' is not a function');
       }
       arguments[callbackParameter] = wrapCallback(
           callback, functionName == 'addListener');
-      return originalFunction.apply(namespace, arguments);
+
+      var targetFunctionContainer = chrome;
+      functionIdentifierParts.map(function(fragment) {
+        targetFunctionContainer = targetFunctionContainer[fragment];
+      });
+      return targetFunctionContainer[functionName].
+          apply(targetFunctionContainer, arguments);
     };
   }
 
-  instrumentApiFunction(chrome.alarms, 'get', 1);
-  instrumentApiFunction(chrome.alarms.onAlarm, 'addListener', 0);
-  instrumentApiFunction(chrome.identity, 'getAuthToken', 1);
-  instrumentApiFunction(chrome.runtime.onSuspend, 'addListener', 0);
+  instrumentChromeApiFunction('alarms.get', 1);
+  instrumentChromeApiFunction('alarms.onAlarm.addListener', 0);
+  instrumentChromeApiFunction('identity.getAuthToken', 1);
+  instrumentChromeApiFunction('identity.removeCachedAuthToken', 1);
+  instrumentChromeApiFunction('runtime.onSuspend.addListener', 0);
 
   chrome.runtime.onSuspend.addListener(function() {
     var stringifiedPendingCallbacks = JSON.stringify(pendingCallbacks);
     verify(
         queue.length == 0 && stringifiedPendingCallbacks == '{}',
         'Incomplete task or pending callbacks when unloading event page,' +
         ' queue = ' + JSON.stringify(queue) +
         ', pendingCallbacks = ' + stringifiedPendingCallbacks);
   });
 
   return {
     add: add,
     debugSetStepName: function() {},  // TODO(vadimt): remove
-    instrumentApiFunction: instrumentApiFunction,
+    instrumentChromeApiFunction: instrumentChromeApiFunction,
     wrapCallback: wrapCallback
   };
 }
 
-var storage = chrome.storage.local;

[vadimt, 2013/08/08 01:15:10]

Why not  var storage = instrumented.storage.local;
?

[robliao, 2013/08/08 17:43:31]

instrumented.storage.local doesn't exist until after buildTaskManager.

We'd have to instantiate buildTaskManager here for this to work.
On 2013/08/08 01:15:10, vadimt wrote:

[vadimt, 2013/08/08 18:19:25]

I's always a good idea to instrument stuff before the code that uses it. Treat
this as 'using' directive.
Let's instrument storage before the task manager, and define 'storage' variable
in the next line.

[robliao, 2013/08/08 21:35:35]

Per discussion, I am going to leave things as is. Having a direct
chrome.storage.local.set/get allows for the bifurcation of storage to
instrumented and noninstrumented parts as well as keeping the calls simple. 

On 2013/08/08 18:19:25, vadimt wrote:

-
 /**
  * Builds an object to manage retrying activities with exponential backoff.
  * @param {string} name Name of this attempt manager.
  * @param {function()} attempt Activity that the manager retries until it
  *     calls 'stop' method.
  * @param {number} initialDelaySeconds Default first delay until first retry.
  * @param {number} maximumDelaySeconds Maximum delay between retries.
  * @return {Object} Attempt manager interface.
  */
 function buildAttemptManager(
@@ skipping 13 matching lines @@
     };
     chrome.alarms.create(alarmName, alarmInfo);
   }
 
   /**
    * Indicates if this attempt manager has started.
    * @param {function(boolean)} callback The function's boolean parameter is
    *     true if the attempt manager has started, false otherwise.
    */
   function isRunning(callback) {
-    chrome.alarms.get(alarmName, function(alarmInfo) {
+    instrumented.alarms.get(alarmName, function(alarmInfo) {
       callback(!!alarmInfo);
     });
   }
 
   /**
    * Schedules next attempt.
    * @param {number=} opt_previousDelaySeconds Previous delay in a sequence of
    *     retry attempts, if specified. Not specified for scheduling first retry
    *     in the exponential sequence.
    */
   function scheduleNextAttempt(opt_previousDelaySeconds) {
     var base = opt_previousDelaySeconds ? opt_previousDelaySeconds * 2 :
                                           initialDelaySeconds;
     var newRetryDelaySeconds =
         Math.min(base * (1 + 0.2 * Math.random()), maximumDelaySeconds);
 
     createAlarm(newRetryDelaySeconds);
 
     var items = {};
     items[currentDelayStorageKey] = newRetryDelaySeconds;
-    storage.set(items);
+    chrome.storage.local.set(items);
   }
 
   /**
    * Starts repeated attempts.
    * @param {number=} opt_firstDelaySeconds Time until the first attempt, if
    *     specified. Otherwise, initialDelaySeconds will be used for the first
    *     attempt.
    */
   function start(opt_firstDelaySeconds) {
     if (opt_firstDelaySeconds) {
       createAlarm(opt_firstDelaySeconds);
-      storage.remove(currentDelayStorageKey);
+      chrome.storage.local.remove(currentDelayStorageKey);
     } else {
       scheduleNextAttempt();
     }
   }
 
   /**
    * Stops repeated attempts.
    */
   function stop() {
     chrome.alarms.clear(alarmName);
-    storage.remove(currentDelayStorageKey);
+    chrome.storage.local.remove(currentDelayStorageKey);
   }
 
   /**
    * Plans for the next attempt.
    * @param {function()} callback Completion callback. It will be invoked after
    *     the planning is done.
    */
   function planForNext(callback) {
-    storage.get(currentDelayStorageKey, function(items) {
+    instrumented.storage.local.get(currentDelayStorageKey, function(items) {
       console.log('planForNext-get-storage ' + JSON.stringify(items));
       scheduleNextAttempt(items[currentDelayStorageKey]);
       callback();
     });
   }
 
-  chrome.alarms.onAlarm.addListener(function(alarm) {
+  instrumented.alarms.onAlarm.addListener(function(alarm) {
     if (alarm.name == alarmName)
       isRunning(function(running) {
         if (running)
           attempt();
       });
   });
 
   return {
     start: start,
     planForNext: planForNext,
@@ skipping 13 matching lines @@
  */
 function buildAuthenticationManager() {
   var alarmName = 'sign-in-alarm';
 
   /**
    * Determines if the user is signed in and provides a token if signed in.
    * @param {function(string=)} callback Called on completion.
    *     If the user is signed in, the string contains the token.
    */
   function isSignedIn(callback) {
-    chrome.identity.getAuthToken({interactive: false}, function(token) {
+    instrumented.identity.getAuthToken({interactive: false}, function(token) {
       token = chrome.runtime.lastError ? undefined : token;
       callback(token);
       checkAndNotifyListeners(!!token);
     });
   }
 
   /**
    * Removes the specified cached token.
    * @param {string} token Authentication Token to remove from the cache.
    * @param {function} onSuccess Called on completion.
    */
   function removeToken(token, onSuccess) {
-    chrome.identity.removeCachedAuthToken({token: token}, function() {
+    instrumented.identity.removeCachedAuthToken({token: token}, function() {
       // Removing the token from the cache will change the sign in state.
       // Repoll now to check the state and notify listeners.
       // This also lets Chrome now about a possible problem with the token.
       isSignedIn(function() {});
       onSuccess();
     });
   }
 
   var listeners = [];
 
@@ skipping 14 matching lines @@
   function checkAndNotifyListeners(currentSignedInState) {
     if ((lastReturnedSignedInState !== currentSignedInState) &&
         (lastReturnedSignedInState !== null)) {
       for (var listenerIndex in listeners) {
         listeners[listenerIndex]();
       }
     }
     lastReturnedSignedInState = currentSignedInState;
   }
 
-  chrome.alarms.onAlarm.addListener(function(alarm) {
+  instrumented.alarms.onAlarm.addListener(function(alarm) {
     if (alarm.name == alarmName)
       isSignedIn(function() {});
   });
 
   // Poll for the sign in state every hour.
   // One hour is just an arbitrary amount of time chosen.
   chrome.alarms.create(alarmName, {periodInMinutes: 60});
 
   return {
     addListener: addListener,
     isSignedIn: isSignedIn,
     removeToken: removeToken
   };
 }