Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
385123004:
Implement the WindowsLogoffRace finch experiment. (Closed)
|
Created:
6 years, 5 months ago by Sigurður Ásgeirsson Modified:
6 years, 5 months ago CC:
chromium-reviews, rginda+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionImplement the WindowsLogoffRace finch experiment.
R=gab@chromium.org,asvitkine@chromium.org
TBR=sky@chromium.org
BUG=388741
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283282
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address Gab's comments. #Patch Set 3 : Add an additional timing check around EndSession to prevent test success when timing out on the blo… #
Total comments: 11
Patch Set 4 : Address gab and asvitkine's comments. #Patch Set 5 : Revert chrome_browser_field_trials change. #
Messages
Total messages: 17 (0 generated)
Hey guys, this is to measure the effect size of the Windows logoff race. PTAL. sky, asvitkine for owners approval please. Siggi
https://codereview.chromium.org/385123004/diff/1/chrome/browser/browser_proce... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/385123004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:448: return group_name == "BrokenSynchronization"; As discussed I think you want a control group and an experiment group (so that you can ignore the default group which kicks in on weird configurations where Finch loses its seed/etc. and could corrupt your data). This probably also justifies having a common method to get the experiment instead of duplicating code here and in the next file of this CL. https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_browsertest.cc:404: base::Bind(&BlockThread, &is_blocked, base::Owned(unblock))); Actually base::Owned here makes it such that |unblock| is deleted after running BlockThread()... so |unblock->Signal()| at the bottom is a UAF. What you want is to put |unblock| on the stack and use base::Unretained() for this callback IMO. https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_browsertest.cc:420: if (end - start < base::TimeDelta::FromSeconds(5)) { Since you're adding this here, I think it makes to assert in your non-experiment test that despite the wait happening, EndSession() finished before the timeout expired (i.e. end - start < 10). https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_browsertest.cc:435: "BrokenSynchronization"); Each field trial should be unique to a given browser process IMO and each browser test runs in its own browser process so no need to clean this up IMO.
thanks, please take another look. https://codereview.chromium.org/385123004/diff/1/chrome/browser/browser_proce... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/385123004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:448: return group_name == "BrokenSynchronization"; On 2014/07/11 17:13:44, gab wrote: > As discussed I think you want a control group and an experiment group (so that > you can ignore the default group which kicks in on weird configurations where > Finch loses its seed/etc. and could corrupt your data). > > This probably also justifies having a common method to get the experiment > instead of duplicating code here and in the next file of this CL. As-is, this supports a three group split, as the default and control groups have no differences in behavior. https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_browsertest.cc:404: base::Bind(&BlockThread, &is_blocked, base::Owned(unblock))); On 2014/07/11 17:13:44, gab wrote: > Actually base::Owned here makes it such that |unblock| is deleted after running > BlockThread()... so |unblock->Signal()| at the bottom is a UAF. > > What you want is to put |unblock| on the stack and use base::Unretained() for > this callback IMO. As we discussed, you've read this backwards. The code is good as-is. https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_browsertest.cc:420: if (end - start < base::TimeDelta::FromSeconds(5)) { On 2014/07/11 17:13:44, gab wrote: > Since you're adding this here, I think it makes to assert in your non-experiment > test that despite the wait happening, EndSession() finished before the timeout > expired (i.e. end - start < 10). Done. https://codereview.chromium.org/385123004/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_browsertest.cc:435: "BrokenSynchronization"); On 2014/07/11 17:13:44, gab wrote: > Each field trial should be unique to a given browser process IMO and each > browser test runs in its own browser process so no need to clean this up IMO. Done.
https://codereview.chromium.org/385123004/diff/40001/chrome/browser/browser_p... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/browser_p... chrome/browser/browser_process_impl.cc:445: bool ExperimentUseBrokenSynchronization() { It would be useful for the reader to add a comment here to explain what the two different behaviours are. https://codereview.chromium.org/385123004/diff/40001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/chrome_br... chrome/browser/chrome_browser_field_trials.cc:60: base::FieldTrialList::FindValue("WindowsLogoffRace"); I don't think this is necessary since your experiment runs entirely in the browser process. https://codereview.chromium.org/385123004/diff/40001/chrome/browser/lifetime/... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/lifetime/... chrome/browser/lifetime/application_lifetime.cc:296: if (!ExperimentUseBrokenSynchronization()) { Nit: This would be easier to parse if you didn't have the negation. Either reverse the blocks or change the function to check for the opposite with a different name.
lgtm % asvitkine https://codereview.chromium.org/385123004/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_browsertest.cc:377: // The EndSession timeout is 10 seconds. If we take moer than half that, s/moer/more https://codereview.chromium.org/385123004/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_browsertest.cc:382: if ((end - start) > base::TimeDelta::FromSeconds(5)) nit: Remove brackets around "end - start" (as you did below)
Thanks, please take another look. https://codereview.chromium.org/385123004/diff/40001/chrome/browser/browser_p... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/browser_p... chrome/browser/browser_process_impl.cc:445: bool ExperimentUseBrokenSynchronization() { On 2014/07/14 18:55:02, Alexei Svitkine wrote: > It would be useful for the reader to add a comment here to explain what the two > different behaviours are. Done. https://codereview.chromium.org/385123004/diff/40001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/chrome_br... chrome/browser/chrome_browser_field_trials.cc:60: base::FieldTrialList::FindValue("WindowsLogoffRace"); On 2014/07/14 18:55:02, Alexei Svitkine wrote: > I don't think this is necessary since your experiment runs entirely in the > browser process. I don't know how this machinery works, but the comment above leads me to believe that my experiment will never be "used" until logoff. The metric I'm after will however be reported right after launch, e.g. the unclean shutdown count. Are you sure I'm good without this? I guess I'm asking whether all UMA reports contain the experiment groups that client is enlisted in? https://codereview.chromium.org/385123004/diff/40001/chrome/browser/lifetime/... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/lifetime/... chrome/browser/lifetime/application_lifetime.cc:296: if (!ExperimentUseBrokenSynchronization()) { On 2014/07/14 18:55:02, Alexei Svitkine wrote: > Nit: This would be easier to parse if you didn't have the negation. Either > reverse the blocks or change the function to check for the opposite with a > different name. Too right, fixed. https://codereview.chromium.org/385123004/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_browsertest.cc:377: // The EndSession timeout is 10 seconds. If we take moer than half that, On 2014/07/14 19:11:14, gab wrote: > s/moer/more Done. https://codereview.chromium.org/385123004/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_browsertest.cc:382: if ((end - start) > base::TimeDelta::FromSeconds(5)) On 2014/07/14 19:11:14, gab wrote: > nit: Remove brackets around "end - start" (as you did below) Done.
LGTM % reverting the change to chrome_browser_field_trials.cc per my comment below. Thanks! https://codereview.chromium.org/385123004/diff/40001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/385123004/diff/40001/chrome/browser/chrome_br... chrome/browser/chrome_browser_field_trials.cc:60: base::FieldTrialList::FindValue("WindowsLogoffRace"); On 2014/07/14 20:31:38, Sigurður Ásgeirsson wrote: > On 2014/07/14 18:55:02, Alexei Svitkine wrote: > > I don't think this is necessary since your experiment runs entirely in the > > browser process. > > I don't know how this machinery works, but the comment above leads me to believe > that my experiment will never be "used" until logoff. The metric I'm after will > however be reported right after launch, e.g. the unclean shutdown count. > > Are you sure I'm good without this? > I guess I'm asking whether all UMA reports contain the experiment groups that > client is enlisted in? Having the trial start in "active" state makes sense. Still, you don't need this line as that can be done from the server-side with a starts_active:true flag.
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/385123004/80001
The CQ bit was unchecked by siggi@chromium.org
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/385123004/80001
The CQ bit was unchecked by siggi@chromium.org
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/385123004/80001
On Mon, Jul 14, 2014 at 5:40 PM, <asvitkine@chromium.org> wrote: > LGTM % reverting the change to chrome_browser_field_trials.cc per my > comment > below. Thanks! > > > > https://codereview.chromium.org/385123004/diff/40001/ > chrome/browser/chrome_browser_field_trials.cc > File chrome/browser/chrome_browser_field_trials.cc (right): > > https://codereview.chromium.org/385123004/diff/40001/ > chrome/browser/chrome_browser_field_trials.cc#newcode60 > chrome/browser/chrome_browser_field_trials.cc:60: > base::FieldTrialList::FindValue("WindowsLogoffRace"); > On 2014/07/14 20:31:38, Sigurður Ásgeirsson wrote: > >> On 2014/07/14 18:55:02, Alexei Svitkine wrote: >> > I don't think this is necessary since your experiment runs entirely >> > in the > >> > browser process. >> > > I don't know how this machinery works, but the comment above leads me >> > to believe > >> that my experiment will never be "used" until logoff. The metric I'm >> > after will > >> however be reported right after launch, e.g. the unclean shutdown >> > count. > > Are you sure I'm good without this? >> I guess I'm asking whether all UMA reports contain the experiment >> > groups that > >> client is enlisted in? >> > > Having the trial start in "active" state makes sense. Still, you don't > need this line as that can be done from the server-side with a > starts_active:true flag. > > Thanks - so amended... > https://codereview.chromium.org/385123004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Change committed as 283282 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
