Preventing race conditions in Google Now extension

chrome/browser/resources/google_now/background.js

Index: chrome/browser/resources/google_now/background.js
diff --git a/chrome/browser/resources/google_now/background.js b/chrome/browser/resources/google_now/background.js
index a65618cea0d01c4655826286adba2907229bd212..33bfcee128a96c809bc946211fe1c93a2943263d 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -25,9 +25,10 @@
 // TODO(vadimt): Report errors to the user.
 
 // TODO(vadimt): Figure out the server name. Use it in the manifest and for
-// TODO(vadimt): Consider processing errors for all storage.set calls.
 // NOTIFICATION_CARDS_URL. Meanwhile, to use the feature, you need to manually
 // edit NOTIFICATION_CARDS_URL before building Chrome.
+// TODO(vadimt): Consider processing errors for all storage.set calls.
+
 /**
  * URL to retrieve notification cards.
  */
@@ -51,9 +52,179 @@ var INITIAL_POLLING_PERIOD_SECONDS = 300;  // 5 minutes
  */
 var MAXIMUM_POLLING_PERIOD_SECONDS = 3600;  // 1 hour
 
+/**
+ * Names for tasks that can be created by the extension.
+ */
+var UPDATE_CARDS_TASK_NAME = 'update-cards';

[skare_, 2013/02/22 20:53:39]

enum?

/** @enum {string} */
var Task = {
  UPDATE_CARDS = 'update-cards',
  CHECK_AWESOMENESS = 'make-sure-things-are-awesome'
};

then you can use Task.UPDATE_CARDS, and pass these around as types.

if we ever run through jscompiler, even just for type-checking, we get some
benefits too.

[vadimt, 2013/02/28 02:23:22]

Done.

+var DISMISS_CARD_TASK_NAME = 'dismiss-card';
+var CARD_CLICKED_TASK_NAME = 'card-clicked';
+
+/**
+ * Name of the alarm that triggers updating the cards.

[skare_, 2013/02/22 20:53:39]

probably don't need this comment

[vadimt, 2013/02/28 02:23:22]

Done.

+ */
+var UPDATE_ALARM_NAME = 'UPDATE';

[skare_, 2013/02/22 20:53:39]

any advantage to having the two alarm names in the same scope? then you could
enum those too.

[vadimt, 2013/02/28 02:23:22]

We are speaking about UPDATE_NOTIFICATIONS_ALARM_NAME and
CANNOT_UNLOAD_ALARM_NAME. They serve different purposes, therefore, cannot be
merged to one.

UPDATE_NOTIFICATIONS_ALARM_NAME is global, while CANNOT_UNLOAD_ALARM_NAME is
local to TaskManager, therefore, they cannot be gathered under one enum.

[skare_, 2013/03/02 00:11:39]

[ok] 
yeah, figured this was the case; phrased as "any plans for moving them" on the
off chance they could be combined.

[vadimt, 2013/03/06 21:04:38]

Done.

+
 var storage = chrome.storage.local;
 
 /**
+ * Check for internal errors.

[skare_, 2013/02/22 20:53:39]

split into another file? Do we have access to a module loader or is the
extension just copied into chrome binaries without modification?

[vadimt, 2013/02/28 02:23:22]

This won't be an issue; I've added a comment to do this after we settle with
other notes.

+ * @param {boolean} condition Condition that must be true.
+ * @param {string} message Diagnostic message for the case when the condition is
+ *     false.
+ */
+function assert(condition, message) {
+  // TODO(vadimt): Send UMAs instead of showing alert.
+  if (!condition) {
+    var errorText = 'ASSERT: ' + message;
+    console.error(errorText);
+    alert(errorText);
+  }
+}
+
+/**
+ * Build the closure that manages tasks (mutually exclusive chains of events).
+ * @return {Object} Set of methods to control tasks.
+ */
+function getTaskManager() {

[skare_, 2013/02/22 20:53:39]

you might want to just call this function TaskManager since a first read (e.g.
mine) is that this is a singleton getter.

[vadimt, 2013/02/28 02:23:22]

Done.

+  /**
+   * 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.
+   */
+  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;
+
+  /**
+   * Start the first queued task.
+   */
+  function startFirst() {
+    // 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});
+
+    // Start the oldest queued task, but not remove it from the queue.
+    assert(stepName == null, 'tasks.startFirst: stepName is not null');
+    var entry = queue[0];
+    stepName = entry.name + '-initial';
+    entry.task();
+  }
+
+  /**
+   * Check if a new task can be added to a queue that contains an existing task.
+   * @param {string} newTaskName Name of the new task.
+   * @param {string} queuedTaskName Name of the task in the queue.
+   * @return {boolean} Whether the new task doesn't conflict with the existing
+         task.

[skare_, 2013/02/22 20:53:39]

this line lost an asterisk

[vadimt, 2013/02/28 02:23:22]

Done.

+   */
+  function isCompatible(newTaskName, queuedTaskName) {
+    if (newTaskName == UPDATE_CARDS_TASK_NAME &&
+       queuedTaskName == UPDATE_CARDS_TASK_NAME) {

[skare_, 2013/02/22 20:53:39]

one more space

[vadimt, 2013/02/28 02:23:22]

Done.

+      // If a card update is requested while an old update is still in the
+      // queue, we don't need the new update.
+      return false;
+    }
+
+    return true;
+  }
+
+  /**
+   * Check 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 (!isCompatible(taskName, queue[i].name))
+        return false;
+
+    return true;
+  }
+
+  /**
+   * Add 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();
+    }
+  }
+
+  /**
+   * Complete the current task and start the next queued task if available.
+   */
+  function finish() {
+    assert(queue.length >= 1, 'tasks.finish: The task queue is empty.');
+    queue.shift();
+    stepName = null;
+
+    if (queue.length >= 1)
+      startFirst();
+  }
+
+  /**
+   * Associate a name with the current step of the task. Used for diagnostics
+   * only. A task is a chain of asynchronous events; setStep should be called
+   * before starting any asynchronous operation.
+   * @param {string} step Name of new step.
+   */
+  function setStep(step) {
+    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 assert once this bug is fixed:
+      // crbug.com/177563
+      // assert(false, 'Event page didn\'t unload, queue = ' +
+      // JSON.stringify(tasks) + ', step = ' + stepName + ' (ignore this assert
+      // if devtools is attached).');
+    }
+  });
+
+  chrome.runtime.onSuspend.addListener(function() {
+    chrome.alarms.clear(CANNOT_UNLOAD_ALARM_NAME);
+    assert(queue.length == 0 && stepName == null,
+      'Incomplete task when unloading event page, queue = ' +
+      JSON.stringify(queue) + ', step = ' + stepName);
+  });
+
+  return {
+    submit: submit,
+    setStep: setStep,
+    finish: finish
+  };
+}
+
+var tasks = getTaskManager();
+
+/**
  * Show a notification and remember information associated with it.
  * @param {Object} card Google Now card represented as a set of parameters for
  *     showing a Chrome notification.
@@ -113,6 +284,7 @@ function parseAndShowNotificationCards(response) {
     return;
   }
 
+  tasks.setStep('parseAndShowNotificationCards-get-active-notifications');

[skare_, 2013/02/22 20:53:39]

so setStep is purely diagnostic information to know how far along a given task
is?

[vadimt, 2013/02/28 02:23:22]

Yep. If a task quietly ends without calling finish(), we'll know which step to
blame.

   storage.get('activeNotifications', function(items) {
     // Mark existing notifications that received an update in this server
     // response.
@@ -148,6 +320,7 @@ function parseAndShowNotificationCards(response) {
     // to the initial value. This retry period will be used the next time we
     // fail to get the server-provided period.
     storage.set({retryDelaySeconds: INITIAL_POLLING_PERIOD_SECONDS});
+    tasks.finish();
   });
 }
 
@@ -163,12 +336,19 @@ function requestNotificationCards(requestParameters) {
   request.onload = function(event) {
     if (request.status == HTTP_OK)
       parseAndShowNotificationCards(request.response);
+    else
+      tasks.finish();
+  }
+
+  request.onerror = function(event) {
+    tasks.finish();
   }
 
   request.open(
       'GET',
       NOTIFICATION_CARDS_URL + '/notifications' + requestParameters,
       true);
+  tasks.setStep('requestNotificationCards-send-request');
   request.send();
 }
 
@@ -199,23 +379,27 @@ function requestNotificationCardsWithoutLocation(positionError) {
  * location.
  */
 function updateNotificationsCards() {
-  storage.get('retryDelaySeconds', function(items) {
-    // Immediately schedule the update after the current retry period. Then,
-    // we'll use update time from the server if available.
-    scheduleNextUpdate(items.retryDelaySeconds);
-
-    // TODO(vadimt): Consider interrupting waiting for the next update if we
-    // detect that the network conditions have changed. Also, decide whether the
-    // exponential backoff is needed both when we are offline and when there are
-    // failures on the server side.
-    var newRetryDelaySeconds =
-        Math.min(items.retryDelaySeconds * 2 * (1 + 0.2 * Math.random()),
-                 MAXIMUM_POLLING_PERIOD_SECONDS);
-    storage.set({retryDelaySeconds: newRetryDelaySeconds});
-
-    navigator.geolocation.getCurrentPosition(
-        requestNotificationCardsWithLocation,
-        requestNotificationCardsWithoutLocation);
+  tasks.submit(UPDATE_CARDS_TASK_NAME, function() {
+    tasks.setStep('updateNotificationsCards-get-retryDelaySeconds');
+    storage.get('retryDelaySeconds', function(items) {
+      // Immediately schedule the update after the current retry period. Then,
+      // we'll use update time from the server if available.
+      scheduleNextUpdate(items.retryDelaySeconds);
+
+      // TODO(vadimt): Consider interrupting waiting for the next update if we
+      // detect that the network conditions have changed. Also, decide whether
+      // the exponential backoff is needed both when we are offline and when
+      // there are failures on the server side.
+      var newRetryDelaySeconds =
+          Math.min(items.retryDelaySeconds * 2 * (1 + 0.2 * Math.random()),
+                   MAXIMUM_POLLING_PERIOD_SECONDS);
+      storage.set({retryDelaySeconds: newRetryDelaySeconds});
+
+      tasks.setStep('updateNotificationsCards-get-location');
+      navigator.geolocation.getCurrentPosition(
+          requestNotificationCardsWithLocation,
+          requestNotificationCardsWithoutLocation);
+    });
   });
 }
 
@@ -225,13 +409,17 @@ function updateNotificationsCards() {
  * @param {string} area Name of the notification's clicked area.
  */
 function onNotificationClicked(notificationId, area) {
-  storage.get('activeNotifications', function(items) {
-    var notificationActions = items.activeNotifications[notificationId] || {};
-    if (area in notificationActions) {
-      // Open URL specified for the clicked area.
-      // TODO(vadimt): Figure out whether to open link in a new tab etc.
-      chrome.windows.create({url: notificationActions[area]});
-    }
+  tasks.submit(CARD_CLICKED_TASK_NAME, function() {
+    tasks.setStep('onNotificationClicked-get-activeNotifications');
+    storage.get('activeNotifications', function(items) {
+      var notificationActions = items.activeNotifications[notificationId] || {};
+      if (area in notificationActions) {
+        // Open URL specified for the clicked area.
+        // TODO(vadimt): Figure out whether to open link in a new tab etc.
+        chrome.windows.create({url: notificationActions[area]});
+      }
+      tasks.finish();
+    });
   });
 }
 
@@ -242,19 +430,29 @@ function onNotificationClicked(notificationId, area) {
  */
 function onNotificationClosed(notificationId, byUser) {
   if (byUser) {
-    // TODO(vadimt): Analyze possible race conditions between request for cards
-    // and dismissal.
-    // Send a dismiss request to the server.
-    var requestParameters = '?id=' + notificationId;
-    var request = new XMLHttpRequest();
-    request.responseType = 'text';
-    // TODO(vadimt): If the request fails, for example, because there is no
-    // internet connection, do retry with exponential backoff.
-    request.open(
-      'GET',
-      NOTIFICATION_CARDS_URL + '/dismiss' + requestParameters,
-      true);
-    request.send();
+    tasks.submit(DISMISS_CARD_TASK_NAME, function() {
+      // Deleting the notification in case it was re-added while this task was
+      // waiting in the queue.
+      chrome.experimental.notification.delete(
+          notificationId,
+          function(wasDeleted) {});

[skare_, 2013/02/22 20:53:39]

unneeded param?

[vadimt, 2013/02/28 02:23:22]

Done.

+
+      // Send a dismiss request to the server.
+      var requestParameters = '?id=' + notificationId;
+      var request = new XMLHttpRequest();
+      request.responseType = 'text';
+      request.onloadend = function(event) {
+        tasks.finish();
+      }
+      // TODO(vadimt): If the request fails, for example, because there is no
+      // internet connection, do retry with exponential backoff.
+      request.open(
+        'GET',
+        NOTIFICATION_CARDS_URL + '/dismiss' + requestParameters,
+        true);
+      tasks.setStep('onNotificationClosed-send-request');
+      request.send();
+    });
   }
 }
 
@@ -266,10 +464,12 @@ function onNotificationClosed(notificationId, byUser) {
 function scheduleNextUpdate(delaySeconds) {
   // Schedule an alarm after the specified delay. 'periodInMinutes' is for the
   // case when we fail to re-register the alarm.
-  chrome.alarms.create({
+  var alarmInfo = {
     delayInMinutes: delaySeconds / 60,
     periodInMinutes: MAXIMUM_POLLING_PERIOD_SECONDS / 60
-  });
+  };
+
+  chrome.alarms.create(UPDATE_ALARM_NAME, alarmInfo);
 }
 
 /**
@@ -280,7 +480,8 @@ function initialize() {
     activeNotifications: {},
     retryDelaySeconds: INITIAL_POLLING_PERIOD_SECONDS
   };
-  storage.set(initialStorage, updateNotificationsCards);
+  storage.set(initialStorage);
+  updateNotificationsCards();
 }
 
 chrome.runtime.onInstalled.addListener(function(details) {
@@ -293,7 +494,8 @@ chrome.runtime.onStartup.addListener(function() {
 });
 
 chrome.alarms.onAlarm.addListener(function(alarm) {
-  updateNotificationsCards();
+  if (alarm.name == UPDATE_ALARM_NAME)

[skare_, 2013/02/22 20:53:39]

though I don't think it's in the style guide, you might hear some people request
braces around single-line if() statements in JS.

[vadimt, 2013/02/28 02:23:22]

As far as I recall, if the condition or the statement takes more than 1 line, we
need braces, otherwise, we don't need them.

[skare_, 2013/03/02 00:11:39]

yes that's correct. And I was a member of Team Omit-Braces-For-One-Line. But
always including braces seems to be a de facto style rule in some code... not
sure within Chrome; I guess we'll see.

On 2013/02/28 02:23:22, vadimt wrote:

[rgustafson, 2013/03/04 22:31:27]

"Do not use braces for single-line logic blocks."
http://www.chromium.org/developers/web-development-style-guide

On 2013/03/02 00:11:39, Travis Skare wrote:

[vadimt, 2013/03/06 21:04:38]

See rgustafson's note.

[vadimt, 2013/03/06 21:04:38]

Thanks rgustafson@!

+    updateNotificationsCards();
 });
 
 chrome.experimental.notification.onClicked.addListener(