|
|
Created:
7 years, 1 month ago by vadimt Modified:
7 years, 1 month ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPolling the server upon receiving push messages.
Creating a general mechanism for this.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231712
Patch Set 1 #Patch Set 2 : Cosmetics #
Total comments: 6
Patch Set 3 : CR comments #
Total comments: 6
Patch Set 4 : robliao@ comments. #
Total comments: 2
Patch Set 5 : More comments. #Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1061: chrome.storage.local.set( Can this be abused? If pushMessaging for some reason floods us with requests, how do we handle this?
https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1061: chrome.storage.local.set( On 2013/10/28 22:54:50, Robert Liao wrote: > Can this be abused? If pushMessaging for some reason floods us with requests, > how do we handle this? We'll ask pushMessaging folks to fix the bug. I don't think we need to do anything else.
https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1061: chrome.storage.local.set( Can we narrow the scope down? We may not want all push messages in the future to trigger an update. On 2013/10/28 23:01:10, vadimt wrote: > On 2013/10/28 22:54:50, Robert Liao wrote: > > Can this be abused? If pushMessaging for some reason floods us with requests, > > how do we handle this? > > We'll ask pushMessaging folks to fix the bug. I don't think we need to do > anything else.
https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1061: chrome.storage.local.set( On 2013/10/28 23:43:37, Robert Liao wrote: > Can we narrow the scope down? We may not want all push messages in the future to > trigger an update. > On 2013/10/28 23:01:10, vadimt wrote: > > On 2013/10/28 22:54:50, Robert Liao wrote: > > > Can this be abused? If pushMessaging for some reason floods us with > requests, > > > how do we handle this? > > > > We'll ask pushMessaging folks to fix the bug. I don't think we need to do > > anything else. > you could enforce that it start with PUSH, or be PUSH[\d]+
> Can we narrow the scope down? We may not want all push messages in the future to trigger an update. As written, all 4 channels are reserved for causing a poll. If we want to change this, we'll have to change the client. Reserving channels now won't help anything. As for the "crazy push messages" scenario, please participate in the email discussion I've started (assuming that you still have concerns).
https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1061: chrome.storage.local.set( As discussed in the email, a known payload string would guard against unsolicited messages and a version prevents repeats. These seem like good defenses here. On 2013/10/28 23:50:24, Travis Skare wrote: > On 2013/10/28 23:43:37, Robert Liao wrote: > > Can we narrow the scope down? We may not want all push messages in the future > to > > trigger an update. > > On 2013/10/28 23:01:10, vadimt wrote: > > > On 2013/10/28 22:54:50, Robert Liao wrote: > > > > Can this be abused? If pushMessaging for some reason floods us with > > requests, > > > > how do we handle this? > > > > > > We'll ask pushMessaging folks to fix the bug. I don't think we need to do > > > anything else. > > > > you could enforce that it start with PUSH, or be PUSH[\d]+
https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/30001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1061: chrome.storage.local.set( On 2013/10/28 23:50:24, Travis Skare wrote: > On 2013/10/28 23:43:37, Robert Liao wrote: > > Can we narrow the scope down? We may not want all push messages in the future > to > > trigger an update. > > On 2013/10/28 23:01:10, vadimt wrote: > > > On 2013/10/28 22:54:50, Robert Liao wrote: > > > > Can this be abused? If pushMessaging for some reason floods us with > > requests, > > > > how do we handle this? > > > > > > We'll ask pushMessaging folks to fix the bug. I don't think we need to do > > > anything else. > > > > you could enforce that it start with PUSH, or be PUSH[\d]+ If we want another message in the future to do something else, the old clients will always respond to it will a poll, no? That seems bad. We should allow some way for future messages to do different things and the older clients will ignore it. Checking payload seems like a better choice since there are more than 4 available choices.
Looks like everyone would be happy if I checked payloads. Doing do.
https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1088: items.lastPollNowPayloads = items.lastPollNowPayloads || {}; The previous CR expressed this better without early returns. Since you rely on an additional existance of lastPollNowPayload, you can do this: if (items && items.lastPollNowPayload && items.lastPollNowPayload[message.subchannelId] != messagePayload) { /// Rest of Code }
https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1088: items.lastPollNowPayloads = items.lastPollNowPayloads || {}; On 2013/10/29 02:25:20, Robert Liao wrote: > The previous CR expressed this better without early returns. > Since you rely on an additional existance of lastPollNowPayload, you can do > this: > > if (items && items.lastPollNowPayload && > items.lastPollNowPayload[message.subchannelId] != messagePayload) { > /// Rest of Code > } I want it to do nothing if !items (i.e. if there are errors in get function). However, I want the code to work if !items.lastPollNowPayload; i.e. when push message comes for the very first time. Your condition works differently. Hence the code.
https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1088: items.lastPollNowPayloads = items.lastPollNowPayloads || {}; Most of the code instances fill in items with an empty object if it is undefined unless it violates a critical assumption (like in onNotificationClicked where notifications are expected to be there). What's the issue of us simply filling in items with an empty object? On 2013/10/29 02:55:18, vadimt wrote: > On 2013/10/29 02:25:20, Robert Liao wrote: > > The previous CR expressed this better without early returns. > > Since you rely on an additional existance of lastPollNowPayload, you can do > > this: > > > > if (items && items.lastPollNowPayload && > > items.lastPollNowPayload[message.subchannelId] != messagePayload) { > > /// Rest of Code > > } > > I want it to do nothing if !items (i.e. if there are errors in get function). > However, I want the code to work if !items.lastPollNowPayload; i.e. when push > message comes for the very first time. Your condition works differently. Hence > the code.
https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1088: items.lastPollNowPayloads = items.lastPollNowPayloads || {}; On 2013/10/29 17:06:30, Robert Liao wrote: > Most of the code instances fill in items with an empty object if it is undefined > unless it violates a critical assumption (like in onNotificationClicked where > notifications are expected to be there). > > What's the issue of us simply filling in items with an empty object? > > On 2013/10/29 02:55:18, vadimt wrote: > > On 2013/10/29 02:25:20, Robert Liao wrote: > > > The previous CR expressed this better without early returns. > > > Since you rely on an additional existance of lastPollNowPayload, you can do > > > this: > > > > > > if (items && items.lastPollNowPayload && > > > items.lastPollNowPayload[message.subchannelId] != messagePayload) { > > > /// Rest of Code > > > } > > > > I want it to do nothing if !items (i.e. if there are errors in get function). > > However, I want the code to work if !items.lastPollNowPayload; i.e. when push > > message comes for the very first time. Your condition works differently. Hence > > the code. > We want to request cards only if the payload changes. If payload is same, we don't want to poll. instrumented.storage.local.get returns 'undefined' only if there was an error. This means even if we fill in the object, 'storage.set' will likely fail. We'll probably end up getting same payload on every Chrome startup, filling in the object, polling cards and failing to set the last value. This is why it's safer to do nothing if storage.get returns error. Note that when running for the first time in a normal case, items will be !== undefined, but items.lastPollNowPayload === undefined, and we'll happily fill it in.
https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1088: items.lastPollNowPayloads = items.lastPollNowPayloads || {}; It's worth adding some of the above discussion to the existing comment above. On 2013/10/29 17:46:30, vadimt wrote: > On 2013/10/29 17:06:30, Robert Liao wrote: > > Most of the code instances fill in items with an empty object if it is > undefined > > unless it violates a critical assumption (like in onNotificationClicked where > > notifications are expected to be there). > > > > What's the issue of us simply filling in items with an empty object? > > > > On 2013/10/29 02:55:18, vadimt wrote: > > > On 2013/10/29 02:25:20, Robert Liao wrote: > > > > The previous CR expressed this better without early returns. > > > > Since you rely on an additional existance of lastPollNowPayload, you can > do > > > > this: > > > > > > > > if (items && items.lastPollNowPayload && > > > > items.lastPollNowPayload[message.subchannelId] != messagePayload) { > > > > /// Rest of Code > > > > } > > > > > > I want it to do nothing if !items (i.e. if there are errors in get > function). > > > However, I want the code to work if !items.lastPollNowPayload; i.e. when > push > > > message comes for the very first time. Your condition works differently. > Hence > > > the code. > > > > We want to request cards only if the payload changes. If payload is same, we > don't want to poll. > instrumented.storage.local.get returns 'undefined' only if there was an error. > This means even if we fill in the object, 'storage.set' will likely fail. We'll > probably end up getting same payload on every Chrome startup, filling in the > object, polling cards and failing to set the last value. This is why it's safer > to do nothing if storage.get returns error. > Note that when running for the first time in a normal case, items will be !== > undefined, but items.lastPollNowPayload === undefined, and we'll happily fill it > in.
https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/140001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1088: items.lastPollNowPayloads = items.lastPollNowPayloads || {}; On 2013/10/29 17:56:59, Robert Liao wrote: > It's worth adding some of the above discussion to the existing comment above. > On 2013/10/29 17:46:30, vadimt wrote: > > On 2013/10/29 17:06:30, Robert Liao wrote: > > > Most of the code instances fill in items with an empty object if it is > > undefined > > > unless it violates a critical assumption (like in onNotificationClicked > where > > > notifications are expected to be there). > > > > > > What's the issue of us simply filling in items with an empty object? > > > > > > On 2013/10/29 02:55:18, vadimt wrote: > > > > On 2013/10/29 02:25:20, Robert Liao wrote: > > > > > The previous CR expressed this better without early returns. > > > > > Since you rely on an additional existance of lastPollNowPayload, you can > > do > > > > > this: > > > > > > > > > > if (items && items.lastPollNowPayload && > > > > > items.lastPollNowPayload[message.subchannelId] != messagePayload) { > > > > > /// Rest of Code > > > > > } > > > > > > > > I want it to do nothing if !items (i.e. if there are errors in get > > function). > > > > However, I want the code to work if !items.lastPollNowPayload; i.e. when > > push > > > > message comes for the very first time. Your condition works differently. > > Hence > > > > the code. > > > > > > > We want to request cards only if the payload changes. If payload is same, we > > don't want to poll. > > instrumented.storage.local.get returns 'undefined' only if there was an error. > > This means even if we fill in the object, 'storage.set' will likely fail. > We'll > > probably end up getting same payload on every Chrome startup, filling in the > > object, polling cards and failing to set the last value. This is why it's > safer > > to do nothing if storage.get returns error. > > Note that when running for the first time in a normal case, items will be !== > > undefined, but items.lastPollNowPayload === undefined, and we'll happily fill > it > > in. > Done.
LGTM with one comment. https://codereview.chromium.org/48273007/diff/230001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/230001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1086: // If storage.get fails, it's safer to do nothing. Modify: it's safer to do nothing, preventing polling the server when the payload really didn't change.
https://codereview.chromium.org/48273007/diff/230001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/48273007/diff/230001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1086: // If storage.get fails, it's safer to do nothing. On 2013/10/29 18:09:41, Robert Liao wrote: > Modify: it's safer to do nothing, preventing polling the server when the payload > really didn't change. Done.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/48273007/280001
Message was sent while issue was closed.
Change committed as 231712 |