Preventing race conditions in Google Now extension

chrome/browser/resources/google_now/utility.js

Index: chrome/browser/resources/google_now/utility.js
diff --git a/chrome/browser/resources/google_now/utility.js b/chrome/browser/resources/google_now/utility.js
new file mode 100644
index 0000000000000000000000000000000000000000..a4f39e58ce2365d4f2a54b1407b75c8157892bba
--- /dev/null
+++ b/chrome/browser/resources/google_now/utility.js
@@ -0,0 +1,155 @@
+// 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.
+ */
+
+/**
+ * Checks for internal errors.
+ * @param {boolean} condition Condition that must be true.
+ * @param {string} message Diagnostic message for the case when the condition is
+ *     false.
+ */
+function verify(condition, message) {
+  // TODO(vadimt): Send UMAs instead of showing alert.
+  // TODO(vadimt): Make sure the execution doesn't continue after this call.
+  if (!condition) {
+    var errorText = 'ASSERT: ' + message;
+    console.error(errorText);
+    alert(errorText);

[rgustafson, 2013/03/14 19:05:20]

I've become worried about this alert for a different reason. Some browsers block
the currently executing code on alert, but continue to run event handlers if an
event comes back. I haven't been able to find out if Chrome does this, but it
could create stranger bugs.

[vadimt, 2013/03/14 22:56:05]

In most cases,the second alert won't happen because the current task is still
running, and the other task won't start.
But even if this happens, the new alert won't be shown till the first one is
closed.
Showing a prominent error dialog is so useful that I'd suggest living with it
for a while.

+  }
+}
+
+/**
+ * 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} Set of methods to manage tasks.
+ */
+function TaskManager(areConflicting) {
+  /**
+   * Name of the alarm that triggers the error saying that the event page cannot
+   * unload.
+   */
+  var CANNOT_UNLOAD_ALARM_NAME = 'CANNOT-UNLOAD';
+
+  /**
+   * Maximal time we expect the event page to stay loaded after starting a task.
+   */
+  var MAXIMUM_LOADED_TIME_MINUTES = 5;
+
+  /**
+   * Queue of scheduled tasks. The first element, if present, corresponds to the
+   * currently running task.
+   * @type {Array.<Object.<string, 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.
+   */
+  var stepName = null;
+
+  /**
+   * Starts the first queued task.
+   */
+  function startFirst() {
+    verify(queue.length >= 1, 'startFirst: queue is empty');
+
+    // Set alarm to verify that the event page will unload in a reasonable time.
+    chrome.alarms.create(CANNOT_UNLOAD_ALARM_NAME,
+                         {delayInMinutes: MAXIMUM_LOADED_TIME_MINUTES});
+
+    // Starts the oldest queued task, but doesn't remove it from the queue.
+    verify(stepName == null, 'tasks.startFirst: stepName is not null');
+    var entry = queue[0];
+    stepName = entry.name + '-initial';
+    entry.task();
+  }
+
+  /**
+   * 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))
+        return false;
+
+    return true;
+  }
+
+  /**
+   * Adds a new task. If another task is not running, run the task immediately.
+   * If any task in the queue is not compatible with the task, ignore the new
+   * task. Otherwise, store the task for future execution.
+   * @param {string} taskName Name of the task.
+   * @param {function()} task Code of the task.
+   */
+  function submit(taskName, task) {
+    if (!canQueue(taskName))
+      return;
+
+    queue.push({name: taskName, task: task});
+
+    if (queue.length == 1) {
+      startFirst();
+    }
+  }
+
+  /**
+   * Completes the current task and start the next queued task if available.
+   */

[skare_, 2013/03/14 01:19:53]

currently clients need to be aware of this function:

function someTask() {
   doSomethingAsyncy(function() {
     tasks.finish();
   });
}

which I believe
-makes everything that finishes a chain of functions need to know about the task
manager.
-requires that code that runs in a task is only for use in a Task.

could you instead use callbacks and eliminate .finish() from the API? e.g.
client code would look like

function someTask(callback) {
   doSomethingAsyncy(function() {
     callback();
   });
}

Then in the task manager, you can do something like

function TaskManager() {
 [...]

  function runNext() {
    var task = queue[0];
    task(function(err) {
      if (err)
        handle(err);
      finish();
    });
  }

} // TaskManager

callback() can often be of the form
callback(err, optionalParamsAndStuff);
where the first arg 'err' is undefined/null on success.


also, if you don't need to do anything special there, finish can be your actual
callback.

runNext() {
  var task = queue[0];
  task(finish);
}

[vadimt, 2013/03/14 22:56:05]

Done.

+  function finish() {
+    verify(queue.length >= 1, 'tasks.finish: The task queue is empty.');
+    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) {
+    // TODO(vadimt): Pass UMA counters instead of step names.
+    stepName = step;
+  }
+
+  chrome.alarms.onAlarm.addListener(function(alarm) {
+    if (alarm.name == CANNOT_UNLOAD_ALARM_NAME) {
+      // Error if the event page wasn't unloaded after a reasonable timeout
+      // since starting the last task.
+      // TODO(vadimt): Uncomment the verify once this bug is fixed:
+      // crbug.com/177563
+      // verify(false, 'Event page didn\'t unload, queue = ' +
+      // JSON.stringify(tasks) + ', step = ' + stepName + ' (ignore this verify
+      // if devtools is attached).');
+    }
+  });
+
+  chrome.runtime.onSuspend.addListener(function() {
+    chrome.alarms.clear(CANNOT_UNLOAD_ALARM_NAME);
+    verify(queue.length == 0 && stepName == null,
+      'Incomplete task when unloading event page, queue = ' +
+      JSON.stringify(queue) + ', step = ' + stepName);
+  });
+
+  return {
+    submit: submit,
+    debugSetStepName: debugSetStepName,
+    finish: finish
+  };
+}