Preventing race conditions in Google Now extension.

Introducing a task manager. A task is an event chain that does certain work that we want to be atomic. Execution of multiple tasks is serialized.

Also, adding checks for anomalous situations, such as a forever-running task keeps the event page from unloading or a task still running when the event page unloads.

BUG=164227

Review URL: https://chromiumcodereview.appspot.com/12316075

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@190023 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/resources/component_extension_resources.grd

@@ -111,6 +111,7 @@
       </if>
       <if expr="pp_ifdef('enable_google_now')">
         <include name="IDR_GOOGLE_NOW_BACKGROUND_JS" file="google_now/background.js" type="BINDATA" />
+        <include name="IDR_GOOGLE_NOW_UTILITY_JS" file="google_now/utility.js" type="BINDATA" />
       </if>
       <include name="IDR_APPS_DEBUGGER_MAIN" file="apps_debugger/main.html" flattenhtml="true" allowexternalscript="true" type="BINDATA" />
       <include name="IDR_APPS_DEBUGGER_BACKGROUND_JS" file="apps_debugger/background.js" flattenhtml="true" type="BINDATA" />

chrome/browser/resources/google_now/background.js

@@ -24,8 +24,8 @@
 // TODO(vadimt): Figure out the final values of the constants.
 // TODO(vadimt): Report internal and server errors. Collect UMAs on errors where
 // appropriate. Also consider logging errors or throwing exceptions.
-
 // TODO(vadimt): Consider processing errors for all storage.set calls.
+
 // TODO(vadimt): Figure out the server name. Use it in the manifest and for
 // NOTIFICATION_CARDS_URL. Meanwhile, to use the feature, you need to manually
 // set the server name via local storage.
@@ -52,10 +52,39 @@ var INITIAL_POLLING_PERIOD_SECONDS = 300;  // 5 minutes
  */
 var MAXIMUM_POLLING_PERIOD_SECONDS = 3600;  // 1 hour
 
+var UPDATE_NOTIFICATIONS_ALARM_NAME = 'UPDATE';
+
 var storage = chrome.storage.local;
 
 /**
- * Show a notification and remember information associated with it.
+ * Names for tasks that can be created by the extension.
+ */
+var UPDATE_CARDS_TASK_NAME = 'update-cards';
+var DISMISS_CARD_TASK_NAME = 'dismiss-card';
+var CARD_CLICKED_TASK_NAME = 'card-clicked';
+
+/**
+ * Checks if a new task can't be scheduled when another task is already
+ * scheduled.
+ * @param {string} newTaskName Name of the new task.
+ * @param {string} queuedTaskName Name of the task in the queue.
+ * @return {boolean} Whether the new task conflicts with the existing task.
+ */
+function areTasksConflicting(newTaskName, queuedTaskName) {
+  if (newTaskName == UPDATE_CARDS_TASK_NAME &&
+      queuedTaskName == UPDATE_CARDS_TASK_NAME) {
+    // If a card update is requested while an old update is still scheduled, we
+    // don't need the new update.
+    return true;
+  }
+
+  return false;
+}
+
+var tasks = buildTaskManager(areTasksConflicting);
+
+/**
+ * Shows a notification and remembers information associated with it.
  * @param {Object} card Google Now card represented as a set of parameters for
  *     showing a Chrome notification.
  * @param {Object} notificationsUrlInfo Map from notification id to the
@@ -67,17 +96,18 @@ function createNotification(card, notificationsUrlInfo) {
   chrome.notifications.create(
       card.notificationId,
       card.notification,
-      function(assignedNotificationId) {});
+      function() {});
 
   notificationsUrlInfo[card.notificationId] = card.actionUrls;
 }
 
 /**
- * Parse JSON response from the notification server, show notifications and
+ * Parses JSON response from the notification server, show notifications and
  * schedule next update.
  * @param {string} response Server response.
+ * @param {function()} callback Completion callback.
  */
-function parseAndShowNotificationCards(response) {
+function parseAndShowNotificationCards(response, callback) {
   try {
     var parsedResponse = JSON.parse(response);
   } catch (error) {
@@ -97,6 +127,8 @@ function parseAndShowNotificationCards(response) {
     return;
   }
 
+  tasks.debugSetStepName(
+      'parseAndShowNotificationCards-get-active-notifications');
   storage.get('activeNotifications', function(items) {
     // Mark existing notifications that received an update in this server
     // response.
@@ -107,11 +139,12 @@ function parseAndShowNotificationCards(response) {
     }
 
     // Delete notifications that didn't receive an update.
-    for (var notificationId in items.activeNotifications)
+    for (var notificationId in items.activeNotifications) {
       if (!items.activeNotifications[notificationId].hasUpdate) {
         chrome.notifications.clear(
             notificationId,
-            function(wasDeleted) {});
+            function() {});
+      }
     }
 
     // Create/update notifications and store their new properties.
@@ -132,74 +165,80 @@ 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});
+    callback();
   });
 }
 
 /**
- * Request notification cards from the server.
+ * Requests notification cards from the server.
  * @param {string} requestParameters Query string for the request.
+ * @param {function()} callback Completion callback.
  */
-function requestNotificationCards(requestParameters) {
+function requestNotificationCards(requestParameters, callback) {
   // TODO(vadimt): Figure out how to send user's identity to the server.
   var request = new XMLHttpRequest();
 
   request.responseType = 'text';
-  request.onload = function(event) {
+  request.onloadend = function(event) {
     if (request.status == HTTP_OK)
-      parseAndShowNotificationCards(request.response);
-  }
+      parseAndShowNotificationCards(request.response, callback);
+    else
+      callback();
+  };
 
   request.open(
       'GET',
       NOTIFICATION_CARDS_URL + '/notifications' + requestParameters,
       true);
+  tasks.debugSetStepName('requestNotificationCards-send-request');
   request.send();
 }
 
 /**
- * Request notification cards from the server when we have geolocation.
+ * Requests notification cards from the server when we have geolocation.
  * @param {Geoposition} position Location of this computer.
+ * @param {function()} callback Completion callback.
  */
-function requestNotificationCardsWithLocation(position) {
+function requestNotificationCardsWithLocation(position, callback) {
   // TODO(vadimt): Should we use 'q' as the parameter name?
   var requestParameters =
       '?q=' + position.coords.latitude +
       ',' + position.coords.longitude +
       ',' + position.coords.accuracy;
 
-  requestNotificationCards(requestParameters);
+  requestNotificationCards(requestParameters, callback);
 }
 
 /**
- * Request notification cards from the server when we don't have geolocation.
- * @param {PositionError} positionError Position error.
- */
-function requestNotificationCardsWithoutLocation(positionError) {
-  requestNotificationCards('');
-}
-
-/**
- * Obtain new location; request and show notification cards based on this
+ * Obtains new location; requests and shows notification cards based on this
  * 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);
+  tasks.add(UPDATE_CARDS_TASK_NAME, function(callback) {
+    tasks.debugSetStepName('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});
+      // 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.debugSetStepName('updateNotificationsCards-get-location');
+      navigator.geolocation.getCurrentPosition(
+          function(position) {
+            requestNotificationCardsWithLocation(position, callback);
+          },
+          function() {
+            requestNotificationCards('', callback);
+          });
+    });
   });
 }
 
@@ -210,21 +249,27 @@ function updateNotificationsCards() {
  *     the clicked area from the button action URLs info.
  */
 function onNotificationClicked(notificationId, selector) {
-  storage.get('activeNotifications', function(items) {
-    var actionUrls = items.activeNotifications[notificationId];
-    if (typeof actionUrls != 'object') {
-      // TODO(vadimt): report an error.
-      return;
-    }
+  tasks.add(CARD_CLICKED_TASK_NAME, function(callback) {
+    tasks.debugSetStepName('onNotificationClicked-get-activeNotifications');
+    storage.get('activeNotifications', function(items) {
+      var actionUrls = items.activeNotifications[notificationId];
+      if (typeof actionUrls != 'object') {
+        // TODO(vadimt): report an error.
+        callback();
+        return;
+      }
 
-    var url = selector(actionUrls);
+      var url = selector(actionUrls);
 
-    if (typeof url != 'string') {
-      // TODO(vadimt): report an error.
-      return;
-    }
+      if (typeof url != 'string') {
+        // TODO(vadimt): report an error.
+        callback();
+        return;
+      }
 
-    chrome.tabs.create({url: url});
+      chrome.tabs.create({url: url});
+      callback();
+    });
   });
 }
 
@@ -234,46 +279,60 @@ function onNotificationClicked(notificationId, selector) {
  * @param {boolean} byUser Whether the notification was closed by the user.
  */
 function onNotificationClosed(notificationId, byUser) {
-  if (byUser) {
-    // TODO(vadimt): Analyze possible race conditions between request for cards
-    // and dismissal.
+  if (!byUser)
+    return;
+
+  tasks.add(DISMISS_CARD_TASK_NAME, function(callback) {
+    // Deleting the notification in case it was re-added while this task was
+    // waiting in the queue.
+    chrome.notifications.clear(
+        notificationId,
+        function() {});
+
     // Send a dismiss request to the server.
     var requestParameters = '?id=' + notificationId;
     var request = new XMLHttpRequest();
     request.responseType = 'text';
+    request.onloadend = function(event) {
+      callback();
+    };
     // 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.debugSetStepName('onNotificationClosed-send-request');
     request.send();
-  }
+  });
 }
 
 /**
- * Schedule next update for notification cards.
+ * Schedules next update for notification cards.
  * @param {int} delaySeconds Length of time in seconds after which the alarm
  *     event should fire.
  */
 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_NOTIFICATIONS_ALARM_NAME, alarmInfo);
 }
 
 /**
- * Initialize the event page on install or on browser startup.
+ * Initializes the event page on install or on browser startup.
  */
 function initialize() {
   var initialStorage = {
     activeNotifications: {},
     retryDelaySeconds: INITIAL_POLLING_PERIOD_SECONDS
   };
-  storage.set(initialStorage, updateNotificationsCards);
+  storage.set(initialStorage);
+  updateNotificationsCards();
 }
 
 chrome.runtime.onInstalled.addListener(function(details) {
@@ -286,7 +345,8 @@ chrome.runtime.onStartup.addListener(function() {
 });
 
 chrome.alarms.onAlarm.addListener(function(alarm) {
-  updateNotificationsCards();
+  if (alarm.name == UPDATE_NOTIFICATIONS_ALARM_NAME)
+    updateNotificationsCards();
 });
 
 chrome.notifications.onClicked.addListener(

chrome/browser/resources/google_now/manifest.json

@@ -16,7 +16,7 @@
   ],
   "manifest_version": 2,
   "background": {
-    "scripts": ["background.js"],
+    "scripts": ["utility.js", "background.js"],
     "persistent": false
   }
 }

chrome/browser/resources/google_now/utility.js

@@ -0,0 +1,156 @@
+// 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);
+  }
+}
+
+/**
+ * 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) {
+  /**
+   * 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(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});
+
+    // Start the oldest queued task, but don'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(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))
+        return false;
+    }
+
+    return true;
+  }
+
+  /**
+   * Adds a new task. If another task is not running, runs the task immediately.
+   * If any task in the queue is not compatible with the task, ignores the new
+   * task. Otherwise, stores the task for future execution.
+   * @param {string} taskName Name of the task.
+   * @param {function(function())} task Function to run. Takes a callback
+   *     parameter.
+   */
+  function add(taskName, task) {
+    if (!canQueue(taskName))
+      return;
+
+    queue.push({name: taskName, task: task});
+
+    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.');
+    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 {
+    add: add,
+    debugSetStepName: debugSetStepName
+  };
+}