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 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.
  */
 var NOTIFICATION_CARDS_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;
 
+/////////////////////////////////////////////////////////////////////////
+////// ABSOLUTELY NOT FORGET TO MOVE ASSERT AND TASK MANAGER INTO SEPARATE FILE
+////// IN THIS CL !!!!!!!!!!!!!
+////////////////////////////////////////////////////////////////////////
+
+/**
+ * Check 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 assert(condition, message) {
+  // TODO(vadimt): Send UMAs instead of showing alert.
+  if (!condition) {
+    var errorText = 'ASSERT: ' + message;
+    console.error(errorText);
+    alert(errorText);

[skare_, 2013/03/02 00:07:05]

console.assert()?

[skare_, 2013/03/02 00:07:05]

I didn't think alert() from a background page works. Might be wrong though..
does it?

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

Then no one will notice it.
I'd prefer these important internal errors to be more prominent.

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

We checked that it works, remember :) ?

[skare_, 2013/03/06 22:35:57]

[ok]
yes, but that was after this set of comments went out.


On 2013/03/06 21:04:38, vadimt wrote:

[vadimt, 2013/03/06 22:56:35]

No doubt about that.

[skare_, 2013/03/07 23:25:13]

bringing this comment back from the dead. Would console.assert() work instead of
console.error? Even supports a text argument like the above.

Or even use console.assert() instead of this function altogether--do we really
want alerts bugging people, especially since the flag that would let you stop
the extension (enable debugging of component extensions) is different from the
one that lets you turn it on (enable chrome now)? 

Also I know this is isolated but consider not having this be something called
assert() that does something slightly different from a standard function like
console.assert()

[vadimt, 2013/03/08 21:11:58]

Renamed assert to verify.

This assert is for things that should absolutely not happen, and if this
happens, we are in a big trouble (note that this will be removed before the
release). So I want our testers to know about such events immediately.
console.assert doesn't show a popup. So, I cannot use it here instead of alert.

Now that we have to check the condition anyways, console.error seems adequate.

I don't expect these assert to bug people a lot because (see above) this
absolutely shouldn't happen.

People will see and report to us the additional information included in the
assert, which will help to identify issues. So, showing asserts makes sense even
when debugging component extensions is off (note that early users often won't
use the component extension, but use the CRX extension).

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

If it's stuff that should never happen and this will be removed before release,
why isn't this just contained in tests? Those tests will continue to exist past
release, so you can continue to ensure that problems aren't coming up. 

On 2013/03/08 21:11:58, vadimt wrote:

[skare_, 2013/03/11 18:37:12]

re: the alert() line, would it be ok to not show alerts to users from the ether
without some opt-in (special extension, localStorage flag)? 

I think we talked about this in person briefly and there's some mention upthread
too, just asking explicitly here.

On 2013/03/11 18:11:04, rgustafson wrote:

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

The assert shouldn't happen in a different sense :)
Asserts stand for error condition that shouldn't happen if there were no bugs.
We need this code to catch bugs. If we move it to tests, we won't learn about
bugs in real code.

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

RE: re: the alert() line
Per offline talk, leaving alert for now.

+  }
+}
+
+/**
+ * Build the closure that manages tasks (mutually exclusive chains of events).

[skare_, 2013/03/02 00:07:05]

you could omit "build the closure" and just say e.g. "object to manage..."

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

Done.

+ * @return {Object} Set of methods to control 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.

[skare_, 2013/03/07 23:25:13]

@type this --
{Array.<Task>} // where you'd typedef, or
{Array.<Object.<TaskName, function()>>}
I think?

[vadimt, 2013/03/08 21:11:58]

Coool

+   */
+  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() {

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

startFirstTask() makes this more self explanatory imo

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

The idea is that these are members of TaskManager, so basically, this is
TaskManager::startFirst().
So, I didn't include 'task' word in method names.

+    // 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.

[skare_, 2013/03/02 00:07:05]

nit: s/not/don't

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

Done.

+    assert(stepName == null, 'tasks.startFirst: stepName is not null');
+    var entry = queue[0];

[skare_, 2013/03/02 00:07:05]

maybe bail out if queue is empty here?

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

This seems to be called only when there is a first task. Either check for empty
here for complete confidence or at least add to the description that at least
one task is a pre-req.
On 2013/03/02 00:07:05, Travis Skare wrote:

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

The method will be called only when the queue is not empty. I've added a comment
the this is a pre-req.

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

Done.

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

Done.

[skare_, 2013/03/06 22:35:57]

I still like bailing out on an empty queue (or like rgustafson says, enforce the
constraint via a code path) in case someone didn't read the comments for every
function he/she calls; otherwise the function will end halfway through with an
exception after changing state (i.e. setting an alarm)

On 2013/03/06 21:04:38, vadimt wrote:

[skare_, 2013/03/07 23:25:13]

[bringing this into latest round of comments too]

[vadimt, 2013/03/08 21:11:58]

Per the offline talk, seems we are proceeding as is?

[skare_, 2013/03/11 18:37:12]

so long as whatever the assert() is replaced with handles the failure gracefully
-- ideally by early-return and possibly adding to an error stat... prefer not to
depend on exceptions logged to the console for control flow.

On 2013/03/08 21:11:58, vadimt wrote:

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

Added to verify():
// TODO(vadimt): Make sure the execution doesn't continue after this call.

+    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

[skare_, 2013/03/02 00:07:05]

maybe invert logic -- isCompatible -> tasksConflict() or similar to avoid the
valid-but-slightly-hard-to-parse "whether X doesn't conflict" language?

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

Done.

+   *     task.
+   */
+  function isCompatible(newTaskName, queuedTaskName) {
+    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 false;
+    }
+
+    return true;
+  }
+
+  /**
+   * Check if a new task can be added to the task queue.

[skare_, 2013/03/02 00:07:05]

nit (throughout comments in the file): this is in the imperative mood. JS Style
(via C++ style) is generally to describe what the method does.

here, that'd be "Checks if a new task"

so maybe add some s's through the comments :)

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

Done.

+   * @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 (!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 {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();

[skare_, 2013/03/02 00:07:05]

it's ok as used now now, but you're ok with the fact that this function may
synchronously start the work (via startFirst, after setting alarm).

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

Yes, I think that it's better than starting asynchronously, which itself is an
async operation and requires care.

+    }
+  }
+
+  /**
+   * Complete the current task and start the next queued task if available.
+   */
+  function finish() {

[skare_, 2013/03/02 00:07:05]

high-level comment: a lot of JS patterns end up having a success (and/or error)
callback that can be passed around and called by a chain of async functions.

Did you consider having a function like that rather than having everything call
tasks.finish? Won't go into more detail on potential advantages there in case
you considered that.

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

I've considered this. I thought that this would be more beautiful from the
design standpoint, but would add callback params to everything. With the current
state of the code, it seems more reasonable for me to have explicit finish()
calls.
And, also, we have to call tasks.setStep explicitly...

[skare_, 2013/03/06 22:35:57]

feel free to get other people to comment here but I'll read the use of
start/finish again after responding to the more immediately useful patch reviews

On 2013/03/06 21:04:38, vadimt wrote:

[vadimt, 2013/03/06 22:56:35]

Done.

+    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) {

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

Is there a reason you don't use this inside the TaskManager also and opt for
'stepName = '? It makes it a bit harder to find all the places you are setting
the step.

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

One day, for example, I may want to add a "stepName != null" assert. This won't
work for the initial assignment.
In general, I believe that setting fields directly from class' methods is OK.

[skare_, 2013/03/06 22:35:57]

more general question -- could state name be instrumented into private debugging
builds as needed, getting rid of these calls in the production code? Does having
access to call chains at various points help? Are we expecting to need to read
these strings frequently?

also, setStep -> setStepName or some such? "setStep" seems like it might do more
than set a debug string

On 2013/03/06 21:04:38, vadimt wrote:

[vadimt, 2013/03/06 22:56:35]

The plan about this is:
in the release, we'll probably have a UMA counter per each step name. We'll
increment that counter if something went wrong during that step. The code will
evolve to that state.

In addition to this, having these calls only in debugging builds would only
increase the integration tax (== hell), I'd prefer to avoid this.

[skare_, 2013/03/07 23:25:13]

would UMA error counters be slightly more granular than step name?

overall -- if you expect the traces to be useful in the future, ok, but we
instrumented/added breakpoints after the fact to debug the G+ extensions and it
worked out pretty well. You might have enough info between queue[0].name and the
call stack to debug this on local machines.

setStep() makes things very slightly heavier to read, possibly because task
status seems more tied to a task than its parent collection class.

[vadimt, 2013/03/08 21:11:58]

May be, yes, may be, no. We'll have to decide then.
One situation is when the even page unloads, and there are incomplete tasks.
This may mean that the event chain was broken. We want (1) a popup appear (see
'verify()' implementation); (2) this popup should contain the step name to
indicate where the event chain was broken. If we don't set step names, we won't
know what step to blame.
Not sure I've understood this completely (about the parent collection class),
but I agree that setStepName makes the code heavier. However, I believe that
this is an absolutely necessary thing to catch hard-to-repro sutuations.

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

Are we really expecting our event page being improperly evicted being a problem?
I know that we had a problem with the location api, but that was also not some
corner case that was difficult to catch. Are there some other examples of when
this would be useful that can't be caught in tests?

On 2013/03/08 21:11:58, vadimt wrote:

[skare_, 2013/03/11 18:37:12]

re: 'parent name' I meant hypothetically
myTask.setInfo(debugString)
might be clearer than
taskManager.setStep('')
but we don't have properties of task objects like that.

overall setStep() seems slightly obtuse to read (for me at least) just because
it appears at somewhat random places in the code (legitimately), and the name
seems like it should have side effects rather than setting a debug string. I'd
still suggest instrumenting later or using private builds vs. shipping this, but
again you're the expert on what can go wrong.


On 2013/03/08 21:11:58, vadimt wrote:

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

ignore setStep() naming comment; resolved in next patch.

On 2013/03/11 18:37:12, Travis Skare wrote:

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

If I knew situations that cannot be caught with tests, I would write tests for
them :)

We check 2 things:
1. Event page is unloaded, and a task is still running. This means that the
chain of event was broken, and the task didn't get to the finish() call. We must
have an assert for this.
2. Event page doesn't get unloaded in a reasonable time. This likely means that
the task or task manager is in an infinitely looping chain of events.

Both situations are so important that we need to show alert in before-release
versions, and increase UMA counters after the release.

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

RE: re: 'parent name' I meant hypothetically
In this case, we want this code to evolve into something that we ship. Agreed,
these calls don't make code more beautiful, but this is how things often have to
be done in this imperfect world...

For now, renaming setStepName into debugSetStepName.

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

(In a gloom voice:) Too late.
Already renamed to debugSetStepName, which is quite adequate to the current
scenario. Will rename again as the semantics changes.

+    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 = TaskManager();
+
 /**
  * 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.
  * @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.experimental.notification.create(
       card.notificationId,
       card.notification,
-      function(assignedNotificationId) {});
+      function() {});
 
   notificationsUrlInfo[card.notificationId] = card.actionUrls;
 }
 
 /**
  * Parse 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.setStep('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.experimental.notification.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.
  * @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.onload = function() {
     if (request.status == HTTP_OK)
       parseAndShowNotificationCards(request.response);
+    else
+      tasks.finish();
+  }
+
+  request.onerror = function() {

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

Are there any other callbacks we need to handle like ontimeout? perhaps use
onloadend again but only finish conditionally on status?

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

Done.

+    tasks.finish();
   }
 
   request.open(
       'GET',
       NOTIFICATION_CARDS_URL + '/notifications' + requestParameters,
       true);
+  tasks.setStep('requestNotificationCards-send-request');
   request.send();
 }
 
 /**
  * Request 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 =
@@ skipping 10 matching lines @@
  */
 function requestNotificationCardsWithoutLocation(positionError) {
   requestNotificationCards('');
 }
 
 /**
  * Obtain new location; request and show 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.submit(TaskName.UPDATE_CARDS, 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});
+      // 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.setStep('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;
-    }
+  tasks.submit(TaskName.CARD_CLICKED, function() {
+    tasks.setStep('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);
+      var url = selector(actionUrls);
 
-    if (typeof url != 'string') {
-      // TODO(vadimt): report an error.
-      return;
-    }
+      if (typeof url != 'string') {
+        // TODO(vadimt): report an error.
+        tasks.finish();
+        return;
+      }
 
-    chrome.tabs.create({url: url});
+      chrome.tabs.create({url: url});
+      tasks.finish();
+    });
   });
 }
 
 /**
  * Callback for chrome.experimental.notification.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();
+    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(

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

This is going to have a notification pop up for a small period of time and then
disappear? Seems sloppy. Couldn't we never reshow dismissed notifications?

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

The reasoning for not doing this is that I expect this to happen extremely
rarely. I could add a logic that would try to reduce likelihood of this, but I
feel that complicating the code is not worth the outcome in this case.

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

I'm okay with this for now. Eventually, I think the complexity may be worth
ridding our users of weird behavior.
On 2013/03/06 21:04:38, vadimt wrote:

[skare_, 2013/03/11 18:37:12]

especially if there's ever the ability to have the notifications center trigger
sound, or for priority 1/2 notifications.

On 2013/03/11 18:11:05, rgustafson wrote:

+          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.setStep('onNotificationClosed-send-request');
+      request.send();
+    });
   }
 }
 
 /**
  * Schedule 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.
  */
 function initialize() {
   var initialStorage = {
     activeNotifications: {},
     retryDelaySeconds: INITIAL_POLLING_PERIOD_SECONDS
   };
-  storage.set(initialStorage, updateNotificationsCards);
+  storage.set(initialStorage);
+  updateNotificationsCards();

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

Couldn't retryDelaySeconds not be set when update tries to fetch it now?

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

I had a long conversation with extensions guys who promised me that this won't
happen. They'll document this, and add a test.

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

What exactly was their promise? A set followed by a get will always include the
new set? Or something more/less specific than that?
On 2013/03/06 21:04:38, vadimt wrote:

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

Literally:
If get is executed after set(X), 'get' will receive X.
From another mail:
set(1); get; set(2); get-callback; will feed get-callback with 1.
===================================
From another mail: the behavior that was confirmed by them:
1. We use the fact that you can 'get' the value set by 'set' without waiting for
'set's callback. This is not cutting a corner:

function eventHandler1() {
  set()
}

function eventHandler2() {
  get()
}

Assuming that any of these event handlers can be executed at any moment, it
really helps to know that eventHandler2() following eventHandler1() gets a
correct value even if it gets executed before set's callback.

 }
 
 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.experimental.notification.onClicked.addListener(
     function(notificationId) {
       onNotificationClicked(notificationId, function(actionUrls) {
         return actionUrls.messageUrl;
       });
     });
 
 chrome.experimental.notification.onButtonClicked.addListener(
     function(notificationId, buttonIndex) {
       onNotificationClicked(notificationId, function(actionUrls) {
         if (!Array.isArray(actionUrls.buttonUrls))
           return undefined;
 
         return actionUrls.buttonUrls[buttonIndex];
       });
     });
 
 chrome.experimental.notification.onClosed.addListener(onNotificationClosed);