Chrome Now notificationGroups Storage Race Condition Fix

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240798

[rgustafson, 2013-12-12 22:52:27]

lgtm

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background.js:542:
recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
tiny comment: This should probably be before the storage.set.

[robliao, 2013-12-12 23:01:20]

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background.js:542:
recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
Indeed. I was going to save this for the unchained refactor.
On 2013/12/12 22:52:27, rgustafson wrote:
> tiny comment: This should probably be before the storage.set.

[vadimt, 2013-12-13 18:20:54]

Thanks for taking this!
Below are some notes.

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background.js:328:
chrome.storage.local.set({notificationsData: notificationsData});
Please set notificationGroups here. getAll is an async call, and setting it from
parseAndShowNotificationCards will set groups not yet updated by
cardSet.update() calls.

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/cards.js:298: notificationData:
update(cardId, newCombinedCard)
Please pass notificationGroups here too.

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/cards.js:344: update(cardId, combinedCard,
cardShownCallback);
Please pass notificationGroups here too.

[robliao, 2013-12-13 19:31:26]

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background.js:328:
chrome.storage.local.set({notificationsData: notificationsData});
Took a different approach here and delegated that responsibility to the caller.
On 2013/12/13 18:20:54, vadimt wrote:
> Please set notificationGroups here. getAll is an async call, and setting it
from
> parseAndShowNotificationCards will set groups not yet updated by
> cardSet.update() calls.

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background.js:542:
recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
Well, now it does.
On 2013/12/12 23:01:21, robliao wrote:
> Indeed. I was going to save this for the unchained refactor.
> On 2013/12/12 22:52:27, rgustafson wrote:
> > tiny comment: This should probably be before the storage.set.
>

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/cards.js:298: notificationData:
update(cardId, newCombinedCard)
On 2013/12/13 18:20:54, vadimt wrote:
> Please pass notificationGroups here too.

Done.

https://codereview.chromium.org/114533002/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/cards.js:344: update(cardId, combinedCard,
cardShownCallback);
On 2013/12/13 18:20:54, vadimt wrote:
> Please pass notificationGroups here too.

Done.

[vadimt, 2013-12-13 20:07:39]

https://codereview.chromium.org/114533002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/114533002/diff/20001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:543:
combineAndShowNotificationCards(updatedGroups,
nit: please start "updatedGroups," from new line

https://codereview.chromium.org/114533002/diff/20001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:1145:
combineAndShowNotificationCards(items.notificationGroups, function() {});
We need to store notificationGroups here too, in case when all notifications in
the combined card have expired, and get deleted from groups.

[robliao, 2013-12-13 20:38:17]

https://codereview.chromium.org/114533002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/114533002/diff/20001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:543:
combineAndShowNotificationCards(updatedGroups,
On 2013/12/13 20:07:40, vadimt wrote:
> nit: please start "updatedGroups," from new line

Done.

https://codereview.chromium.org/114533002/diff/20001/chrome/browser/resources...
chrome/browser/resources/google_now/background.js:1145:
combineAndShowNotificationCards(items.notificationGroups, function() {});
Ah. I for some reason thought we didn't need this. That's easy enough.
On 2013/12/13 20:07:40, vadimt wrote:
> We need to store notificationGroups here too, in case when all notifications
in
> the combined card have expired, and get deleted from groups.

[vadimt, 2013-12-13 20:39:32]

lgtm

[commit-bot: I haz the power, 2013-12-13 20:43:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/114533002/40001

[commit-bot: I haz the power, 2013-12-13 23:30:54]

Change committed as 240798