Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23205008:
Fix a bug where the minimum granular time for Alarms was not calculated properly. (Closed)
|
Created:
7 years, 4 months ago by dhnishi (use Chromium) Modified:
6 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix a bug where the minimum granular time for Alarms was not calculated properly.
BUG=268300
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218398
Patch Set 1 #
Total comments: 2
Patch Set 2 : Unit tests. #
Total comments: 4
Messages
Total messages: 16 (0 generated)
@mpcomplete: PTAL when you get a moment.
https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/alarms/alarm_manager.cc:327: base::TimeDelta cur_alarm_delta = cur_alarm_time - clock_->Now(); I am worried about this causing a potential race condition. Thoughts?
Could you add a unittest that covers the case outlined in the bug (and make sure it fails without your patch)? Thanks for the thorough investigation and quick fix!
https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/alarms/alarm_manager.cc:327: base::TimeDelta cur_alarm_delta = cur_alarm_time - clock_->Now(); On 2013/08/16 22:28:29, Daniel Nishi wrote: > I am worried about this causing a potential race condition. Thoughts? In what sense? I'm not sure I see a problem.
On 2013/08/16 22:30:59, Matt Perry wrote: > https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... > File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): > > https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... > chrome/browser/extensions/api/alarms/alarm_manager.cc:327: base::TimeDelta > cur_alarm_delta = cur_alarm_time - clock_->Now(); > On 2013/08/16 22:28:29, Daniel Nishi wrote: > > I am worried about this causing a potential race condition. Thoughts? > > In what sense? I'm not sure I see a problem. If for some reason execution got stalled in there for a very, very long time, the delta calculation would be way off which could impact what the actual min_granularity should be. It just feels like execution could be different depending on how parts got delayed. It shouldn't have any big impact on how everything comes out in the end, though.
On 2013/08/16 22:35:48, Daniel Nishi wrote: > On 2013/08/16 22:30:59, Matt Perry wrote: > > > https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... > > File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): > > > > > https://codereview.chromium.org/23205008/diff/1/chrome/browser/extensions/api... > > chrome/browser/extensions/api/alarms/alarm_manager.cc:327: base::TimeDelta > > cur_alarm_delta = cur_alarm_time - clock_->Now(); > > On 2013/08/16 22:28:29, Daniel Nishi wrote: > > > I am worried about this causing a potential race condition. Thoughts? > > > > In what sense? I'm not sure I see a problem. > > If for some reason execution got stalled in there for a very, very long time, > the delta calculation would be way off which could impact what the actual > min_granularity should be. It just feels like execution could be different > depending on how parts got delayed. > > It shouldn't have any big impact on how everything comes out in the end, though. Even if it stalls, you're still computing the time remaining before the alarm should fire. So it won't fire too quickly. Seems fine to me.
I added a unit test. It fails without my patch at 240000 (one extra minute) as expected and succeeds with my patch.
Great, LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23205008/9001
Retried try job too often on ios_dbg_simulator for step(s) media_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23205008/9001
Message was sent while issue was closed.
Change committed as 218398
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... chrome/browser/extensions/api/alarms/alarm_manager.cc:330: if (min_granularity < l_it->minimum_granularity) I think these 2 ifs are reversed. The idea here is that l_it->minimum_granularity is the extension's granularity (0 for an unpacked extension, 1 min otherwise). So if a release-mode alarm will go off 30 sec from now, it should be capped to 1min instead. But that shouldn't override the min_granularity if it's already set to go off in, say, 10 sec. TLDR: base::TimeDelta cur_alarm_delta = cur_alarm_time - clock_->Now(); if (cur_alarm_delta < l_it->minimum_granularity) cur_alarm_delta = l_it->minimum_granularity; if (cur_alarm_delta < min_granularity) min_granularity = cur_alarm_delta;
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... chrome/browser/extensions/api/alarms/alarm_manager.cc:330: if (min_granularity < l_it->minimum_granularity) On 2014/01/16 22:58:34, Matt Perry wrote: > I think these 2 ifs are reversed. The idea here is that > l_it->minimum_granularity is the extension's granularity (0 for an unpacked > extension, 1 min otherwise). So if a release-mode alarm will go off 30 sec from > now, it should be capped to 1min instead. But that shouldn't override the > min_granularity if it's already set to go off in, say, 10 sec. > > TLDR: > > base::TimeDelta cur_alarm_delta = cur_alarm_time - clock_->Now(); > if (cur_alarm_delta < l_it->minimum_granularity) > cur_alarm_delta = l_it->minimum_granularity; > if (cur_alarm_delta < min_granularity) > min_granularity = cur_alarm_delta; This lg to me.
Message was sent while issue was closed.
I've added some examples showing how the computed min_granularity now depends on the *order* of alarms visited. https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... chrome/browser/extensions/api/alarms/alarm_manager.cc:330: if (min_granularity < l_it->minimum_granularity) On 2014/01/16 22:58:34, Matt Perry wrote: > I think these 2 ifs are reversed. The idea here is that > l_it->minimum_granularity is the extension's granularity (0 for an unpacked > extension, 1 min otherwise). So if a release-mode alarm will go off 30 sec from > now, it should be capped to 1min instead. But that shouldn't override the > min_granularity if it's already set to go off in, say, 10 sec. I don't think it's reversed -- it should _increase_ min_granularity if it's below an alarm's minimum_granularity. However, it makes the result sensitive to the order of the alarms iterated over. If an early alarm has a large minimum_granularity, then min_granularity would be set to that, but a later alarm with a small granularity (not minimum_granularity) would lower it again. Example: alarm1: granularity=1min, minumum_granularity=1min alarm2: granularity=10min, minimum_granularity=1min alarm3: granularity=1000min, minimum_granularity=100min Given (alarm1, alarm2, alarm3), min_granularity ends up at 100min. Given (alarm2, alarm3, alarm1), min_granularity ends up at 1min. Given (alarm1, alarm3, alarm2), min_granularity ends up at 10min. I think what was intended here was: min_granularity = max ( min(all granularity members), max(all minimum_granularity members) ) But that's not what's being computed. > > TLDR: > > base::TimeDelta cur_alarm_delta = cur_alarm_time - clock_->Now(); > if (cur_alarm_delta < l_it->minimum_granularity) > cur_alarm_delta = l_it->minimum_granularity; > if (cur_alarm_delta < min_granularity) > min_granularity = cur_alarm_delta;
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... File chrome/browser/extensions/api/alarms/alarm_manager.cc (right): https://chromiumcodereview.appspot.com/23205008/diff/9001/chrome/browser/exte... chrome/browser/extensions/api/alarms/alarm_manager.cc:330: if (min_granularity < l_it->minimum_granularity) On 2014/01/17 14:54:32, iclelland wrote: > On 2014/01/16 22:58:34, Matt Perry wrote: > > I think these 2 ifs are reversed. The idea here is that > > l_it->minimum_granularity is the extension's granularity (0 for an unpacked > > extension, 1 min otherwise). So if a release-mode alarm will go off 30 sec > from > > now, it should be capped to 1min instead. But that shouldn't override the > > min_granularity if it's already set to go off in, say, 10 sec. > > I don't think it's reversed -- it should _increase_ min_granularity if it's > below an alarm's minimum_granularity. > > However, it makes the result sensitive to the order of the alarms iterated over. > If an early alarm has a large minimum_granularity, then min_granularity would be > set to that, but a later alarm with a small granularity (not > minimum_granularity) would lower it again. > > Example: > alarm1: granularity=1min, minumum_granularity=1min > alarm2: granularity=10min, minimum_granularity=1min > alarm3: granularity=1000min, minimum_granularity=100min > > Given (alarm1, alarm2, alarm3), min_granularity ends up at 100min. > Given (alarm2, alarm3, alarm1), min_granularity ends up at 1min. > Given (alarm1, alarm3, alarm2), min_granularity ends up at 10min. > > > I think what was intended here was: > min_granularity = max ( min(all granularity members), max(all > minimum_granularity members) ) That's not the intended calculation. Each alarm's minimum_granularity should only affect that alarm. So for your example, our desired min_granularity is 1min, due to alarm 1. What we want is: min_granularity = min( for each alarm, max(alarm granularity, alarm minimum_granularity) ) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
