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
 
+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';

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

Why did TaskName go away?

Why are these and the task-related function below not in the other file? You'd
have to reference them in both files, but I don't see a reason why we'd need
another areTasksConflicting-type function that we would be using for task
management.

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

http://en.wikipedia.org/wiki/Layer_(object-oriented_design)
For the sake of hygiene, readability, maintainability etc, it makes sense to
follow OOP rules even in the case of such a simple extension.
The contents of utility.js is a layer that is used by background.js. For the
above benefits, I want to design it as if it was reusable, even if it is's not
going to be used this way. Then, I cannot have TaskName, which list all concrete
task names, in this utility.js. Having it in background.js doesn't make sense
too, because no method in background.js takes it as a parameter.

+var DISMISS_CARD_TASK_NAME = 'dismiss-card';
+var CARD_CLICKED_TASK_NAME = 'card-clicked';
+
+/**
+  * Checks if a new task can't be added to a task 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 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 in the
+    // queue, we don't need the new update.
+    return true;
+  }
+

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

extra line

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

It makes sense to separate groups of operators with empty lines for readability.
I feel that this is a case where this is appropriate.

+  return false;
+}
+
+var tasks = TaskManager(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
  *     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.debugSetStepName(
+      '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[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.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.
  */
 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(UPDATE_CARDS_TASK_NAME, function() {
+    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});
+
+      tasks.debugSetStepName('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(CARD_CLICKED_TASK_NAME, function() {
+    tasks.debugSetStepName('onNotificationClicked-get-activeNotifications');
+    storage.get('activeNotifications', function(items) {
+      var actionUrls = items.activeNotifications[notificationId];
+      if (typeof actionUrls != 'object') {
+        // TODO(vadimt): report an error.
+        tasks.finish();
+        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(DISMISS_CARD_TASK_NAME, function() {
+      // 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() {
+        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.debugSetStepName('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);