Simplifying using tasks

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';
 
 // TODO(vadimt): Remove alerts.
 
 /**
  * @fileoverview Utility objects and functions for Google Now extension.
@@ skipping 43 matching lines @@
  */
 function buildTaskManager(areConflicting) {
   /**
    * Queue of scheduled tasks. The first element, if present, corresponds to the
    * currently running task.
    * @type {Array.<Object.<string, function(function())>>}
    */
   var queue = [];
 
   /**
-   * Name of the current step of the currently running task if present,
-   * otherwise, null. For diagnostics only.
-   * It's set when the task is started and before each asynchronous operation.
+   * Count of unfinished callbacks of the current task.
+   * @type {number}
    */
-  var stepName = null;
+  var taskPendingCallbackCount = 0;
+
+  /**
+   * Required callbacks that are not yet called. Includes both task and non-task
+   * callbacks. This is a map from unique callback id to the stack at the moment
+   * when the callback was wrapped. This stack identifies the callback.
+   * Used only for diagnostics.
+   * @type {Object.<number, string>}
+   */
+  var pendingCallbacks = {};
+
+  /**
+   * True if currently executed code is a part of a task.
+   * @type {boolean}
+   */
+  var isInTask = false;

[skare_, 2013/07/23 21:10:52]

maybe "Whether a task is currently running?"

"currently executed code" might not be authoritative (e.g. a task makes an async
call and execution goes to another event in the queue)

[vadimt, 2013/07/23 22:35:51]

"Whether a task is currently running" may be confusing. It may imply that it's
true starting from a moment when a task was started, finishing at the moment
when the task is done.
But it should NOT be true for non-task callback that get executed between task's
start and end. It should be true only when executing task callbacks (and the
task bootstrap passed to start() method). This is why I chose this name.

 
   /**
    * Starts the first queued task.
    */
   function startFirst() {
     verify(queue.length >= 1, 'startFirst: queue is empty');
+    verify(!isInTask, 'startFirst: already in task');
+    isInTask = true;
 
     // Start the oldest queued task, but don't remove it from the queue.
     verify(
-        stepName == null,
-        'tasks.startFirst: stepName is not null: ' + stepName +
-        ', queue = ' + JSON.stringify(queue));
+        taskPendingCallbackCount == 0,
+        'tasks.startFirst: still have pending task callbacks: ' +
+        taskPendingCallbackCount +
+        ', queue = ' + JSON.stringify(queue) +
+        ', pendingCallbacks = ' + JSON.stringify(pendingCallbacks));
     var entry = queue[0];
-    stepName = entry.name + '-initial';
     console.log('Starting task ' + entry.name);
-    entry.task(finish);
+
+    entry.task(function() {});  // TODO(vadimt): Don't pass parameter.
+
+    verify(isInTask, 'startFirst: not in task at exit');
+    isInTask = false;
+    if (taskPendingCallbackCount == 0)
+      finish();
   }
 
   /**
    * Checks if a new task can be added to the task queue.
    * @param {string} taskName Name of the new task.
    * @return {boolean} Whether the new task can be added.
    */
   function canQueue(taskName) {
     for (var i = 0; i < queue.length; ++i) {
       if (areConflicting(taskName, queue[i].name)) {
@@ skipping 24 matching lines @@
     if (queue.length == 1) {
       startFirst();
     }
   }
 
   /**
    * Completes the current task and starts the next queued task if available.
    */
   function finish() {
     verify(queue.length >= 1,
-           'tasks.finish: The task queue is empty; step = ' + stepName);
+           'tasks.finish: The task queue is empty');
     console.log('Finishing task ' + queue[0].name);
     queue.shift();
-    stepName = null;
 
     if (queue.length >= 1)
       startFirst();
   }
 
-  /**
-   * Associates a name with the current step of the task. Used for diagnostics
-   * only. A task is a chain of asynchronous events; debugSetStepName should be
-   * called before starting any asynchronous operation.
-   * @param {string} step Name of new step.
-   */
-  function debugSetStepName(step) {
-    stepName = step;
-  }
-
   // Limiting 1 error report per background page load.
   var errorReported = false;
 
   /**
    * Sends an error report to the server.
    * @param {Error} error Error to report.
    */
   function sendErrorReport(error) {
     var filteredStack = error.stack.replace(/.*\n/, '\n');
     var file;
@@ skipping 22 matching lines @@
         '&trace=' + encodeURIComponent(filteredStack);
     var request = buildServerRequest('jserror',
                                      'application/x-www-form-urlencoded');
     request.onloadend = function(event) {
       console.log('sendErrorReport status: ' + request.status);
     };
     request.send(requestParameters);
   }
 
   /**
+   * Unique ID of the next callback.
+   * @type {number}
+   */
+  var nextCallbackId = 0;
+
+  /**
    * Adds error processing to an API callback.
    * @param {Function} callback Callback to instrument.
+   * @param {boolean=} opt_dontRequire True if the callback is not required to

[skare_, 2013/07/23 21:10:52]

more comments or logic flip could help here, just IMO from my reading since this
seems slightly confusing (true to not enforce some requirement). Mostly my first
question on reading the comments was "what is 'not required to be invoked'?"

[vadimt, 2013/07/23 22:35:51]

You may call this method without passing this param. I wanted 'undefined' to
mean 'required'. Hence the "don't" part.

+   *     be invoked.
    * @return {Function} Instrumented callback.
    */
-  function wrapCallback(callback) {
+  function wrapCallback(callback, opt_dontRequire) {
+    verify(!(opt_dontRequire && isInTask), 'Unrequired callback in a task.');
+    var callbackId = nextCallbackId++;
+    var isTaskCallback = isInTask;
+    if (isInTask)
+      ++taskPendingCallbackCount;
+    if (!opt_dontRequire)
+      pendingCallbacks[callbackId] = new Error().stack;
+
     return function() {
       // This is the wrapper for the callback.
       try {
-        return callback.apply(null, arguments);
+        if (isTaskCallback) {
+          verify(!isInTask, 'wrapCallback: already in task');
+          isInTask = true;
+        }
+        if (!opt_dontRequire)
+          delete pendingCallbacks[callbackId];
+
+        // Call the original callback.
+        callback.apply(null, arguments);
+
+        if (isTaskCallback) {
+          verify(isInTask, 'wrapCallback: not in task at exit');
+          isInTask = false;
+          if (--taskPendingCallbackCount == 0)
+            finish();
+        }
       } catch (error) {
         var message = 'Uncaught exception:\n' + error.stack;
         console.error(message);
         if (!errorReported) {
           errorReported = true;
           chrome.metricsPrivate.getIsCrashReportingEnabled(function(isEnabled) {
             if (isEnabled)
               sendErrorReport(error);
           });
           alert(message);
@@ skipping 17 matching lines @@
       alert('Cannot instrument ' + functionName);
 
     namespace[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') {
         alert('Argument ' + callbackParameter + ' of ' + functionName +
               ' is not a function');
       }
-      arguments[callbackParameter] = wrapCallback(callback);
+      arguments[callbackParameter] = wrapCallback(
+          callback, functionName == 'addListener');
       return originalFunction.apply(namespace, arguments);
     };
   }
 
   instrumentApiFunction(chrome.alarms.onAlarm, 'addListener', 0);
   instrumentApiFunction(chrome.runtime.onSuspend, 'addListener', 0);
 
   chrome.runtime.onSuspend.addListener(function() {
+    var stringifiedPendingCallbacks = JSON.stringify(pendingCallbacks);
     verify(
-        queue.length == 0,
-        'Incomplete task when unloading event page, queue = ' +
-        JSON.stringify(queue) + ', step = ' + stepName);
-    verify(
-        stepName == null,
-        'Step name not null when unloading event page, queue = ' +
-        JSON.stringify(queue) + ', step = ' + stepName);
+        queue.length == 0 && stringifiedPendingCallbacks == '{}',
+        'Incomplete task or pending callbacks when unloading event page,' +
+        ' queue = ' + JSON.stringify(queue) +
+        ', pendingCallbacks = ' + stringifiedPendingCallbacks);
   });
 
   return {
     add: add,
-    // TODO(vadimt): Replace with instrumenting callbacks.
-    debugSetStepName: debugSetStepName,
+    debugSetStepName: function() {},  // TODO(vadimt): remove
     instrumentApiFunction: instrumentApiFunction,
     wrapCallback: wrapCallback
   };
 }
 
 var storage = chrome.storage.local;
 
 /**
  * Builds an object to manage retrying activities with exponential backoff.
  * @param {string} name Name of this attempt manager.
@@ skipping 62 matching lines @@
     chrome.alarms.clear(alarmName);
     storage.remove(currentDelayStorageKey);
   }
 
   /**
    * Plans for the next attempt.
    * @param {function()} callback Completion callback. It will be invoked after
    *     the planning is done.
    */
   function planForNext(callback) {
     tasks.debugSetStepName('planForNext-get-storage');

[rgustafson, 2013/07/24 19:53:02]

Can we remove from here right now? Or is this also being edited right now?

[vadimt, 2013/07/24 20:01:00]

Done.

     storage.get(currentDelayStorageKey, function(items) {
       console.log('planForNext-get-storage ' + JSON.stringify(items));
       scheduleNextAttempt(items[currentDelayStorageKey]);
       callback();
     });
   }
 
   chrome.alarms.onAlarm.addListener(function(alarm) {
     if (alarm.name == alarmName)
       attempt();
   });
 
   return {
     start: start,
     planForNext: planForNext,
     stop: stop
   };
 }