Preventing race conditions in Google Now extension

chrome/browser/resources/google_now/background.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 The event page for Google Now for Chrome implementation.
  * The Google Now event page gets Google Now cards from the server and shows
  * them as Chrome notifications.
  * The service performs periodic updating of Google Now cards.
  * Each updating of the cards includes 3 steps:
  * 1. Obtaining the location of the machine;
  * 2. Making a server request based on that location;
  * 3. Showing the received cards as notifications.
  */
 
 // TODO(vadimt): Use background permission to show notifications even when all
 // browser windows are closed.
 // TODO(vadimt): Remove the C++ implementation.
 // TODO(vadimt): Decide what to do in incognito mode.
 // TODO(vadimt): Gather UMAs.
 // TODO(vadimt): Honor the flag the enables Google Now integration.
 // 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.
 /**
  * URL to retrieve notification cards.
  */
 var NOTIFICATION_CARDS_URL = localStorage['server_url'];
 
 /**
  * Standard response code for successful HTTP requests. This is the only success
  * code the server will send.
  */
 var HTTP_OK = 200;
 
 /**
  * Initial period for polling for Google Now Notifications cards to use when the
  * period from the server is not available.
  */
 var INITIAL_POLLING_PERIOD_SECONDS = 300;  // 5 minutes
 
 /**
  * Maximal period for polling for Google Now Notifications cards to use when the
  * period from the server is not available.
  */
 var MAXIMUM_POLLING_PERIOD_SECONDS = 3600;  // 1 hour
 
+/**
+ * Names for tasks that can be created by the extension.
+ * @enum {string}
+ */
+var TaskName = {
+  UPDATE_CARDS: 'update-cards',
+  DISMISS_CARD: 'dismiss-card',
+  CARD_CLICKED: 'card-clicked'
+};
+
+var UPDATE_NOTIFICATIONS_ALARM_NAME = 'UPDATE';
+
 var storage = chrome.storage.local;
 
-/**
- * Show a notification and remember information associated with it.
+/////////////////////////////////////////////////////////////////////////
+////// ABSOLUTELY NOT FORGET TO MOVE ASSERT AND TASK MANAGER INTO SEPARATE FILE
+////// IN THIS CL !!!!!!!!!!!!!
+////////////////////////////////////////////////////////////////////////
+
+/**
+ * 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.
+  if (!condition) {
+    var errorText = 'ASSERT: ' + message;
+    console.error(errorText);
+    alert(errorText);
+  }
+}
+
+/**
+ * Builds the object to manage tasks (mutually exclusive chains of events).
+ * @return {Object} Set of methods to manage tasks.
+ */
+function TaskManager() {
+  /**
+   * 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.<TaskName, 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't 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 conflicts with the existing task.
+   */
+  function doConflict(newTaskName, queuedTaskName) {

[rgustafson, 2013/03/11 18:11:05]

Things starting with do generally mean taking action rather than coming up with
a boolean value. 

isConflicting
hasConflict
or just forget the verb with tasksConflict

[vadimt, 2013/03/11 21:31:53]

Done.

+    if (newTaskName == TaskName.UPDATE_CARDS &&
+        queuedTaskName == TaskName.UPDATE_CARDS) {
+      // If a card update is requested while an old update is still in the
+      // queue, we don't need the new update.
+      return true;
+    }
+
+    return false;
+  }
+
+  /**
+   * Checks if a new task can be added to the task queue.
+   * @param {TaskName} 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 (doConflict(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.

[skare_, 2013/03/11 19:46:26]

silently ignoring the task will always be ok?
seems like it, just asking.

[vadimt, 2013/03/11 21:31:53]

TaskManager::areConflicting() returns true if:
(1) It's necessary to ignore a task;
AND
(2) It's safe to ignore this task.

So, yes.

+   * @param {TaskName} 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.
+   */
+  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; setStepName should be
+   * called before starting any asynchronous operation.
+   * @param {string} step Name of new step.
+   */
+  function setStepName(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,
+    setStepName: setStepName,
+    finish: finish
+  };
+}
+
+var tasks = TaskManager();
+
+/**
+ * Shows a notification and remember information associated with it.

[skare_, 2013/03/11 19:53:07]

tiny nit: s/remember/remembers

[vadimt, 2013/03/11 21:31:53]

Done.

  * @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
  *     notification's set of URLs.
  */
 function createNotification(card, notificationsUrlInfo) {
   // Create a notification or quietly update if it already exists.
   // TODO(vadimt): Implement non-quiet updates.
   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.
  */
 function parseAndShowNotificationCards(response) {
   try {
     var parsedResponse = JSON.parse(response);
   } catch (error) {
     // TODO(vadimt): Report errors to the user.
     return;
   }
 
   var cards = parsedResponse.cards;
 
   if (!(cards instanceof Array)) {
     // TODO(vadimt): Report errors to the user.
     return;
   }
 
   if (typeof parsedResponse.expiration_timestamp_seconds != 'number') {
     // TODO(vadimt): Report errors to the user.
     return;
   }
 
+  tasks.setStepName('parseAndShowNotificationCards-get-active-notifications');
   storage.get('activeNotifications', function(items) {
     // Mark existing notifications that received an update in this server
     // response.
     for (var i = 0; i < cards.length; ++i) {
       var notificationId = cards[i].notificationId;
       if (notificationId in items.activeNotifications)
         items.activeNotifications[notificationId].hasUpdate = true;
     }
 
     // Delete notifications that didn't receive an update.
     for (var notificationId in items.activeNotifications)
       if (items.activeNotifications.hasOwnProperty(notificationId) &&
           !items.activeNotifications[notificationId].hasUpdate) {
         chrome.notifications.clear(
             notificationId,
-            function(wasDeleted) {});
+            function() {});
     }
 
     // Create/update notifications and store their new properties.
     var notificationsUrlInfo = {};
 
     for (var i = 0; i < cards.length; ++i) {
       try {
         createNotification(cards[i], notificationsUrlInfo);
       } catch (error) {
         // TODO(vadimt): Report errors to the user.
       }
     }
     storage.set({activeNotifications: notificationsUrlInfo});
 
     scheduleNextUpdate(parsedResponse.expiration_timestamp_seconds);
 
     // Now that we got a valid response from the server, reset the retry period
     // 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();
   });
 }
 
 /**
- * Request notification cards from the server.
+ * Requests notification cards from the server.
  * @param {string} requestParameters Query string for the request.
  */
 function requestNotificationCards(requestParameters) {
   // 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() {
     if (request.status == HTTP_OK)
       parseAndShowNotificationCards(request.response);
+    else
+      tasks.finish();
   }
 
   request.open(
       'GET',
       NOTIFICATION_CARDS_URL + '/notifications' + requestParameters,
       true);
+  tasks.setStepName('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.
  */
 function requestNotificationCardsWithLocation(position) {
   // TODO(vadimt): Should we use 'q' as the parameter name?
   var requestParameters =
       '?q=' + position.coords.latitude +
       ',' + position.coords.longitude +
       ',' + position.coords.accuracy;
 
   requestNotificationCards(requestParameters);
 }
 
 /**
- * Request notification cards from the server when we don't have geolocation.
+ * Requests 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);
-
-    // 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(TaskName.UPDATE_CARDS, function() {
+    tasks.setStepName('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.setStepName('updateNotificationsCards-get-location');
+      navigator.geolocation.getCurrentPosition(
+          requestNotificationCardsWithLocation,
+          requestNotificationCardsWithoutLocation);
+    });
   });
 }
 
 /**
  * Opens URL corresponding to the clicked part of the notification.
  * @param {string} notificationId Unique identifier of the notification.
  * @param {function(Object): string} selector Function that extracts the url for
  *     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;
-    }
-
-    var url = selector(actionUrls);
-
-    if (typeof url != 'string') {
-      // TODO(vadimt): report an error.
-      return;
-    }
-
-    chrome.tabs.create({url: url});
+  tasks.submit(TaskName.CARD_CLICKED, function() {
+    tasks.setStepName('onNotificationClicked-get-activeNotifications');
+    storage.get('activeNotifications', function(items) {
+      var actionUrls = items.activeNotifications[notificationId];
+      if (typeof actionUrls != 'object') {
+        // TODO(vadimt): report an error.
+        tasks.finish();

[skare_, 2013/03/11 19:53:07]

would it be useful in the future to have finish() accept an error object/string?

[vadimt, 2013/03/11 21:31:53]

At this point, it doesn't seem so. finish() is to tell the task manager to start
next task, and for this it doesn't need to know how happy the previous task was.

+        return;
+      }
+
+      var url = selector(actionUrls);
+
+      if (typeof url != 'string') {
+        // TODO(vadimt): report an error.
+        tasks.finish();
+        return;
+      }
+
+      chrome.tabs.create({url: url});
+      tasks.finish();
+    });
   });
 }
 
 /**
  * Callback for chrome.notifications.onClosed event.
  * @param {string} notificationId Unique identifier of the notification.
  * @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.
-    // 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();
-  }
-}
-
-/**
- * Schedule next update for notification cards.
+    tasks.submit(TaskName.DISMISS_CARD, function() {
+      // Deleting the notification in case it was re-added while this task was
+      // waiting in the queue.
+      chrome.experimental.notification.clear(
+          notificationId,
+          function() {});
+
+      // Send a dismiss request to the server.
+      var requestParameters = '?id=' + notificationId;
+      var request = new XMLHttpRequest();
+      request.responseType = 'text';
+      request.onloadend = function() {
+        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.setStepName('onNotificationClosed-send-request');
+      request.send();
+    });
+  }
+}
+
+/**
+ * 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
-  });
-}
-
-/**
- * Initialize the event page on install or on browser startup.
+  };
+
+  chrome.alarms.create(UPDATE_NOTIFICATIONS_ALARM_NAME, alarmInfo);
+}
+
+/**
+ * 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) {
   if (details.reason != 'chrome_update')
     initialize();
 });
 
 chrome.runtime.onStartup.addListener(function() {
   initialize();
 });
 
 chrome.alarms.onAlarm.addListener(function(alarm) {
-  updateNotificationsCards();
+  if (alarm.name == UPDATE_NOTIFICATIONS_ALARM_NAME)
+    updateNotificationsCards();
 });
 
 chrome.notifications.onClicked.addListener(
     function(notificationId) {
       onNotificationClicked(notificationId, function(actionUrls) {
         return actionUrls.messageUrl;
       });
     });
 
 chrome.notifications.onButtonClicked.addListener(
     function(notificationId, buttonIndex) {
       onNotificationClicked(notificationId, function(actionUrls) {
         if (!Array.isArray(actionUrls.buttonUrls))
           return undefined;
 
         return actionUrls.buttonUrls[buttonIndex];
       });
     });
 
 chrome.notifications.onClosed.addListener(onNotificationClosed);