Chrome Now notificationGroups Storage Race Condition Fix

Updating cards involves updating the notificationGroups in rapid succession,
leading to race conditions between storage.get and storage.set. The queued
up storage.gets may dispatch all at once before each storage.set, meaning that
the storage.set will set stale values.

In other words, we get
storage.get
storage.get
storage.set
storage.set

instead of
storage.get
storage.set
storage.get
storage.set

The fix here is to move the requests outside of the for loop, adding
arguments to functions where necessary to work around this gotcha.

BUG=164227

Review URL: https://codereview.chromium.org/114533002

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

2 files changed, 111 insertions, 85 deletions

diff --git a/chrome/browser/resources/google_now/background.js b/chrome/browser/resources/google_now/background.js
index 1a35d40..94b019e 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -293,12 +293,16 @@ function setAuthorization(request, callbackBoolean) {
 
 /**
  * Shows parsed and combined cards as notifications.
+ * @param {Object.<string, StoredNotificationGroup>} notificationGroups Map from
+ *     group name to group information.
  * @param {Object.<ChromeNotificationId, CombinedCard>} cards Map from
  *     chromeNotificationId to the combined card, containing cards to show.
+ * @param {function()} onSuccess Called on success.
  * @param {function(ReceivedNotification)=} onCardShown Optional parameter
  *     called when each card is shown.
  */
-function showNotificationCards(cards, onCardShown) {
+function showNotificationCards(
+    notificationGroups, cards, onSuccess, onCardShown) {
   console.log('showNotificationCards ' + JSON.stringify(cards));
 
   instrumented.notifications.getAll(function(notifications) {
@@ -320,9 +324,11 @@ function showNotificationCards(cards, onCardShown) {
       notificationsData[chromeNotificationId] = cardSet.update(
           chromeNotificationId,
           cards[chromeNotificationId],
+          notificationGroups,
           onCardShown);
     }
     chrome.storage.local.set({notificationsData: notificationsData});
+    onSuccess();
   });
 }
 
@@ -417,10 +423,12 @@ function scheduleNextPoll(groups, isOptedIn) {
  * them.
  * @param {Object.<string, StoredNotificationGroup>} notificationGroups Map from
  *     group name to group information.
+ * @param {function()} onSuccess Called on success.
  * @param {function(ReceivedNotification)=} onCardShown Optional parameter
  *     called when each card is shown.
  */
-function combineAndShowNotificationCards(notificationGroups, onCardShown) {
+function combineAndShowNotificationCards(
+    notificationGroups, onSuccess, onCardShown) {
   console.log('combineAndShowNotificationCards ' +
       JSON.stringify(notificationGroups));
   /** @type {Object.<ChromeNotificationId, CombinedCard>} */
@@ -429,7 +437,8 @@ function combineAndShowNotificationCards(notificationGroups, onCardShown) {
   for (var groupName in notificationGroups)
     combineGroup(combinedCards, notificationGroups[groupName]);
 
-  showNotificationCards(combinedCards, onCardShown);
+  showNotificationCards(
+      notificationGroups, combinedCards, onSuccess, onCardShown);
 }
 
 /**
@@ -531,12 +540,16 @@ function parseAndShowNotificationCards(response, onCardShown) {
         }
 
         scheduleNextPoll(updatedGroups, !parsedResponse.googleNowDisabled);
-        chrome.storage.local.set({
-          notificationGroups: updatedGroups,
-          recentDismissals: updatedRecentDismissals
-        });
-        combineAndShowNotificationCards(updatedGroups, onCardShown);
-        recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
+        combineAndShowNotificationCards(
+            updatedGroups,
+            function() {
+              chrome.storage.local.set({
+                notificationGroups: updatedGroups,
+                recentDismissals: updatedRecentDismissals
+              });
+              recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
+            },
+            onCardShown);
       });
 }
 
@@ -905,42 +918,47 @@ function onNotificationClosed(chromeNotificationId, byUser) {
     dismissalAttempts.start();
 
     instrumented.storage.local.get(
-        ['pendingDismissals', 'notificationsData'], function(items) {
-      items = items || {};
-      /** @type {Array.<PendingDismissal>} */
-      items.pendingDismissals = items.pendingDismissals || [];
-      /** @type {Object.<string, NotificationDataEntry>} */
-      items.notificationsData = items.notificationsData || {};
-
-      /** @type {NotificationDataEntry} */
-      var notificationData = items.notificationsData[chromeNotificationId] || {
-        timestamp: Date.now(),
-        combinedCard: []
-      };
-
-      var dismissalResult =
-          cardSet.onDismissal(chromeNotificationId, notificationData);
-
-      for (var i = 0; i < dismissalResult.dismissals.length; i++) {
-        /** @type {PendingDismissal} */
-        var dismissal = {
-          chromeNotificationId: chromeNotificationId,
-          time: Date.now(),
-          dismissalData: dismissalResult.dismissals[i]
-        };
-        items.pendingDismissals.push(dismissal);
-      }
+        ['pendingDismissals', 'notificationsData', 'notificationGroups'],
+        function(items) {
+          items = items || {};
+          /** @type {Array.<PendingDismissal>} */
+          items.pendingDismissals = items.pendingDismissals || [];
+          /** @type {Object.<string, NotificationDataEntry>} */
+          items.notificationsData = items.notificationsData || {};
+          /** @type {Object.<string, StoredNotificationGroup>} */
+          items.notificationGroups = items.notificationGroups || {};
 
-      items.notificationsData[chromeNotificationId] =
-          dismissalResult.notificationData;
+          /** @type {NotificationDataEntry} */
+          var notificationData =
+              items.notificationsData[chromeNotificationId] ||
+              {
+                timestamp: Date.now(),
+                combinedCard: []
+              };
+
+          var dismissalResult =
+              cardSet.onDismissal(
+                  chromeNotificationId,
+                  notificationData,
+                  items.notificationGroups);
+
+          for (var i = 0; i < dismissalResult.dismissals.length; i++) {
+            /** @type {PendingDismissal} */
+            var dismissal = {
+              chromeNotificationId: chromeNotificationId,
+              time: Date.now(),
+              dismissalData: dismissalResult.dismissals[i]
+            };
+            items.pendingDismissals.push(dismissal);
+          }
 
-      chrome.storage.local.set({
-        pendingDismissals: items.pendingDismissals,
-        notificationsData: items.notificationsData
-      });
+          items.notificationsData[chromeNotificationId] =
+              dismissalResult.notificationData;
 
-      processPendingDismissals(function(success) {});
-    });
+          chrome.storage.local.set(items);
+
+          processPendingDismissals(function(success) {});
+        });
   });
 }
 
@@ -1125,7 +1143,9 @@ instrumented.runtime.onStartup.addListener(function() {
       /** @type {Object.<string, StoredNotificationGroup>} */
       items.notificationGroups = items.notificationGroups || {};
 
-      combineAndShowNotificationCards(items.notificationGroups);
+      combineAndShowNotificationCards(items.notificationGroups, function() {
+        chrome.storage.local.set(items);
+      });
     });
   });
 
diff --git a/chrome/browser/resources/google_now/cards.js b/chrome/browser/resources/google_now/cards.js
index 547730f..24df10b 100644
--- a/chrome/browser/resources/google_now/cards.js
+++ b/chrome/browser/resources/google_now/cards.js
@@ -183,12 +183,14 @@ function buildCardSet() {
    * based on the current time and show-hide intervals in the combined card.
    * @param {ChromeNotificationId} cardId Card ID.
    * @param {CombinedCard} combinedCard Combined cards with |cardId|.
+   * @param {Object.<string, StoredNotificationGroup>} notificationGroups
+   *     Map from group name to group information.
    * @param {function(ReceivedNotification)=} onCardShown Optional parameter
    *     called when each card is shown.
    * @return {(NotificationDataEntry|undefined)} Notification data entry for
    *     this card. It's 'undefined' if the card's life is over.
    */
-  function update(cardId, combinedCard, onCardShown) {
+  function update(cardId, combinedCard, notificationGroups, onCardShown) {
     console.log('cardManager.update ' + JSON.stringify(combinedCard));
 
     chrome.alarms.clear(alarmPrefix + cardId);
@@ -254,7 +256,7 @@ function buildCardSet() {
       // If there are no more events, we are done with this card. Note that all
       // received notifications have hideTime.
       verify(!winningCard, 'No events left, but card is shown.');
-      clearCardFromGroups(cardId);
+      clearCardFromGroups(cardId, notificationGroups);
       return undefined;
     }
   }
@@ -265,12 +267,14 @@ function buildCardSet() {
    * @param {ChromeNotificationId} cardId Card ID.
    * @param {NotificationDataEntry} notificationData Stored notification entry
    *     for this card.
+   * @param {Object.<string, StoredNotificationGroup>} notificationGroups
+   *     Map from group name to group information.
    * @return {{
    *   dismissals: Array.<DismissalData>,
    *   notificationData: (NotificationDataEntry|undefined)
    * }}
    */
-  function onDismissal(cardId, notificationData) {
+  function onDismissal(cardId, notificationData, notificationGroups) {
     var dismissals = [];
     var newCombinedCard = [];
 
@@ -293,34 +297,27 @@ function buildCardSet() {
 
     return {
       dismissals: dismissals,
-      notificationData: update(cardId, newCombinedCard)
+      notificationData: update(cardId, newCombinedCard, notificationGroups)
     };
   }
 
   /**
-   * Removes card information from 'notificationGroups'.
+   * Removes card information from |notificationGroups|.
    * @param {ChromeNotificationId} cardId Card ID.
+   * @param {Object.<string, StoredNotificationGroup>} notificationGroups
+   *     Map from group name to group information.
    */
-  function clearCardFromGroups(cardId) {
+  function clearCardFromGroups(cardId, notificationGroups) {
     console.log('cardManager.clearCardFromGroups ' + cardId);
-
-    instrumented.storage.local.get('notificationGroups', function(items) {
-      items = items || {};
-      /** @type {Object.<string, StoredNotificationGroup>} */
-      items.notificationGroups = items.notificationGroups || {};
-
-      for (var groupName in items.notificationGroups) {
-        var group = items.notificationGroups[groupName];
-        for (var i = 0; i != group.cards.length; ++i) {
-          if (group.cards[i].chromeNotificationId == cardId) {
-            group.cards.splice(i, 1);
-            break;
-          }
+    for (var groupName in notificationGroups) {
+      var group = notificationGroups[groupName];
+      for (var i = 0; i != group.cards.length; ++i) {
+        if (group.cards[i].chromeNotificationId == cardId) {
+          group.cards.splice(i, 1);
+          break;
         }
       }
-
-      chrome.storage.local.set(items);
-    });
+    }
   }
 
   instrumented.alarms.onAlarm.addListener(function(alarm) {
@@ -330,26 +327,35 @@ function buildCardSet() {
       // Alarm to show the card.
       tasks.add(UPDATE_CARD_TASK_NAME, function() {
         var cardId = alarm.name.substring(alarmPrefix.length);
-        instrumented.storage.local.get('notificationsData', function(items) {
-          console.log('cardManager.onAlarm.get ' + JSON.stringify(items));
-          items = items || {};
-          /** @type {Object.<string, NotificationDataEntry>} */
-          items.notificationsData = items.notificationsData || {};
-          var combinedCard =
-            (items.notificationsData[cardId] &&
-             items.notificationsData[cardId].combinedCard) || [];
-
-          var cardShownCallback = undefined;
-          if (localStorage['locationCardsShown'] <
-              LOCATION_CARDS_LINK_THRESHOLD) {
-             cardShownCallback = countLocationCard;
-          }
-
-          items.notificationsData[cardId] =
-              update(cardId, combinedCard, cardShownCallback);
-
-          chrome.storage.local.set(items);
-        });
+        instrumented.storage.local.get(
+            ['notificationsData', 'notificationGroups'],
+            function(items) {
+              console.log('cardManager.onAlarm.get ' + JSON.stringify(items));
+              items = items || {};
+              /** @type {Object.<string, NotificationDataEntry>} */
+              items.notificationsData = items.notificationsData || {};
+              /** @type {Object.<string, StoredNotificationGroup>} */
+              items.notificationGroups = items.notificationGroups || {};
+
+              var combinedCard =
+                (items.notificationsData[cardId] &&
+                 items.notificationsData[cardId].combinedCard) || [];
+
+              var cardShownCallback = undefined;
+              if (localStorage['locationCardsShown'] <
+                  LOCATION_CARDS_LINK_THRESHOLD) {
+                 cardShownCallback = countLocationCard;
+              }
+
+              items.notificationsData[cardId] =
+                  update(
+                      cardId,
+                      combinedCard,
+                      items.notificationGroups,
+                      cardShownCallback);
+
+              chrome.storage.local.set(items);
+            });
       });
     }
   });