Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
349263004:
Fix Windows logoff race. (Closed)
|
Created:
6 years, 6 months ago by Sigurður Ásgeirsson Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix Windows logoff race by posting a sentinel task to each taskrunner used to save state, and blocking on the completion of all sentinels.
De-flake and re-enable ProfileBrowserTests, by not running the tasks queued to initialize Bookmarks. This caused test failures when the delayed initialization managed to run.
This depends on https://codereview.chromium.org/370323002/, which fixes a BookmarkStorage lifetime issue, and https://codereview.chromium.org/375683002/ which annotates a remaining leak.
TBR=sky@chromium.org, brettw@chromium.org
R=erikwright@chromium.org, gab=chromium.org, brettw@chromium.org
BUG=386126, 141141, 141517, 142787, 140882, 165760, 163713, 391508
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281746
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281807
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address Gab and Erik's comments. #
Total comments: 4
Patch Set 3 : Address Gab's comments. #Patch Set 4 : Rejig the rundown post accounting for better encapsulation. #
Total comments: 4
Patch Set 5 : Start counter at one and maintain throughout lifetime for easier use. #Patch Set 6 : Remove unused static function. #
Total comments: 8
Patch Set 7 : Address Gab's comments. #
Total comments: 1
Patch Set 8 : De-flake ProfileBrowserTest and add an EndSession test. #Patch Set 9 : Enable all profile tests, remove #if 0 code. Rebase to ToT. #
Total comments: 17
Patch Set 10 : Address Monica's comment. #
Total comments: 2
Patch Set 11 : Test that Profile settings are written through to disk on EndSession. #
Total comments: 8
Patch Set 12 : Remove refcounting from BookmarkStorage. #Patch Set 13 : Add an annotation for the remaining leak, as I can't see how to fix it. #Patch Set 14 : Moar better documentation on leak. #Patch Set 15 : Address Monicas and Gabs comments. #Patch Set 16 : BookmarkStorage changes pulled out to issue 370323002. #
Total comments: 2
Patch Set 17 : ChromeBookmarkClient changes pulled out to issue 375683002. #
Total comments: 3
Patch Set 18 : Fix build break by compiling EndSession test conditionally. #Patch Set 19 : Also make a file-local function conditionally compiled with its sole user. #
Messages
Total messages: 61 (0 generated)
Nice job hunting this down! s/tak/task in CL description https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:384: class RundownTaskCounter { This could simply be a RefCounted object (especially since atomic_ref_count.h mentions that its preferable to use RefCounted rather than this low level implementation). Each task would hold a reference to the object (bound to a no-op method). This class would then simply signal the WaitableEvent in its destructor. You then don't need to initialize this class with a specific |count|, you instead hand as many references as you want and don't run the risk of initializing with an incorrect |count|. i.e., either all tasks complete and this RefCounted object goes away, signaling the event, or the timeout on the event fires and the remaining tasks become irrelevant. https://codereview.chromium.org/349263004/diff/1/chrome/browser/lifetime/appl... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/lifetime/appl... chrome/browser/lifetime/application_lifetime.cc:298: base::win::SetShouldCrashOnProcessDetach(false); This code is now unreachable.
I agree with Gab's suggestion. https://codereview.chromium.org/349263004/diff/1/chrome/browser/lifetime/appl... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/lifetime/appl... chrome/browser/lifetime/application_lifetime.cc:286: base::KillProcess(base::Process::Current().handle(), 0, false); Is it not potentially better to use the same logic on all platforms for consistency?
Thanks - PTAL while I try and find a way to add some tests to this. https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:384: class RundownTaskCounter { On 2014/06/23 19:53:48, gab wrote: > This could simply be a RefCounted object (especially since atomic_ref_count.h > mentions that its preferable to use RefCounted rather than this low level > implementation). > > Each task would hold a reference to the object (bound to a no-op method). > > This class would then simply signal the WaitableEvent in its destructor. > > You then don't need to initialize this class with a specific |count|, you > instead hand as many references as you want and don't run the risk of > initializing with an incorrect |count|. > > i.e., either all tasks complete and this RefCounted object goes away, signaling > the event, or the timeout on the event fires and the remaining tasks become > irrelevant. Conflating lifetime management with state is generally a bad idea, so I don't think it's advisable to do that. Also the count must be set up such that there's precisely a single transition from one to zero. Posting the object to multiple threads potentially interleaves the up and down counts, which can lead to multiple one->zero transitions. Making the object RefCounted for lifetime management alone is however not crazy. https://codereview.chromium.org/349263004/diff/1/chrome/browser/lifetime/appl... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/lifetime/appl... chrome/browser/lifetime/application_lifetime.cc:286: base::KillProcess(base::Process::Current().handle(), 0, false); On 2014/06/23 19:59:00, erikwright wrote: > Is it not potentially better to use the same logic on all platforms for > consistency? I don't know that it is - this is fulfilling a draconian, Windows-specific contract. I don't know that we're not better off with the existing code on other platforms. https://codereview.chromium.org/349263004/diff/1/chrome/browser/lifetime/appl... chrome/browser/lifetime/application_lifetime.cc:298: base::win::SetShouldCrashOnProcessDetach(false); On 2014/06/23 19:53:48, gab wrote: > This code is now unreachable. Done.
https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:384: class RundownTaskCounter { On 2014/06/23 20:20:12, Sigurður Ásgeirsson wrote: > On 2014/06/23 19:53:48, gab wrote: > > This could simply be a RefCounted object (especially since atomic_ref_count.h > > mentions that its preferable to use RefCounted rather than this low level > > implementation). > > > > Each task would hold a reference to the object (bound to a no-op method). > > > > This class would then simply signal the WaitableEvent in its destructor. > > > > You then don't need to initialize this class with a specific |count|, you > > instead hand as many references as you want and don't run the risk of > > initializing with an incorrect |count|. > > > > i.e., either all tasks complete and this RefCounted object goes away, > signaling > > the event, or the timeout on the event fires and the remaining tasks become > > irrelevant. > > Conflating lifetime management with state is generally a bad idea, so I don't > think it's advisable to do that. Also the count must be set up such that there's > precisely a single transition from one to zero. Posting the object to multiple > threads potentially interleaves the up and down counts, which can lead to > multiple one->zero transitions. Ah right, good point about the possibility of a thread running its task before other tasks have time to be posted. > > Making the object RefCounted for lifetime management alone is however not crazy. https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:405: DCHECK_LT(1U, count); DCHECK_GT(count, 1U); (We always put constants on the RHS of equations in Chromium) https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:409: DCHECK_NE(static_cast<RundownTaskCounter*>(NULL), counter); DCHECK(counter); https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:454: base::Bind(RundownTaskCounter::Decrement, Now that RundownTaskCounter is refcounted, Decrement can be non-static (having it static yet take a pointer to a RundownTaskCounter is essentially the same as non-static but less readable IMO). https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:455: counter.get())); On ChromeOS this never gets posted (as per the ifdef above) so that the RundownTaskCounter will never hit 0. This is why I would have preferred to explicitly hand references/ups to tasks rather than initialize a global count above, but this doesn't work as-is because of the possibility of a thread finishing its task before we've posted other tasks here (as you mentioned above). One way could perhaps be to post another task in front of this one which would force each thread hitting it early to spin until we release it here after all tasks have been posted (and all ups have thus been registered)? Or even better, without posting another task, the RundownTaskCounter could have an event which it signals when its ready for threads to start decrementing (and Decrement would block until that event is signaled). Allowing the RundownTaskCounter to register all its ups prior to allowing Decrements to start kicking in. https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:456: rm empty line https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... File chrome/browser/lifetime/application_lifetime.cc (left): https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... chrome/browser/lifetime/application_lifetime.cc:289: base::win::SetShouldCrashOnProcessDetach(false); Don't you still want to set this before killing the process? https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... chrome/browser/lifetime/application_lifetime.cc:291: // can properly shutdown before we exit. From this comment it appears tests may expect this to run.
Thanks - liking the look of this yet? https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:454: base::Bind(RundownTaskCounter::Decrement, On 2014/06/23 21:09:53, gab wrote: > Now that RundownTaskCounter is refcounted, Decrement can be non-static (having > it static yet take a pointer to a RundownTaskCounter is essentially the same as > non-static but less readable IMO). Done. https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:455: counter.get())); On 2014/06/23 21:09:53, gab wrote: > On ChromeOS this never gets posted (as per the ifdef above) so that the > RundownTaskCounter will never hit 0. > > This is why I would have preferred to explicitly hand references/ups to tasks > rather than initialize a global count above, but this doesn't work as-is because > of the possibility of a thread finishing its task before we've posted other > tasks here (as you mentioned above). > > One way could perhaps be to post another task in front of this one which would > force each thread hitting it early to spin until we release it here after all > tasks have been posted (and all ups have thus been registered)? > Or even better, without posting another task, the RundownTaskCounter could have > an event which it signals when its ready for threads to start decrementing (and > Decrement would block until that event is signaled). Allowing the > RundownTaskCounter to register all its ups prior to allowing Decrements to start > kicking in. Thanks great catch. I've added some robustness against mis-counting here by asserting on the correct count at wait time. I'm disinclined to block or serialize everything at this critical lifetime point of Windows applications. Also KISS :). https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:456: On 2014/06/23 21:09:53, gab wrote: > rm empty line Done. https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... File chrome/browser/lifetime/application_lifetime.cc (left): https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... chrome/browser/lifetime/application_lifetime.cc:289: base::win::SetShouldCrashOnProcessDetach(false); It really doesn't matter, as TerminateProcess implies immediate process termination. I've added this anyways... https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/349263004/diff/20001/chrome/browser/lifetime/... chrome/browser/lifetime/application_lifetime.cc:291: // can properly shutdown before we exit. On 2014/06/23 21:09:53, gab wrote: > From this comment it appears tests may expect this to run. Thanks, I guess we'll see - I have a hard time believing that this runs in any test :).
Is there a browser test that covers this scenario? https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/1/chrome/browser/browser_proce... chrome/browser/browser_process_impl.cc:384: class RundownTaskCounter { On 2014/06/23 20:20:12, Sigurður Ásgeirsson wrote: > On 2014/06/23 19:53:48, gab wrote: > > This could simply be a RefCounted object (especially since atomic_ref_count.h > > mentions that its preferable to use RefCounted rather than this low level > > implementation). > > > > Each task would hold a reference to the object (bound to a no-op method). > > > > This class would then simply signal the WaitableEvent in its destructor. > > > > You then don't need to initialize this class with a specific |count|, you > > instead hand as many references as you want and don't run the risk of > > initializing with an incorrect |count|. > > > > i.e., either all tasks complete and this RefCounted object goes away, > signaling > > the event, or the timeout on the event fires and the remaining tasks become > > irrelevant. > > Conflating lifetime management with state is generally a bad idea, so I don't > think it's advisable to do that. Also the count must be set up such that there's > precisely a single transition from one to zero. Posting the object to multiple > threads potentially interleaves the up and down counts, which can lead to > multiple one->zero transitions. > > Making the object RefCounted for lifetime management alone is however not crazy. The concern about multiple 1->0 transitions is not founded as you would presumably hold a local scoped_refptr while you are busy posting all of the tasks. https://codereview.chromium.org/349263004/diff/60001/chrome/browser/browser_p... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/60001/chrome/browser/browser_p... chrome/browser/browser_process_impl.cc:394: void Decrement(); Make this private. https://codereview.chromium.org/349263004/diff/60001/chrome/browser/browser_p... chrome/browser/browser_process_impl.cc:447: // Add one for local state on Windows. This comment is inaccurate - it's on non-ChromeOS platforms including OS X etc.
> > The concern about multiple 1->0 transitions is not founded as you would > presumably hold a local scoped_refptr while you are busy posting all of > the tasks. > > This concern is specifically to keeping a posted/released counter for releasing the waitable event, and not the lifetime management, which is straightforward. However, now that you mention it, it's simpler - and absolutely straightforward - to adopt the same method for this counter as we'd do for lifetime management. E.g. to simply maintain an extra count on the posted counter and release it only just before waiting ... I wish I'd thought of that :). > https://codereview.chromium.org/349263004/diff/60001/ > chrome/browser/browser_process_impl.cc > File chrome/browser/browser_process_impl.cc (right): > > https://codereview.chromium.org/349263004/diff/60001/ > chrome/browser/browser_process_impl.cc#newcode394 > chrome/browser/browser_process_impl.cc:394: void Decrement(); > Make this private. > > https://codereview.chromium.org/349263004/diff/60001/ > chrome/browser/browser_process_impl.cc#newcode447 > chrome/browser/browser_process_impl.cc:447: // Add one for local state > on Windows. > This comment is inaccurate - it's on non-ChromeOS platforms including OS > X etc. > > https://codereview.chromium.org/349263004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I don't see any tests exercising this, will keep looking. https://codereview.chromium.org/349263004/diff/60001/chrome/browser/browser_p... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/60001/chrome/browser/browser_p... chrome/browser/browser_process_impl.cc:394: void Decrement(); On 2014/06/24 16:10:53, erikwright wrote: > Make this private. Done. https://codereview.chromium.org/349263004/diff/60001/chrome/browser/browser_p... chrome/browser/browser_process_impl.cc:447: // Add one for local state on Windows. On 2014/06/24 16:10:53, erikwright wrote: > This comment is inaccurate - it's on non-ChromeOS platforms including OS X etc. Removed.
Code looks good. I would still have a slight preference to have lifetime management and count management under the same umbrella; I really don't see an advantage to having two counts being increased/decreased together only for the sake of "separating" the two concerns. A browser test could be written for this that would: 1) Write to a few profile stores and local state in the test body. 1.5) Register the current |time_| at the end of the test body (or maybe in TearDownOnMainThread() which is explicitly the end of the test before any TearDown/shutdown occurs in the browser). 2) In TearDown() verify that shutdown completed faster than kEndSessionTimeoutSeconds (comparing |Now() - time_|) and that modified prefs were properly flushed to disk. https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:379: class RundownTaskCounter : Add a meta-comment explaining the general purpose of this class. https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:397: // event on transition to zero. Some of this second line fits on the previous line. https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:459: rundown_counter->Post(local_state_task_runner_); This only happens inside the "if (metrics && local_state())" check; is the assumption that local_state has already been flushed otherwise? https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:471: // that if we don't complete "quickly enough", Windows will terminate our Can you quantify "quickly enough" based on documentation/experimentation? kEndSessionTimeoutSeconds is 10 seconds at the moment, is that "quickly enough"?
Since I'm vehemently opposed to mixing lifetime management and other state, we've agreed not to attempt that here. The two counts are also not identical, so this would be a little difficult in any case. https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:379: class RundownTaskCounter : On 2014/06/25 14:21:50, gab wrote: > Add a meta-comment explaining the general purpose of this class. Done. https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:397: // event on transition to zero. On 2014/06/25 14:21:50, gab wrote: > Some of this second line fits on the previous line. Done. https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:459: rundown_counter->Post(local_state_task_runner_); On 2014/06/25 14:21:50, gab wrote: > This only happens inside the "if (metrics && local_state())" check; is the > assumption that local_state has already been flushed otherwise? Apparently things work differently for local state on ChromeOS. https://codereview.chromium.org/349263004/diff/100001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:471: // that if we don't complete "quickly enough", Windows will terminate our On 2014/06/25 14:21:50, gab wrote: > Can you quantify "quickly enough" based on documentation/experimentation? > > kEndSessionTimeoutSeconds is 10 seconds at the moment, is that "quickly enough"? The timeout is documented something like 5 seconds, but it's not deterministic. What happens when you overrun is that Windows (as of Vista or Win7) flings up a dialog complaining that Chrome's tardy, and gives the user the option of blowing it out of the sky manually. Normally this should complete much faster than 10 seconds, but I don't think the amount of delay is very important - it's only a safeguard against deadlocking.
Hey guys, I feel testing for this and the finch experiment ought to be separate CLs. Unless you object, I'd like to land this fix alone and then work on testing etc. Siggi
lgtm w/ nit I would like to see a test in the same CL as: 1) It makes it easier to confirm that your new test fails flakily without this CL and passes all the time with it (and you can even add that to your CL description to strengthen your point). 2) I don't think the browser test I suggested is that hard to write. 3) Since there will be no Dev channel push for ~2 weeks, there is no rush to commit this to get early data IMO. But if despite all of this you still want to write the test in a follow-up CL I guess I'm okay with it (I'd typically say no as promised follow-up CLs tend to not occur with high probability, but I trust a select few like yourself to care enough about stability to follow through). Cheers! GAb https://codereview.chromium.org/349263004/diff/120001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/120001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:424: base::Bind(&RundownTaskCounter::Decrement, this)); nit: Align with '(' on previous line.
I like writing a test too in this patch. On Wed, Jun 25, 2014 at 8:23 AM, <gab@chromium.org> wrote: > lgtm w/ nit > > I would like to see a test in the same CL as: > 1) It makes it easier to confirm that your new test fails flakily without > this > CL and passes all the time with it (and you can even add that to your CL > description to strengthen your point). > 2) I don't think the browser test I suggested is that hard to write. > 3) Since there will be no Dev channel push for ~2 weeks, there is no rush > to > commit this to get early data IMO. > > But if despite all of this you still want to write the test in a follow-up > CL I > guess I'm okay with it (I'd typically say no as promised follow-up CLs > tend to > not occur with high probability, but I trust a select few like yourself to > care > enough about stability to follow through). > > Cheers! > GAb > > > https://codereview.chromium.org/349263004/diff/120001/ > chrome/browser/browser_process_impl.cc > File chrome/browser/browser_process_impl.cc (right): > > https://codereview.chromium.org/349263004/diff/120001/ > chrome/browser/browser_process_impl.cc#newcode424 > chrome/browser/browser_process_impl.cc:424: > base::Bind(&RundownTaskCounter::Decrement, this)); > nit: Align with '(' on previous line. > > https://codereview.chromium.org/349263004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hey guys, thanks for keeping me honest. I think this might be the right place to assert on the threading behavior of profile saving [ https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro..., and I'll be damned if these tests aren't flaky for the same reason as logoff is :/. Siggi On Wed, Jun 25, 2014 at 12:54 PM, Scott Violet <sky@chromium.org> wrote: > I like writing a test too in this patch. > > > On Wed, Jun 25, 2014 at 8:23 AM, <gab@chromium.org> wrote: > >> lgtm w/ nit >> >> I would like to see a test in the same CL as: >> 1) It makes it easier to confirm that your new test fails flakily without >> this >> CL and passes all the time with it (and you can even add that to your CL >> description to strengthen your point). >> 2) I don't think the browser test I suggested is that hard to write. >> 3) Since there will be no Dev channel push for ~2 weeks, there is no rush >> to >> commit this to get early data IMO. >> >> But if despite all of this you still want to write the test in a >> follow-up CL I >> guess I'm okay with it (I'd typically say no as promised follow-up CLs >> tend to >> not occur with high probability, but I trust a select few like yourself >> to care >> enough about stability to follow through). >> >> Cheers! >> GAb >> >> >> https://codereview.chromium.org/349263004/diff/120001/ >> chrome/browser/browser_process_impl.cc >> File chrome/browser/browser_process_impl.cc (right): >> >> https://codereview.chromium.org/349263004/diff/120001/ >> chrome/browser/browser_process_impl.cc#newcode424 >> chrome/browser/browser_process_impl.cc:424: >> base::Bind(&RundownTaskCounter::Decrement, this)); >> nit: Align with '(' on previous line. >> >> https://codereview.chromium.org/349263004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Please take (another) look.
https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:136: FlushIoTaskRunnerAndSpinThreads(); Is this used by every test? Could it go in the TearDown() function? https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:326: SpinThreads(); Hmm, why does this call SpinThreads() where all the other tests use FlushIoTaskRunnerAndSpinThreads()?
PTAL? https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:136: FlushIoTaskRunnerAndSpinThreads(); On 2014/07/03 18:07:11, Monica Dinculescu wrote: > Is this used by every test? Could it go in the TearDown() function? Sadly no, the last test has different rundown requirements. https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:326: SpinThreads(); On 2014/07/03 18:07:11, Monica Dinculescu wrote: > Hmm, why does this call SpinThreads() where all the other tests use > FlushIoTaskRunnerAndSpinThreads()? Because the EndSession function is required to block for the end of the IO. This is the only way to test this that I can think of - e.g. this test should only flake if EndSession ceases to synchronize correctly, which is not to say that it will reliably fail in that instance.
profiles lgtm w/ annoying comment request. Thanks for re-enabling the tests! https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:136: FlushIoTaskRunnerAndSpinThreads(); Ah, I see. Thanks! On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote: > On 2014/07/03 18:07:11, Monica Dinculescu wrote: > > Is this used by every test? Could it go in the TearDown() function? > > Sadly no, the last test has different rundown requirements. https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:326: SpinThreads(); Would you mind adding this as a comment? Thanks! On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote: > On 2014/07/03 18:07:11, Monica Dinculescu wrote: > > Hmm, why does this call SpinThreads() where all the other tests use > > FlushIoTaskRunnerAndSpinThreads()? > > Because the EndSession function is required to block for the end of the IO. This > is the only way to test this that I can think of - e.g. this test should only > flake if EndSession ceases to synchronize correctly, which is not to say that it > will reliably fail in that instance.
Thanks! https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:326: SpinThreads(); On 2014/07/03 18:23:42, Monica Dinculescu wrote: > Would you mind adding this as a comment? Thanks! > > On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote: > > On 2014/07/03 18:07:11, Monica Dinculescu wrote: > > > Hmm, why does this call SpinThreads() where all the other tests use > > > FlushIoTaskRunnerAndSpinThreads()? > > > > Because the EndSession function is required to block for the end of the IO. > This > > is the only way to test this that I can think of - e.g. this test should only > > flake if EndSession ceases to synchronize correctly, which is not to say that > it > > will reliably fail in that instance. > Per our chat, the comment on line 321 serves the purpose...
https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:326: SpinThreads(); On 2014/07/03 18:28:58, Sigurður Ásgeirsson wrote: > On 2014/07/03 18:23:42, Monica Dinculescu wrote: > > Would you mind adding this as a comment? Thanks! > > > > On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote: > > > On 2014/07/03 18:07:11, Monica Dinculescu wrote: > > > > Hmm, why does this call SpinThreads() where all the other tests use > > > > FlushIoTaskRunnerAndSpinThreads()? > > > > > > Because the EndSession function is required to block for the end of the IO. > > This > > > is the only way to test this that I can think of - e.g. this test should > only > > > flake if EndSession ceases to synchronize correctly, which is not to say > that > > it > > > will reliably fail in that instance. > > > Per our chat, the comment on line 321 serves the purpose... Ugh - I misunderstood your request. Added a comment for maintainabilityisms.
lgtm w/ test suggestion to prevent regressions. PS: THIS IS SO AWESOME, thanks for fighting through this! https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (left): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:53: // Unfortunately, this also results in warnings during debug runs. As mentioned offline, make sure ASAN bots pass and add memory leak suppressions with an explanatory bug if not. https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:323: g_browser_process->EndSession(); IMO we should test that g_browser_process->EndSession() completes faster then |kEndSessionTimeoutSeconds|. Otherwise if the shutdown code has an error such that we always wait for the timeout (10 seconds), this test might still pass (test timeout is 30s for Release and 45s for Debug IIRC) -- you can try this by introducing a bug in RundownTaskCounter by starting the count at 2 and verifying that this test still passes...
https://codereview.chromium.org/349263004/diff/180001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:323: g_browser_process->EndSession(); Also, this test should explicitly touch a pref IMO so that EndSession() always actually HAS to write something to disk.
https://codereview.chromium.org/349263004/diff/160001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:393: // Posts a rundown task to |task_runner|, can be invoked an arbitrary number I think I understand how this class works, but only through multiple readings of the comments and a bit of inference. IIUC, Post() posts an event to |task_runner|. TimedWait() waits until all posted tasks have executed OR |max_time| has elapsed. I suggest: // Posts a sentinel task to |task_runner|. May be called an arbitrary number of times. https://codereview.chromium.org/349263004/diff/160001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:398: // This can only be called once per instance. // Waits until all sentinel tasks have been executed or |max_time| has elapsed. May only be called once per instance. (In particular, the concept of a "count" is an implementation detail now and confusing in the context of the public API). https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:322: // If this test flakes, then logoff on Windows has probably broken again. I suggest actually loading (synchronously) the contents of the prefs file on disk and asserting that before this line they indicate an unclean shutdown (might require you to flush/spin _before_ EndSession) and after this line they don't. If there is anything else that we want to be absolutely sure happens during shutdown we should add that explicitly here, too, if at all possible. https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:326: SpinThreads(); On 2014/07/03 18:28:58, Sigurður Ásgeirsson wrote: > On 2014/07/03 18:23:42, Monica Dinculescu wrote: > > Would you mind adding this as a comment? Thanks! > > > > On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote: > > > On 2014/07/03 18:07:11, Monica Dinculescu wrote: > > > > Hmm, why does this call SpinThreads() where all the other tests use > > > > FlushIoTaskRunnerAndSpinThreads()? > > > > > > Because the EndSession function is required to block for the end of the IO. > > This > > > is the only way to test this that I can think of - e.g. this test should > only > > > flake if EndSession ceases to synchronize correctly, which is not to say > that > > it > > > will reliably fail in that instance. > > > Per our chat, the comment on line 321 serves the purpose... Presumably, stuff still _can_ be running on the other threads. EndSession only promises that the most important stuff will complete. Is it safe to exit here before those things finish? Presumably it is risky. So If you follow my recommendation above to explicitly check that prefs were written you can just flush/spin as everywhere else.
Hey guys, I rejigged the EndSession test per request. Turns out the test-created profile is not flushed as it's not under the profile manager's management. PTAL, but given the ASAN test failures etc, I suggest landing the test fixes in a separate CL from the bugfix. Siggi https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:322: // If this test flakes, then logoff on Windows has probably broken again. On 2014/07/03 19:06:41, erikwright wrote: > I suggest actually loading (synchronously) the contents of the prefs file on > disk and asserting that before this line they indicate an unclean shutdown > (might require you to flush/spin _before_ EndSession) and after this line they > don't. > > If there is anything else that we want to be absolutely sure happens during > shutdown we should add that explicitly here, too, if at all possible. Done. https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:323: g_browser_process->EndSession(); On 2014/07/03 18:56:56, gab wrote: > IMO we should test that g_browser_process->EndSession() completes faster then > |kEndSessionTimeoutSeconds|. > > Otherwise if the shutdown code has an error such that we always wait for the > timeout (10 seconds), this test might still pass (test timeout is 30s for > Release and 45s for Debug IIRC) -- you can try this by introducing a bug in > RundownTaskCounter by starting the count at 2 and verifying that this test still > passes... I'm done with regression tests for this, if you want more tests then I'd be more than happy to review the CL :). https://codereview.chromium.org/349263004/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:326: SpinThreads(); On 2014/07/03 19:06:42, erikwright wrote: > On 2014/07/03 18:28:58, Sigurður Ásgeirsson wrote: > > On 2014/07/03 18:23:42, Monica Dinculescu wrote: > > > Would you mind adding this as a comment? Thanks! > > > > > > On 2014/07/03 18:14:46, Sigurður Ásgeirsson wrote: > > > > On 2014/07/03 18:07:11, Monica Dinculescu wrote: > > > > > Hmm, why does this call SpinThreads() where all the other tests use > > > > > FlushIoTaskRunnerAndSpinThreads()? > > > > > > > > Because the EndSession function is required to block for the end of the > IO. > > > This > > > > is the only way to test this that I can think of - e.g. this test should > > only > > > > flake if EndSession ceases to synchronize correctly, which is not to say > > that > > > it > > > > will reliably fail in that instance. > > > > > Per our chat, the comment on line 321 serves the purpose... > > Presumably, stuff still _can_ be running on the other threads. EndSession only > promises that the most important stuff will complete. > > Is it safe to exit here before those things finish? Presumably it is risky. So > If you follow my recommendation above to explicitly check that prefs were > written you can just flush/spin as everywhere else. Done. https://codereview.chromium.org/349263004/diff/180001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:323: g_browser_process->EndSession(); On 2014/07/03 18:59:26, gab wrote: > Also, this test should explicitly touch a pref IMO so that EndSession() always > actually HAS to write something to disk. Rejigged this test to go through the profile manager. This tests the profile(s) that are actually under the manager's management.
Latest test lgtm w/ nits (I'll look into adding a timer check around EndSession in the test in a follow-up CL). If re-introducing the previously avoided-via-hack test leaks, I suggest we suppress the leaks for now. https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:86: return ""; s/""/std::string()/ I forget when I was told this, but Chromium prefers specifically using std::string() when specifying empty strings. https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:92: base::DictionaryValue* dict; Initialize to NULL (otherwise |!dict| can be reading an unitialized variable).
https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:345: for (size_t i = 0; i < loaded_profiles.size(); ++i) { Can you add an assert or check that there's at least one loaded profile? This test becomes moot and always successful if there's no loaded profiles...:(
Hey y'all, I think this is ready for prime time. This fixes the Windows logoff race and de-flakes the ProfileBrowserTest at the cost of a leak annotation. The one remaining leak I annotated is benign-ish, though Gab tells me it'd be better to fix this. Brett or Sky, please for owners approval in ***\bookmarks. I'm going to wrap a finch experiment around the fix in a separate CL, as this is suspected of contributing 50-90% of "browser crashes". Siggi
lgtm++ w/ 2 nits mentioned on prior patch sets. Please also expand the CL description to describe each individual problem this is fixing (i.e., the fixed shutdown race, the fixed tests, the BookmarkStorage memory management model, and the added LSAN annotation). Cheers!! Gab https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:86: return ""; On 2014/07/04 15:22:18, gab wrote: > s/""/std::string()/ > > I forget when I was told this, but Chromium prefers specifically using > std::string() when specifying empty strings. Ping https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:92: base::DictionaryValue* dict; On 2014/07/04 15:22:17, gab wrote: > Initialize to NULL (otherwise |!dict| can be reading an unitialized variable). Ping
Thanks! https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:86: return ""; On 2014/07/04 23:39:27, gab wrote: > On 2014/07/04 15:22:18, gab wrote: > > s/""/std::string()/ > > > > I forget when I was told this, but Chromium prefers specifically using > > std::string() when specifying empty strings. > > Ping Done. https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:92: base::DictionaryValue* dict; On 2014/07/04 23:39:27, gab wrote: > On 2014/07/04 15:22:17, gab wrote: > > Initialize to NULL (otherwise |!dict| can be reading an unitialized variable). > > Ping Done. https://codereview.chromium.org/349263004/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_browsertest.cc:345: for (size_t i = 0; i < loaded_profiles.size(); ++i) { On 2014/07/04 15:41:18, Monica Dinculescu wrote: > Can you add an assert or check that there's at least one loaded profile? This > test becomes moot and always successful if there's no loaded profiles...:( Done.
Hey guys, it occurred to me that it might be OK to clean up the remaining leak by passing ownership to the Closure at post time. From the looks of the code, I don't think the UI thread can ever deref that pointer, so at worst if the task never runs it'll hold a dangling pointer it'll never deref. Whatcha think? Siggi On Sat, Jul 5, 2014 at 4:03 PM, <siggi@chromium.org> wrote: > Thanks! > > > > https://codereview.chromium.org/349263004/diff/200001/ > chrome/browser/profiles/profile_browsertest.cc > File chrome/browser/profiles/profile_browsertest.cc (right): > > https://codereview.chromium.org/349263004/diff/200001/ > chrome/browser/profiles/profile_browsertest.cc#newcode86 > chrome/browser/profiles/profile_browsertest.cc:86: return ""; > On 2014/07/04 23:39:27, gab wrote: > >> On 2014/07/04 15:22:18, gab wrote: >> > s/""/std::string()/ >> > >> > I forget when I was told this, but Chromium prefers specifically >> > using > >> > std::string() when specifying empty strings. >> > > Ping >> > > Done. > > > https://codereview.chromium.org/349263004/diff/200001/ > chrome/browser/profiles/profile_browsertest.cc#newcode92 > chrome/browser/profiles/profile_browsertest.cc:92: > base::DictionaryValue* dict; > On 2014/07/04 23:39:27, gab wrote: > >> On 2014/07/04 15:22:17, gab wrote: >> > Initialize to NULL (otherwise |!dict| can be reading an unitialized >> > variable). > > Ping >> > > Done. > > > https://codereview.chromium.org/349263004/diff/200001/ > chrome/browser/profiles/profile_browsertest.cc#newcode345 > chrome/browser/profiles/profile_browsertest.cc:345: for (size_t i = 0; i > < loaded_profiles.size(); ++i) { > On 2014/07/04 15:41:18, Monica Dinculescu wrote: > >> Can you add an assert or check that there's at least one loaded >> > profile? This > >> test becomes moot and always successful if there's no loaded >> > profiles...:( > > Done. > > https://codereview.chromium.org/349263004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Jul 4, 2014 at 7:39 PM, <gab@chromium.org> wrote: > lgtm++ w/ 2 nits mentioned on prior patch sets. > > Please also expand the CL description to describe each individual problem > this > is fixing (i.e., the fixed shutdown race, the fixed tests, the > BookmarkStorage > memory management model, and the added LSAN annotation). > > Done. > Cheers!! > Gab > > > > https://codereview.chromium.org/349263004/diff/200001/ > chrome/browser/profiles/profile_browsertest.cc > File chrome/browser/profiles/profile_browsertest.cc (right): > > https://codereview.chromium.org/349263004/diff/200001/ > chrome/browser/profiles/profile_browsertest.cc#newcode86 > chrome/browser/profiles/profile_browsertest.cc:86: return ""; > > On 2014/07/04 15:22:18, gab wrote: > >> s/""/std::string()/ >> > > I forget when I was told this, but Chromium prefers specifically using >> std::string() when specifying empty strings. >> > > Ping > > > https://codereview.chromium.org/349263004/diff/200001/ > chrome/browser/profiles/profile_browsertest.cc#newcode92 > chrome/browser/profiles/profile_browsertest.cc:92: > base::DictionaryValue* dict; > On 2014/07/04 15:22:17, gab wrote: > >> Initialize to NULL (otherwise |!dict| can be reading an unitialized >> > variable). > > Ping > > https://codereview.chromium.org/349263004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Here's what that looks like: https://codereview.chromium.org/372663002/diff2/40001:60001/chrome/browser/bo... On Sat, Jul 5, 2014 at 4:06 PM, Sigurður Ásgeirsson <siggi@chromium.org> wrote: > Hey guys, > > it occurred to me that it might be OK to clean up the remaining leak by > passing ownership to the Closure at post time. From the looks of the code, > I don't think the UI thread can ever deref that pointer, so at worst if the > task never runs it'll hold a dangling pointer it'll never deref. > > Whatcha think? > > Siggi > > > On Sat, Jul 5, 2014 at 4:03 PM, <siggi@chromium.org> wrote: > >> Thanks! >> >> >> >> https://codereview.chromium.org/349263004/diff/200001/ >> chrome/browser/profiles/profile_browsertest.cc >> File chrome/browser/profiles/profile_browsertest.cc (right): >> >> https://codereview.chromium.org/349263004/diff/200001/ >> chrome/browser/profiles/profile_browsertest.cc#newcode86 >> chrome/browser/profiles/profile_browsertest.cc:86: return ""; >> On 2014/07/04 23:39:27, gab wrote: >> >>> On 2014/07/04 15:22:18, gab wrote: >>> > s/""/std::string()/ >>> > >>> > I forget when I was told this, but Chromium prefers specifically >>> >> using >> >>> > std::string() when specifying empty strings. >>> >> >> Ping >>> >> >> Done. >> >> >> https://codereview.chromium.org/349263004/diff/200001/ >> chrome/browser/profiles/profile_browsertest.cc#newcode92 >> chrome/browser/profiles/profile_browsertest.cc:92: >> base::DictionaryValue* dict; >> On 2014/07/04 23:39:27, gab wrote: >> >>> On 2014/07/04 15:22:17, gab wrote: >>> > Initialize to NULL (otherwise |!dict| can be reading an unitialized >>> >> variable). >> >> Ping >>> >> >> Done. >> >> >> https://codereview.chromium.org/349263004/diff/200001/ >> chrome/browser/profiles/profile_browsertest.cc#newcode345 >> chrome/browser/profiles/profile_browsertest.cc:345: for (size_t i = 0; i >> < loaded_profiles.size(); ++i) { >> On 2014/07/04 15:41:18, Monica Dinculescu wrote: >> >>> Can you add an assert or check that there's at least one loaded >>> >> profile? This >> >>> test becomes moot and always successful if there's no loaded >>> >> profiles...:( >> >> Done. >> >> https://codereview.chromium.org/349263004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hey Siggi, I think that probably makes sense. The UI thread only keeps the pointer in the class' |managed_node_| member for comparison purposes later on (but never reads/writes to it) FWICT. I think the class should instead keep the ID of the node (or a void* as it looks like the ID perhaps changes?); keeping a fully-typed raw pointer that it should never actually deref is just weird and dangerous... Either way, I think this should be fixed as a separate CL, adding the leak annotation with an associated bug is the right thing to do in this CL IMO since this is NOT a new leak. For the ease of merging your fix upstream I suggest you also split off the BookmarkStorage lifetime management change as a precursor CL to this one (it doesn't need to make it upstream as I don't think we run memory bots on browser_tests on branch -- and worst case you can always merge it later to fix bots on branch if need be..). Smaller CLs == better :-)! Cheers, Gab
Ok, I pulled the BookmarkStorage changes out to https://codereview.chromium.org/370323002/. Brett, please give us owner blessing here?
https://codereview.chromium.org/349263004/diff/340001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/349263004/diff/340001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:288: base::KillProcess(base::Process::Current().handle(), 0, false); Instead of killing here, can we let the process continue and cover as much mileage as possible? CloseAllBrowsers() is tangled with App closing and multiple hacks for ASH/Metro close.
thanks https://codereview.chromium.org/349263004/diff/340001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/349263004/diff/340001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:288: base::KillProcess(base::Process::Current().handle(), 0, false); On 2014/07/07 20:02:58, Shrikant Kelkar wrote: > Instead of killing here, can we let the process continue and cover as much > mileage as possible? CloseAllBrowsers() is tangled with App closing and multiple > hacks for ASH/Metro close. I would really rather not do that, as this gives the false impression that useful work can be achieved here, which in turn makes it a certainty that this will regress again. We've been broken on Windows logoff for two years :(, and this code should never run exception as consequence of a WM_ENDSESSION message. Ugh - is this what you're worried about: [https://code.google.com/p/chromium/codesearch#chromium/src/win8/metro_driver/chrome_app_view.cc&q=WM_ENDSESSION&sq=package:chromium&type=cs&l=625]? If this doesn't tolerate a TerminateProcess, then it needs to be rerouted to use another message or some such. The WM_ENDSESSION path is VERY special, as here Windows is going to kill you very shortly after the browser windows are closed. There's also a strict time limit in effect. If we dally, then Windows will fling up a complaint saying something like "Chrome's preventing logoff/shutdown" and give the user the option of killing it anyway. Can you please test whether this causes trouble on Win8 (I don't have hardware to test this), and if so, I can remove the TerminateProcess in the short term. This MUST however be disentangled by using something else than WM_ENDSESSION on Win8.
So cpu and I scrubbed through a bit of code. Apparently the ChromeAppView code that sends WM_ENDSESSION is dead [ https://code.google.com/p/chromium/codesearch#chromium/src/win8/metro_driver/... ]. Nothing else issues WM_ENDSESSION or calls chrome::SessionEnding on Windows. I also looked at the TerminateProcess patch [ https://code.google.com/p/chromium/codesearch#chromium/src/components/breakpa..., and this specifically excludes browsers, so this won't run foul of that. So unless you quickly and vocally object, I'll be landing this change TBR owners in the morning eastern time... On Mon, Jul 7, 2014 at 4:17 PM, <siggi@chromium.org> wrote: > thanks > > > > https://codereview.chromium.org/349263004/diff/340001/ > chrome/browser/lifetime/application_lifetime.cc > File chrome/browser/lifetime/application_lifetime.cc (right): > > https://codereview.chromium.org/349263004/diff/340001/ > chrome/browser/lifetime/application_lifetime.cc#newcode288 > chrome/browser/lifetime/application_lifetime.cc:288: > base::KillProcess(base::Process::Current().handle(), 0, false); > On 2014/07/07 20:02:58, Shrikant Kelkar wrote: > >> Instead of killing here, can we let the process continue and cover as >> > much > >> mileage as possible? CloseAllBrowsers() is tangled with App closing >> > and multiple > >> hacks for ASH/Metro close. >> > > I would really rather not do that, as this gives the false impression > that useful work can be achieved here, which in turn makes it a > certainty that this will regress again. We've been broken on Windows > logoff for two years :(, and this code should never run exception as > consequence of a WM_ENDSESSION message. > > Ugh - is this what you're worried about: > [https://code.google.com/p/chromium/codesearch#chromium/ > src/win8/metro_driver/chrome_app_view.cc&q=WM_ENDSESSION& > sq=package:chromium&type=cs&l=625]? > If this doesn't tolerate a TerminateProcess, then it needs to be > rerouted to use another message or some such. > > The WM_ENDSESSION path is VERY special, as here Windows is going to kill > you very shortly after the browser windows are closed. > There's also a strict time limit in effect. If we dally, then Windows > will fling up a complaint saying something like "Chrome's preventing > logoff/shutdown" and give the user the option of killing it anyway. > > Can you please test whether this causes trouble on Win8 (I don't have > hardware to test this), and if so, I can remove the TerminateProcess in > the short term. This MUST however be disentangled by using something > else than WM_ENDSESSION on Win8. > > https://codereview.chromium.org/349263004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm! I debugged this bit with your patch as I was worrying more about restore tabs during restart to/from metro, which works. On 2014/07/07 21:39:59, Sigurður Ásgeirsson wrote: > So cpu and I scrubbed through a bit of code. Apparently the ChromeAppView > code that sends WM_ENDSESSION is dead [ > https://code.google.com/p/chromium/codesearch#chromium/src/win8/metro_driver/... > ]. > Nothing else issues WM_ENDSESSION or calls chrome::SessionEnding on Windows. > > I also looked at the TerminateProcess patch [ > https://code.google.com/p/chromium/codesearch#chromium/src/components/breakpa..., > and this specifically excludes browsers, so this won't run foul of that. > > So unless you quickly and vocally object, I'll be landing this change TBR > owners in the morning eastern time... > > > > > On Mon, Jul 7, 2014 at 4:17 PM, <mailto:siggi@chromium.org> wrote: > > > thanks > > > > > > > > https://codereview.chromium.org/349263004/diff/340001/ > > chrome/browser/lifetime/application_lifetime.cc > > File chrome/browser/lifetime/application_lifetime.cc (right): > > > > https://codereview.chromium.org/349263004/diff/340001/ > > chrome/browser/lifetime/application_lifetime.cc#newcode288 > > chrome/browser/lifetime/application_lifetime.cc:288: > > base::KillProcess(base::Process::Current().handle(), 0, false); > > On 2014/07/07 20:02:58, Shrikant Kelkar wrote: > > > >> Instead of killing here, can we let the process continue and cover as > >> > > much > > > >> mileage as possible? CloseAllBrowsers() is tangled with App closing > >> > > and multiple > > > >> hacks for ASH/Metro close. > >> > > > > I would really rather not do that, as this gives the false impression > > that useful work can be achieved here, which in turn makes it a > > certainty that this will regress again. We've been broken on Windows > > logoff for two years :(, and this code should never run exception as > > consequence of a WM_ENDSESSION message. > > > > Ugh - is this what you're worried about: > > [https://code.google.com/p/chromium/codesearch#chromium/ > > src/win8/metro_driver/chrome_app_view.cc&q=WM_ENDSESSION& > > sq=package:chromium&type=cs&l=625]? > > If this doesn't tolerate a TerminateProcess, then it needs to be > > rerouted to use another message or some such. > > > > The WM_ENDSESSION path is VERY special, as here Windows is going to kill > > you very shortly after the browser windows are closed. > > There's also a strict time limit in effect. If we dally, then Windows > > will fling up a complaint saying something like "Chrome's preventing > > logoff/shutdown" and give the user the option of killing it anyway. > > > > Can you please test whether this causes trouble on Win8 (I don't have > > hardware to test this), and if so, I can remove the TerminateProcess in > > the short term. This MUST however be disentangled by using something > > else than WM_ENDSESSION on Win8. > > > > https://codereview.chromium.org/349263004/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Yea tabs to be restored are stored in prefs which are explicitly flushed in EndSession() before this kill so no reason this would break. Cheers! Gab Le 2014-07-07 19:05, <shrikant@chromium.org> a écrit : > lgtm! > > I debugged this bit with your patch as I was worrying more about restore > tabs > during restart to/from metro, which works. > > > On 2014/07/07 21:39:59, Sigurður Ásgeirsson wrote: > >> So cpu and I scrubbed through a bit of code. Apparently the ChromeAppView >> code that sends WM_ENDSESSION is dead [ >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/win8/metro_driver/chrome_app_view.cc&q=WM_ENDSESSION& > sq=package:chromium&type=cs&l=626 > >> ]. >> Nothing else issues WM_ENDSESSION or calls chrome::SessionEnding on >> Windows. >> > > I also looked at the TerminateProcess patch [ >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/components/breakpad/app/breakpad_win.cc&q= > InitTerminateProcessHooks&sq=package:chromium&type=cs&l=587], > >> and this specifically excludes browsers, so this won't run foul of that. >> > > So unless you quickly and vocally object, I'll be landing this change TBR >> owners in the morning eastern time... >> > > > > > On Mon, Jul 7, 2014 at 4:17 PM, <mailto:siggi@chromium.org> wrote: >> > > > thanks >> > >> > >> > >> > https://codereview.chromium.org/349263004/diff/340001/ >> > chrome/browser/lifetime/application_lifetime.cc >> > File chrome/browser/lifetime/application_lifetime.cc (right): >> > >> > https://codereview.chromium.org/349263004/diff/340001/ >> > chrome/browser/lifetime/application_lifetime.cc#newcode288 >> > chrome/browser/lifetime/application_lifetime.cc:288: >> > base::KillProcess(base::Process::Current().handle(), 0, false); >> > On 2014/07/07 20:02:58, Shrikant Kelkar wrote: >> > >> >> Instead of killing here, can we let the process continue and cover as >> >> >> > much >> > >> >> mileage as possible? CloseAllBrowsers() is tangled with App closing >> >> >> > and multiple >> > >> >> hacks for ASH/Metro close. >> >> >> > >> > I would really rather not do that, as this gives the false impression >> > that useful work can be achieved here, which in turn makes it a >> > certainty that this will regress again. We've been broken on Windows >> > logoff for two years :(, and this code should never run exception as >> > consequence of a WM_ENDSESSION message. >> > >> > Ugh - is this what you're worried about: >> > [https://code.google.com/p/chromium/codesearch#chromium/ >> > src/win8/metro_driver/chrome_app_view.cc&q=WM_ENDSESSION& >> > sq=package:chromium&type=cs&l=625]? >> > If this doesn't tolerate a TerminateProcess, then it needs to be >> > rerouted to use another message or some such. >> > >> > The WM_ENDSESSION path is VERY special, as here Windows is going to kill >> > you very shortly after the browser windows are closed. >> > There's also a strict time limit in effect. If we dally, then Windows >> > will fling up a complaint saying something like "Chrome's preventing >> > logoff/shutdown" and give the user the option of killing it anyway. >> > >> > Can you please test whether this causes trouble on Win8 (I don't have >> > hardware to test this), and if so, I can remove the TerminateProcess in >> > the short term. This MUST however be disentangled by using something >> > else than WM_ENDSESSION on Win8. >> > >> > https://codereview.chromium.org/349263004/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/349263004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@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/349263004/360001
Message was sent while issue was closed.
Change committed as 281746
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/378863003/ by maniscalco@chromium.org. The reason for reverting is: Suspected as cause of browser_tests failures on Mac10.7 Tests: http://build.chromium.org/p/chromium.mac/builders/Mac10.7%20Tests%20%283%29/b... ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession (run #1): [ RUN ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession [10963:519:0708/084058:ERROR:browser_process_impl.cc(495)] Not implemented reached in virtual void BrowserProcessImpl::EndSession() ../../chrome/browser/profiles/profile_browsertest.cc:365: Failure Value of: "SessionEnded" Expected: GetExitTypePreferenceFromDisk(profile) Which is: "Crashed" [ FAILED ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession, where TypeParam = and GetParam() = (780 ms) ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession (run #2): [ RUN ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession [12926:519:0708/084834:ERROR:browser_process_impl.cc(495)] Not implemented reached in virtual void BrowserProcessImpl::EndSession() ../../chrome/browser/profiles/profile_browsertest.cc:365: Failure Value of: "SessionEnded" Expected: GetExitTypePreferenceFromDisk(profile) Which is: "Crashed" [ FAILED ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession, where TypeParam = and GetParam() = (639 ms) ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession (run #3): [ RUN ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession [12935:519:0708/084836:ERROR:browser_process_impl.cc(495)] Not implemented reached in virtual void BrowserProcessImpl::EndSession() ../../chrome/browser/profiles/profile_browsertest.cc:365: Failure Value of: "SessionEnded" Expected: GetExitTypePreferenceFromDisk(profile) Which is: "Crashed" [ FAILED ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession, where TypeParam = and GetParam() = (551 ms) ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession (run #4): [ RUN ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession [12937:519:0708/084837:ERROR:browser_process_impl.cc(495)] Not implemented reached in virtual void BrowserProcessImpl::EndSession() ../../chrome/browser/profiles/profile_browsertest.cc:365: Failure Value of: "SessionEnded" Expected: GetExitTypePreferenceFromDisk(profile) Which is: "Crashed" [ FAILED ] ProfileBrowserTest.WritesProfilesSynchronouslyOnEndSession, where TypeParam = and GetParam() = (560 ms).
Message was sent while issue was closed.
+rsesek; see question below. Thanks! Gab https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:478: #if defined(USE_X11) || defined(OS_WIN) Looks like the failure on Mac is because the wait is only on X11/OS_WIN (i.e. *not* Mac). Any reason to not wait on all platforms? I know Mac is weird in that it doesn't end process when all windows are closed... perhaps this is related? Is EndSession() intentionally different on Mac? +rsesek ^^?
Message was sent while issue was closed.
Also, PS: I think it's fine to ifdef that test for X11/OS_WIN for now just like the existing EndSession() code but it still feels weird that Mac/etc. wouldn't require the same synchronized flush on EndSession(). On 2014/07/08 16:24:03, gab wrote: > +rsesek; see question below. > > Thanks! > Gab > > https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... > File chrome/browser/browser_process_impl.cc (right): > > https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... > chrome/browser/browser_process_impl.cc:478: #if defined(USE_X11) || > defined(OS_WIN) > Looks like the failure on Mac is because the wait is only on X11/OS_WIN (i.e. > *not* Mac). > > Any reason to not wait on all platforms? I know Mac is weird in that it doesn't > end process when all windows are closed... perhaps this is related? Is > EndSession() intentionally different on Mac? > > +rsesek ^^?
https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:478: #if defined(USE_X11) || defined(OS_WIN) On 2014/07/08 16:24:03, gab wrote: > Looks like the failure on Mac is because the wait is only on X11/OS_WIN (i.e. > *not* Mac). > > Any reason to not wait on all platforms? I know Mac is weird in that it doesn't > end process when all windows are closed... perhaps this is related? Is > EndSession() intentionally different on Mac? > > +rsesek ^^? AFAICT, nothing on Mac even calls EndSession.
https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/349263004/diff/360001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:478: #if defined(USE_X11) || defined(OS_WIN) On 2014/07/08 16:29:38, rsesek wrote: > On 2014/07/08 16:24:03, gab wrote: > > Looks like the failure on Mac is because the wait is only on X11/OS_WIN (i.e. > > *not* Mac). > > > > Any reason to not wait on all platforms? I know Mac is weird in that it > doesn't > > end process when all windows are closed... perhaps this is related? Is > > EndSession() intentionally different on Mac? > > > > +rsesek ^^? > > AFAICT, nothing on Mac even calls EndSession. Ah ok, then feels like this whole method should be ifdef'ed, only ifdef'ing the wait feels weird (i.e. we flush above for all platforms, but only wait for the flush to complete on some platforms...).
Hey Gab - PTAL?
On 2014/07/08 16:58:30, Sigurður Ásgeirsson wrote: > Hey Gab - PTAL? lgtm since EndSession() was already only waiting on USE_X11 || OS_WIN; but I don't understand why that is... Robert says Mac doesn't go through EndSession(), I could see that, but I see no cases where we'd want to flush (1st part of this method), but not wait (2nd part of this method which is ifdef'ed). Let's investigate as a follow-up. Cheers, Gab
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/349263004/380001
On Tue, Jul 8, 2014 at 1:04 PM, <gab@chromium.org> wrote: > On 2014/07/08 16:58:30, Sigurður Ásgeirsson wrote: > >> Hey Gab - PTAL? >> > > lgtm since EndSession() was already only waiting on USE_X11 || OS_WIN; but > I > don't understand why that is... Robert says Mac doesn't go through > EndSession(), > I could see that, but I see no cases where we'd want to flush (1st part of > this > method), but not wait (2nd part of this method which is ifdef'ed). > > Well - it depends. If the program winds down in an orderly fashion the blocking thread pool will wait for completion of the flush tasks. I think perhaps it's safe to not block under USE_X11, and only do that on Windows. This has the advantage of expediting shutdown, as then the writes can be paralleled with other windup. > Let's investigate as a follow-up. > > Cheers, > Gab > > https://codereview.chromium.org/349263004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Also, the test failure is hitting NOTREACHED() on Mac :). On Tue, Jul 8, 2014 at 1:09 PM, Sigurður Ásgeirsson <siggi@chromium.org> wrote: > On Tue, Jul 8, 2014 at 1:04 PM, <gab@chromium.org> wrote: > >> On 2014/07/08 16:58:30, Sigurður Ásgeirsson wrote: >> >>> Hey Gab - PTAL? >>> >> >> lgtm since EndSession() was already only waiting on USE_X11 || OS_WIN; >> but I >> don't understand why that is... Robert says Mac doesn't go through >> EndSession(), >> I could see that, but I see no cases where we'd want to flush (1st part >> of this >> method), but not wait (2nd part of this method which is ifdef'ed). >> >> Well - it depends. If the program winds down in an orderly fashion the > blocking thread pool will wait for completion of the flush tasks. > I think perhaps it's safe to not block under USE_X11, and only do that on > Windows. > This has the advantage of expediting shutdown, as then the writes can be > paralleled with other windup. > > >> Let's investigate as a follow-up. >> >> Cheers, >> Gab >> >> https://codereview.chromium.org/349263004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@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/349263004/400001
On 2014/07/08 17:09:45, Sigurður Ásgeirsson wrote: > On Tue, Jul 8, 2014 at 1:04 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > On 2014/07/08 16:58:30, Sigurður Ásgeirsson wrote: > > > >> Hey Gab - PTAL? > >> > > > > lgtm since EndSession() was already only waiting on USE_X11 || OS_WIN; but > > I > > don't understand why that is... Robert says Mac doesn't go through > > EndSession(), > > I could see that, but I see no cases where we'd want to flush (1st part of > > this > > method), but not wait (2nd part of this method which is ifdef'ed). > > > > Well - it depends. If the program winds down in an orderly fashion the > blocking thread pool will wait for completion of the flush tasks. > I think perhaps it's safe to not block under USE_X11, and only do that on > Windows. > This has the advantage of expediting shutdown, as then the writes can be > paralleled with other windup. I see, well whatever we do should be better tested the current code is..! > > > > Let's investigate as a follow-up. > > > > Cheers, > > Gab > > > > https://codereview.chromium.org/349263004/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
On 2014/07/08 17:23:45, Sigurður Ásgeirsson wrote: > Also, the test failure is hitting NOTREACHED() on Mac :). Is it? I thought I'd seen it hit NOTIMPLEMENTED() which is meant to be a benign FYI... Anyways, latest patch set lgtm.
Looks like it got reverted, see 386126 for details.
Yups, I'm re-submitting with a fix... On Tue, Jul 8, 2014 at 2:00 PM, <cpu@google.com> wrote: > Looks like it got reverted, see 386126 for details. > > https://codereview.chromium.org/349263004/ > 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 281807 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
