Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1023473003:
Allow BackoffEntry to be serialized and deserialized. (Closed)
DescriptionAllow BackoffEntry to be serialized and deserialized.
This will be used by push messaging for https://crbug.com/465399,
where we need to continue retrying unregistration even if the browser
gets killed and restarted.
BUG=465399
Committed: https://crrev.com/6727a62ae541389ec4a5264b279485d75e2c4fe5
Cr-Commit-Position: refs/heads/master@{#328755}
Patch Set 1 #Patch Set 2 : Factor out a BackoffRequestSerializer class #Patch Set 3 : Tweak comments #
Total comments: 27
Patch Set 4 : Addressed mmenke's review comments #
Total comments: 25
Patch Set 5 : Addressed mmenke's review nits #
Total comments: 6
Patch Set 6 : Address mmenke's final review nits #
Messages
Total messages: 23 (4 generated)
johnme@chromium.org changed reviewers: + rsleevi@chromium.org
This patch introduces some potential for integer overflows; I've uploaded a follow-up patch which fixes them: https://codereview.chromium.org/1039263003 (it seemed clearest to do that as a separate patch, since the overflow checking makes the fixed patch significantly harder to read, and it seemed useful to have the easier to read version preserved in git history to aid understanding).
rsleevi@chromium.org changed reviewers: + davidben@chromium.org, mmenke@chromium.org - rsleevi@chromium.org
My inclination is not LG, but I'm going to redirect to mmenke and davidben who have thought much more on this problem of Backoff Entries and the design. Reasoning: 1) Arbitrary serialization/deserialization tends to run up against https://www.chromium.org/Home/chromium-security/education/security-tips-for-i.... - definitely, this needs to be discussed with chrome-security during your security-review 2) This immediately raises a code-smell for "single responsibility principle" - I'm not keen on coupling the object to serialization state. Historically, that has ALWAYS backfired. Using a dedicated serializer that doesn't reach into any internal state seems generally better, even if it means looser coupling That your design requires this at all causes me to raise my eyebrow; it'd be much nicer if there was a design-doc that showed how this fit in to the overall picture of what you're trying to accomplish. I think the trend of sharing design docs on such stuff on chromium-dev has been great, and these design docs help both explain context for reviewers, make your security review go easier, and work good for performance packets when you need ;) This isn't a hard requirement, just a strong suggestion to consider. But I'm going to punt this to David and Matt, who are much more familiar with the subsystems at play and the design considerations, and have a better sense of whether my unease is unfounded.
On 2015/03/27 20:26:45, Ryan Sleevi wrote: > My inclination is not LG, but I'm going to redirect to mmenke and davidben who > have thought much more on this problem of Backoff Entries and the design. > > Reasoning: > 1) Arbitrary serialization/deserialization tends to run up against > https://www.chromium.org/Home/chromium-security/education/security-tips-for-i.... > - definitely, this needs to be discussed with chrome-security during your > security-review > > 2) This immediately raises a code-smell for "single responsibility principle" - > I'm not keen on coupling the object to serialization state. Historically, that > has ALWAYS backfired. Using a dedicated serializer that doesn't reach into any > internal state seems generally better, even if it means looser coupling > > That your design requires this at all causes me to raise my eyebrow; it'd be > much nicer if there was a design-doc that showed how this fit in to the overall > picture of what you're trying to accomplish. I think the trend of sharing design > docs on such stuff on chromium-dev has been great, and these design docs help > both explain context for reviewers, make your security review go easier, and > work good for performance packets when you need ;) This isn't a hard > requirement, just a strong suggestion to consider. > > But I'm going to punt this to David and Matt, who are much more familiar with > the subsystems at play and the design considerations, and have a better sense of > whether my unease is unfounded. I agree on code smell here. Is it really such a problem if we just restart from scratch? If you're concerned about being too aggressive on retry, you can change the inittial retry time you're using on resumption.
Ryan wrote: > Arbitrary serialization/deserialization tends to run up against > chromium.org/Home/chromium-security/education/security-tips-for-ipc I'm not sure I get the relevance? This isn't going over IPC, it's being serialized to disk (prefs). > it'd be much nicer if there was a design-doc that showed how this fit in > to the overall picture of what you're trying to accomplish. I've written a design doc: https://docs.google.com/document/d/1QX963RvFzQUcRhNF9vJ6G-JU4g1vaPif79n-m55y5... The most relevant section is "Unsubscription auto-retry with backoff" at the bottom, but the others provide useful context too. > Using a dedicated serializer that doesn't reach into any internal state seems generally better. Sure, I'd be happy to refactor this out to a BackoffEntrySerializer class or similar. I think adding a SetFailureCount method to BackoffEntry would be sufficient to allow that. mmenke wrote: > Is it really such a problem if we just restart from > scratch? If you're concerned about being too aggressive on retry, you can > change the inittial retry time you're using on resumption. On Android, Chrome can be started and killed very frequently (on low-end devices, this could happen every time you switch apps!), such that restarting from scratch whenever the browser is started might be equivalent to retrying periodically with no backoff at all. Not only would this risk DDOSing GCM or wasting battery, but it could cause us to quickly reach our maximum number of retries (whatever we choose to set this to), just because Chrome gets launched a few times while the device is offline (with Service Workers [or appcache], there exist webapps that can usefully be used offline, and we're hoping to encourage this with Fizz). Instead, it seems better to remember the backoff state across browser restarts, as described in the "Unsubscription auto-retry with backoff" section of the doc I linked to above. Thanks for your comments!
On 2015/03/30 20:23:24, johnme wrote: > Ryan wrote: > > Arbitrary serialization/deserialization tends to run up against > > chromium.org/Home/chromium-security/education/security-tips-for-ipc > > I'm not sure I get the relevance? This isn't going over IPC, it's being > serialized to disk (prefs). > > > it'd be much nicer if there was a design-doc that showed how this fit in > > to the overall picture of what you're trying to accomplish. > > I've written a design doc: > https://docs.google.com/document/d/1QX963RvFzQUcRhNF9vJ6G-JU4g1vaPif79n-m55y5... > > The most relevant section is "Unsubscription auto-retry with backoff" at the > bottom, but the others provide useful context too. > > > Using a dedicated serializer that doesn't reach into any > internal state seems generally better. > > Sure, I'd be happy to refactor this out to a BackoffEntrySerializer class or > similar. I think adding a SetFailureCount method to BackoffEntry would be > sufficient to allow that. > > mmenke wrote: > > Is it really such a problem if we just restart from > > scratch? If you're concerned about being too aggressive on retry, you can > > change the inittial retry time you're using on resumption. > > On Android, Chrome can be started and killed very frequently (on low-end > devices, this could happen every time you switch apps!), such that restarting > from scratch whenever the browser is started might be equivalent to retrying > periodically with no backoff at all. > > Not only would this risk DDOSing GCM or wasting battery, but it could cause us > to quickly reach our maximum number of retries (whatever we choose to set this > to), just because Chrome gets launched a few times while the device is offline > (with Service Workers [or appcache], there exist webapps that can usefully be > used offline, and we're hoping to encourage this with Fizz). > > Instead, it seems better to remember the backoff state across browser restarts, > as described in the "Unsubscription auto-retry with backoff" section of the doc > I linked to above. > > Thanks for your comments! So, thinking some more about this, BackoffEntry has become the de-facto way for various Chrome components to do exponential backoff, particularly for internal Chrome network requests. The ability to resume backoff between browser restarts does not seem unreasonable. GCM could just serialize the information it wants, and then pass it back to the BackoffEntry on reload, without changing the BackoffEntry class. This works fine, but I'd really rather not have a couple different implementations of the logic (Given the popularity of BackoffEntry, I suspect it's just a matter of when someone else decides this is needed for their component), so I think modifying BackoffEntry is a reasonable approach. I'll take a look at the code tomorrow. I do think we should have some sort of sanity check - if we get a time 15 decades in the future, we really don't want to eternally block something from happening.
On 2015/03/31 21:09:15, mmenke wrote: > The ability to resume backoff between browser restarts > does not seem unreasonable. Agreed > so I think modifying BackoffEntry is a reasonable approach. I'm a little uncertain about this. It seems to be encapsulating too much logic into the BackoffEntry - that is, knowledge about serialization formats and the constraints that go into those decisions. Would it make sense to decouple the serialization logic? Part of the concern comes from the fact that we have concrete subclasses of this (for reasons I'm not entirely clear on), and it seems weird to encapsulate this in a base class. That said, it seems all the subclasses are just overloading the Now functionality? Perhaps we should make that an explicit parameter so as to discourage any subclasses, which effectively leaves Policy as a POD and the only other data being 'time until next retry' and 'failure count' - and I think both of those need more thought as to whether we deserialize. > I'll take a look at the code tomorrow. I do think we should have some sort of > sanity check - if we get a time 15 decades in the future, we really don't want > to eternally block something from happening.
On 2015/03/31 21:32:26, Ryan Sleevi wrote: > On 2015/03/31 21:09:15, mmenke wrote: > > The ability to resume backoff between browser restarts > > does not seem unreasonable. > > Agreed > > > so I think modifying BackoffEntry is a reasonable approach. > > I'm a little uncertain about this. It seems to be encapsulating too much logic > into the BackoffEntry - that is, knowledge about serialization formats and the > constraints that go into those decisions. > > Would it make sense to decouple the serialization logic? Part of the concern > comes from the fact that we have concrete subclasses of this (for reasons I'm > not entirely clear on), and it seems weird to encapsulate this in a base class. > > That said, it seems all the subclasses are just overloading the Now > functionality? Perhaps we should make that an explicit parameter so as to > discourage any subclasses, which effectively leaves Policy as a POD and the only > other data being 'time until next retry' and 'failure count' - and I think both > of those need more thought as to whether we deserialize. I think that sounds reasonable. > > I'll take a look at the code tomorrow. I do think we should have some sort of > > sanity check - if we get a time 15 decades in the future, we really don't want > > to eternally block something from happening.
On 2015/03/31 21:32:26, Ryan Sleevi wrote: > On 2015/03/31 21:09:15, mmenke wrote: > > so I think modifying BackoffEntry is a reasonable approach. > > I'm a little uncertain about this. It seems to be encapsulating too much logic > into the BackoffEntry - that is, knowledge about serialization formats and the > constraints that go into those decisions. > > Would it make sense to decouple the serialization logic? Part of the concern > comes from the fact that we have concrete subclasses of this (for reasons I'm > not entirely clear on), and it seems weird to encapsulate this in a base class. > > That said, it seems all the subclasses are just overloading the Now > functionality? Perhaps we should make that an explicit parameter so as to > discourage any subclasses, which effectively leaves Policy as a POD and the only > other data being 'time until next retry' and 'failure count' - and I think both > of those need more thought as to whether we deserialize. I could refactor BackoffRequest to remove ImplGetTimeNow and have a SetTickClock method that can be used instead of subclassing it. Is that what you mean by "make that an explicit parameter" (or do you mean to add a TimeTicks parameter to InformOfRequest, ShouldRejectRequest, GetTimeUntilRelease, and CanDiscard, which sounds more intrusive)? Then if you want I could split off the serialization from this patch to a BackoffRequestSerializer class (which would have static Serialize/Deserialize methods which each take both a TimeTicks and a Time as parameters). Does that sound better? It sounds like you'd like me to serialize the Policy as well, which my patch doesn't currently do. Sounds reasonable I guess, though I'd probably have to include a version number in the serialization, and any future changes to Policy would require some form of migration - is this a route we want to go down? Finally, can I check what you mean by "I think ['time until next retry' and 'failure count'] need more thought as to whether we deserialize."? Serializing those is pretty fundamental to this patch :)
On 2015/04/01 19:09:02, johnme wrote: > I could refactor BackoffRequest to remove ImplGetTimeNow and have a SetTickClock > method that can be used instead of subclassing it. Is that what you mean by > "make that an explicit parameter" (or do you mean to add a TimeTicks parameter > to InformOfRequest, ShouldRejectRequest, GetTimeUntilRelease, and CanDiscard, > which sounds more intrusive)? I meant pass a TickClock in to the constructor. Or you could have a "SetTickClockForTesting". The former (ctor) feels more idiomatic, in that it's explicitly expressing the dependencies for dep injection, rather than covertly, and either way, the class needs to have a scoped_ptr<TickClock> clock_ member. > Then if you want I could split off the serialization from this patch to a > BackoffRequestSerializer class (which would have static Serialize/Deserialize > methods which each take both a TimeTicks and a Time as parameters). I don't know if you actually need TimeTicks/Time as parameters. Perhaps I missed something? > It sounds like you'd like me to serialize the Policy as well, which my patch > doesn't currently do. I don't have strong feelings if this is decoupled from the BackoffEntry, just that it's clear in whatever class you do whether or not you serialize the Policy. The concern is just that right now the BackoffEntry is non-copyable, and the Policy set on construction, so you're guaranteed that a BackoffEntry always has the same policy as when it was constructed. Your serialization is effectively the same as making the object Copy-Constructible (since I can always bounce through a Serialize/Deserialize), in which case, it's now possible to have Policy mismatches. > Sounds reasonable I guess, though I'd probably have to > include a version number in the serialization, and any future changes to Policy > would require some form of migration - is this a route we want to go down? *shrug* The common problem of all serializers :) > Finally, can I check what you mean by "I think ['time until next retry' and > 'failure count'] need more thought as to whether we deserialize."? Serializing > those is pretty fundamental to this patch :) Serializing time, rather than time deltas, is almost always a Big Bad Thing. I haven't looked too closely at this patch, other than to note that it's generally wrong.
Refactored as suggested. PTAL. On 2015/04/09 01:01:02, Ryan Sleevi wrote: > On 2015/04/01 19:09:02, johnme wrote: > > I could refactor BackoffRequest to remove ImplGetTimeNow and have a > SetTickClock > > method that can be used instead of subclassing it. Is that what you mean by > > "make that an explicit parameter" (or do you mean to add a TimeTicks parameter > > to InformOfRequest, ShouldRejectRequest, GetTimeUntilRelease, and CanDiscard, > > which sounds more intrusive)? > > I meant pass a TickClock in to the constructor. Or you could have a > "SetTickClockForTesting". The former (ctor) feels more idiomatic, in that it's > explicitly expressing the dependencies for dep injection, rather than covertly, > and either way, the class needs to have a scoped_ptr<TickClock> clock_ member. Done in https://codereview.chromium.org/1076853003 > > Then if you want I could split off the serialization from this patch to a > > BackoffRequestSerializer class (which would have static Serialize/Deserialize > > methods which each take both a TimeTicks and a Time as parameters). Done. > I don't know if you actually need TimeTicks/Time as parameters. Perhaps I missed > something? I ended up re-using BackoffEntry's TickClock, so I only had to pass the Time. In practice, you should always pass base::Time::Now() except for testing. Alternatively I could make BackoffRequestSerializer non-static, taking a base::Clock as a parameter, but it seemed that making it non-static would make things harder rather than easier for callers. > > It sounds like you'd like me to serialize the Policy as well, which my patch > > doesn't currently do. > > I don't have strong feelings if this is decoupled from the BackoffEntry, just > that it's clear in whatever class you do whether or not you serialize the > Policy. Ok, I added comments emphasizing that the same Policy must be passed in. > The concern is just that right now the BackoffEntry is non-copyable, and the > Policy set on construction, so you're guaranteed that a BackoffEntry always has > the same policy as when it was constructed. Your serialization is effectively > the same as making the object Copy-Constructible (since I can always bounce > through a Serialize/Deserialize), in which case, it's now possible to have > Policy mismatches. > > > Sounds reasonable I guess, though I'd probably have to > > include a version number in the serialization, and any future changes to > Policy > > would require some form of migration - is this a route we want to go down? > > *shrug* The common problem of all serializers :) Ok, even though I'm not serializing the Policy, I do depend on it, so I've added a version number to the serialization format, and a comment to increment the version number if Policy ever gets changed. > > Finally, can I check what you mean by "I think ['time until next retry' and > > 'failure count'] need more thought as to whether we deserialize."? Serializing > > those is pretty fundamental to this patch :) > > Serializing time, rather than time deltas, is almost always a Big Bad Thing. I > haven't looked too closely at this patch, other than to note that it's generally > wrong. I've updated this so it now serializes both the delta and the absolute time. When deserializing, it recalculates the delta based on the difference between absolute times, then it uses min(calculated_delta, serialized_delta), which makes it relatively robust against the system wall clock jumping backwards It's not possible, however, to protect against the system wall clock jumping forwards, so in such cases the backoff release time can occur earlier than intended, but since the system clock jumping forward significantly is usually a one-off occurrence, this generally won't significantly affect the behavior of the backoff).
Haven't gone over the tests yet https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry.h File net/base/backoff_entry.h (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry.... net/base/backoff_entry.h:110: base::TimeDelta time_until_release) const; nit: Maybe time_until_release -> backoff (or backoff_[delta|time])? It seems weird to use "maximum_backoff" to limit "time_until_release". https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.cc (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:25: // We redundantly store both the remaining time delta and the absolute time. Avoid using "we" in comments. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:26: // This will let us work around some cases where wall clock time changes. Avoid using "us" in comments. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:29: serialized->AppendDouble(absolute_release_time.ToDoubleT()); Do we really need these both? Seems like we could just record absolute_release_time, and if it's greater than the value of CalculateReleaseTime(), just ignore absolute_release_time. Doesn't quite catch as much...but do we care? https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:41: return nullptr; Use braces when the condition of an if takes up more than 1 line. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:43: if (!serialized.GetInteger(1, &failure_count)) We should fail if this is negative. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:49: if (!serialized.GetDouble(3, &absolute_release_time_double)) We should fail if this is negative. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:52: entry->failure_count_ = failure_count; Erm...Could we just call InformOfRequest(false) failure_count times, make entry->failure_count() public, and then have BackoffEntry de-friend us? The loop isn't lovely, but not a big fan of friend classes unless we really need them. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:67: return entry; Think this method could use a couple blank lines, to make it more readable. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.h (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: New files shouldn't use the (c) (Same goes go other new files as well) https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:26: // Policy is not serialized, instead you'll need to pass an identical Policy* Don't use "you" in comments. Can just use passive voice here. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:49: DISALLOW_IMPLICIT_CONSTRUCTORS(BackoffEntrySerializer); include base/macros.h https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... File net/base/backoff_entry_unittest.cc (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_unittest.cc:326: TEST(BackoffEntryTest, SerializeTimeOffsets) { These should probably go in backoff_entry_serializer_unittest.cc
Addressed mmenke's review comments - thanks! https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry.h File net/base/backoff_entry.h (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry.... net/base/backoff_entry.h:110: base::TimeDelta time_until_release) const; On 2015/05/05 15:47:51, mmenke wrote: > nit: Maybe time_until_release -> backoff (or backoff_[delta|time])? It seems > weird to use "maximum_backoff" to limit "time_until_release". Ok, renamed time_until_release to backoff_duration everywhere. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.cc (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:25: // We redundantly store both the remaining time delta and the absolute time. On 2015/05/05 15:47:52, mmenke wrote: > Avoid using "we" in comments. Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:26: // This will let us work around some cases where wall clock time changes. On 2015/05/05 15:47:51, mmenke wrote: > Avoid using "us" in comments. Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:29: serialized->AppendDouble(absolute_release_time.ToDoubleT()); On 2015/05/05 15:47:52, mmenke wrote: > Do we really need these both? Seems like we could just record > absolute_release_time, and if it's greater than the value of > CalculateReleaseTime(), just ignore absolute_release_time. > > Doesn't quite catch as much...but do we care? I added serialization of the time_until_release (now called backoff_duration) in response to rsleevi's comment[1] that "Serializing time, rather than time deltas, is almost always a Big Bad Thing". This way we're safe against the system (wall) clock jumping backwards, which seems a useful protection (otherwise, if the system clock jumps backwards a lot, the best we could do is to set release time to now + maximum_backoff_ms, which may be excessively large). I'm not sure what you mean by "the value of CalculateReleaseTime()", since during deserialization it's up to us to set that value. Perhaps you mean "the value of CalculateReleaseTime() after calling InformOfRequest(false) failure_count times", but that calculated release time might be wrong, since BackoffEntry has a public SetCustomReleaseTime method that could have been used to override the release time. [1]: https://codereview.chromium.org/1023473003/#msg11 https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:41: return nullptr; On 2015/05/05 15:47:51, mmenke wrote: > Use braces when the condition of an if takes up more than 1 line. Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:43: if (!serialized.GetInteger(1, &failure_count)) On 2015/05/05 15:47:51, mmenke wrote: > We should fail if this is negative. Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:49: if (!serialized.GetDouble(3, &absolute_release_time_double)) On 2015/05/05 15:47:51, mmenke wrote: > We should fail if this is negative. Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:52: entry->failure_count_ = failure_count; On 2015/05/05 15:47:52, mmenke wrote: > Erm...Could we just call InformOfRequest(false) failure_count times, make > entry->failure_count() public, and then have BackoffEntry de-friend us? > > The loop isn't lovely, but not a big fan of friend classes unless we really need > them. Hmm, ok done, but note that I had to make TimeUntilReleaseToReleaseTime (now called BackoffDurationToReleaseTime) and tick_clock public (in addition to the expensive loop). https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:67: return entry; On 2015/05/05 15:47:52, mmenke wrote: > Think this method could use a couple blank lines, to make it more readable. Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.h (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/05/05 15:47:52, mmenke wrote: > nit: New files shouldn't use the (c) (Same goes go other new files as well) Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:26: // Policy is not serialized, instead you'll need to pass an identical Policy* On 2015/05/05 15:47:52, mmenke wrote: > Don't use "you" in comments. Can just use passive voice here. Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:49: DISALLOW_IMPLICIT_CONSTRUCTORS(BackoffEntrySerializer); On 2015/05/05 15:47:52, mmenke wrote: > include base/macros.h Done. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... File net/base/backoff_entry_unittest.cc (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_unittest.cc:326: TEST(BackoffEntryTest, SerializeTimeOffsets) { On 2015/05/05 15:47:52, mmenke wrote: > These should probably go in backoff_entry_serializer_unittest.cc Done.
Most of these are nits. Next pass should be fast, and hope to sign off then. https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.cc (right): https://codereview.chromium.org/1023473003/diff/40001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:29: serialized->AppendDouble(absolute_release_time.ToDoubleT()); On 2015/05/06 12:46:47, johnme wrote: > On 2015/05/05 15:47:52, mmenke wrote: > > Do we really need these both? Seems like we could just record > > absolute_release_time, and if it's greater than the value of > > CalculateReleaseTime(), just ignore absolute_release_time. > > > > Doesn't quite catch as much...but do we care? > > I added serialization of the time_until_release (now called backoff_duration) in > response to rsleevi's comment[1] that "Serializing time, rather than time > deltas, is almost always a Big Bad Thing". This way we're safe against the > system (wall) clock jumping backwards, which seems a useful protection > (otherwise, if the system clock jumps backwards a lot, the best we could do is > to set release time to now + maximum_backoff_ms, which may be excessively > large). > > I'm not sure what you mean by "the value of CalculateReleaseTime()", since > during deserialization it's up to us to set that value. Perhaps you mean "the > value of CalculateReleaseTime() after calling InformOfRequest(false) > failure_count times", but that calculated release time might be wrong, since > BackoffEntry has a public SetCustomReleaseTime method that could have been used > to override the release time. > > [1]: https://codereview.chromium.org/1023473003/#msg11 Ah, right. Forgot about SetCustomReleaseTime. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.cc (right): https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:12: const int SERIALIZATION_VERSION_NUMBER = 1; nit: kSerializationVersionNumber (Or maybe just kFormatVersion would be clearer, or kSerializationFormatVersion). https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:12: const int SERIALIZATION_VERSION_NUMBER = 1; Think this is worth a short comment https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:69: absolute_release_time - time_now; nit: Does this fit on one line? https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.h (right): https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:42: // deserialization was unsuccessful. nit: NULL is generally capitalized in net/ when it refers to a NULL pointer. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:44: const base::ListValue& serialized, Suggest base::Value here and above. Consumer probably shouldn't have to know or care what type of value it is. I assume there's no storage API you plan to use that demands list values, so this also makes it a little simpler on consumers - won't have to get a pointer to serialized as a ListValue, starting from a Value. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... File net/base/backoff_entry_serializer_unittest.cc (right): https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:42: DISALLOW_COPY_AND_ASSIGN(TestTickClock); nit: blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:68: Time later = now_time + TimeDelta::FromDays(1); I find these time names pretty confusing - we're deserializing when "now" is "later", which seems counter-intuitive to me. Maybe, in variable names: now -> original (Matching the corresponding BackoffEntry) later -> deserialization, also matching the corresponding BackoffEntry (Less concerned about this one). https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:75: deserialized->GetReleaseTime()); I think this is weird enough to be worth a comment. Unclear from context that "GetReleaseTime" returns TimeTicks, and since the day has advanced, then the TimeTicks of the release time should have gone backwards. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:75: deserialized->GetReleaseTime()); This is a negative TimeTicks value, which seems weird. Do we allow negative TimeTicks values elsewhere? Should we have a minimum of 0? https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:103: EXPECT_EQ(original.GetReleaseTime(), deserialized->GetReleaseTime()); Again, think this requires an explanation. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:125: TEST(BackoffEntrySerializerTest, SerializeNoFailures) { This is pretty much identical to the first block in SerializeTimeOffsets, except we're using a "time" of 0. Maybe run it using the current time and time ticks, to make it more interesting? Or could consider getting rid of it. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:125: TEST(BackoffEntrySerializerTest, SerializeNoFailures) { Suggest this test first. Think it's nice to have the more basic tests go before the tests that "depend" on them passing. Not as useful as it used to be, when tests were run in order, but still think it makes things clearer.
Addressed mmenke's review nits - thanks :) https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.cc (right): https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:12: const int SERIALIZATION_VERSION_NUMBER = 1; On 2015/05/06 17:34:48, mmenke wrote: > Think this is worth a short comment Done. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:12: const int SERIALIZATION_VERSION_NUMBER = 1; On 2015/05/06 17:34:48, mmenke wrote: > nit: kSerializationVersionNumber (Or maybe just kFormatVersion would be > clearer, or kSerializationFormatVersion). Done. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:69: absolute_release_time - time_now; On 2015/05/06 17:34:48, mmenke wrote: > nit: Does this fit on one line? Done. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.h (right): https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:42: // deserialization was unsuccessful. On 2015/05/06 17:34:48, mmenke wrote: > nit: NULL is generally capitalized in net/ when it refers to a NULL pointer. Done (I'd stopped doing this with C++11, as it's usually nullptr rather than the NULL macro. But happy to keep consistent with net/). https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer.h:44: const base::ListValue& serialized, On 2015/05/06 17:34:48, mmenke wrote: > Suggest base::Value here and above. Consumer probably shouldn't have to know or > care what type of value it is. I assume there's no storage API you plan to use > that demands list values, so this also makes it a little simpler on consumers - > won't have to get a pointer to serialized as a ListValue, starting from a Value. Done. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... File net/base/backoff_entry_serializer_unittest.cc (right): https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:42: DISALLOW_COPY_AND_ASSIGN(TestTickClock); On 2015/05/06 17:34:48, mmenke wrote: > nit: blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:68: Time later = now_time + TimeDelta::FromDays(1); On 2015/05/06 17:34:48, mmenke wrote: > I find these time names pretty confusing - we're deserializing when "now" is > "later", which seems counter-intuitive to me. Maybe, in variable names: > > now -> original (Matching the corresponding BackoffEntry) > later -> deserialization, also matching the corresponding BackoffEntry (Less > concerned about this one). Ok, I went with original_time, original_ticks, later_time, later_ticks, earlier_time. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:75: deserialized->GetReleaseTime()); On 2015/05/06 17:34:48, mmenke wrote: > I think this is weird enough to be worth a comment. Unclear from context that > "GetReleaseTime" returns TimeTicks, and since the day has advanced, then the > TimeTicks of the release time should have gone backwards. Done, I added details comments to all these cases. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:75: deserialized->GetReleaseTime()); On 2015/05/06 17:34:48, mmenke wrote: > This is a negative TimeTicks value, which seems weird. Do we allow negative > TimeTicks values elsewhere? Should we have a minimum of 0? Why is negative TimeTicks weird? If we assume that zero is when the OS booted up (not sure if that's actually true - it's probably platform-specific), it doesn't seem unreasonable to be able to represent times before that. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:103: EXPECT_EQ(original.GetReleaseTime(), deserialized->GetReleaseTime()); On 2015/05/06 17:34:48, mmenke wrote: > Again, think this requires an explanation. Done. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:125: TEST(BackoffEntrySerializerTest, SerializeNoFailures) { On 2015/05/06 17:34:48, mmenke wrote: > Suggest this test first. Think it's nice to have the more basic tests go before > the tests that "depend" on them passing. Not as useful as it used to be, when > tests were run in order, but still think it makes things clearer. Done. https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:125: TEST(BackoffEntrySerializerTest, SerializeNoFailures) { On 2015/05/06 17:34:48, mmenke wrote: > This is pretty much identical to the first block in SerializeTimeOffsets, except > we're using a "time" of 0. Maybe run it using the current time and time ticks, > to make it more interesting? Or could consider getting rid of it. Done. It now covers the case where a) there are no failures, and b) serialization times are both realistic (non-zero).
LGTM https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... File net/base/backoff_entry_serializer_unittest.cc (right): https://codereview.chromium.org/1023473003/diff/60001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:68: Time later = now_time + TimeDelta::FromDays(1); On 2015/05/06 20:38:09, johnme wrote: > On 2015/05/06 17:34:48, mmenke wrote: > > I find these time names pretty confusing - we're deserializing when "now" is > > "later", which seems counter-intuitive to me. Maybe, in variable names: > > > > now -> original (Matching the corresponding BackoffEntry) > > later -> deserialization, also matching the corresponding BackoffEntry (Less > > concerned about this one). > > Ok, I went with original_time, original_ticks, later_time, later_ticks, > earlier_time. SGTM https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.cc (right): https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:66: || absolute_release_time_us < 0) { || should go on the previous line, I believe. You are running "git cl format net", right? Should get a presubmit error if you don't. Fine if this is the code that results in. https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... File net/base/backoff_entry_serializer_unittest.cc (right): https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:116: // This isn't ideal - if we also serialized the current time and time ticks, nit: Don't use we in comments (x2) https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:155: // If we just serialized the absolute wall clock time, subtracting the nit: -we
Addressed mmenke's final review nits - thanks :) https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... File net/base/backoff_entry_serializer.cc (right): https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... net/base/backoff_entry_serializer.cc:66: || absolute_release_time_us < 0) { On 2015/05/06 20:48:44, mmenke wrote: > || should go on the previous line, I believe. You are running "git cl format > net", right? Should get a presubmit error if you don't. Fine if this is the > code that results in. Done (`git cl format net` allows both, and the style guide[1] just says to be consistent; but having them at the end of the line is indeed much more common in net/). [1]: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Boolean_Expre... https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... File net/base/backoff_entry_serializer_unittest.cc (right): https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:116: // This isn't ideal - if we also serialized the current time and time ticks, On 2015/05/06 20:48:44, mmenke wrote: > nit: Don't use we in comments (x2) Done (oops, old habits). https://codereview.chromium.org/1023473003/diff/80001/net/base/backoff_entry_... net/base/backoff_entry_serializer_unittest.cc:155: // If we just serialized the absolute wall clock time, subtracting the On 2015/05/06 20:48:44, mmenke wrote: > nit: -we Done.
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1023473003/#ps100001 (title: "Address mmenke's final review nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023473003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6727a62ae541389ec4a5264b279485d75e2c4fe5 Cr-Commit-Position: refs/heads/master@{#328755} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
