Register signal handlers after shutdown pipe is setup.

Register signal handlers after shutdown pipe is setup.

Chrome fails at RAW_CHECK(g_shutdown_pipe_write_fd != -1) if it receives signal in a window
where signal handlers are registered, but shutdown pipe isn't setup yet.

BUG=chromium-os:17606
TEST=autotest with session manager sending SIGTERM no longer crash

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100739

[Chris Masone, 2011-07-21 01:40:19]

On 2011/07/20 20:15:27, oshima wrote:

This seems to Chrome OS in a state where a SIGTERM that happens too early gets
ignored or something by CHrome.  Chrome gets the signal, nothing seems to
happen, and then we time out and SIGABRT the browser.

[viettrungluu, 2011-07-21 18:00:20]

I think this is okay. Things obviously currently don't/can't work properly in
the window before the shutdown pipe is set up ... though I wonder if we
shouldn't just make the signal handlers do something else sensible when the
shutdown pipe hasn't been set up yet? (It could perhaps set up a deferred
graceful shutdown.)

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
File chrome/browser/browser_main_posix.cc (right):

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
chrome/browser/browser_main_posix.cc:30: // See comment in
|PreEarlyInitialization()|, where sigaction is called.
Update this comment.

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
chrome/browser/browser_main_posix.cc:58: // See comment in
|PreEarlyInitialization()|, where sigaction is called.
Ditto, etc.

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
chrome/browser/browser_main_posix.cc:235: // Setup signal handlers AFTER
shutodwn pipe is setup because
s/shutodwn/shutdown/

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
chrome/browser/browser_main_posix.cc:238: // We need to accept SIGCHLD, even
though our handler is a no-op because
We can presumably set up the SIGCHLD handler earlier, right? It's only the
signal handlers which call GracefulShutdownHandler() which need to be set up
after the pipe.

[Chris Masone, 2011-07-21 18:03:07]

On 2011/07/21 18:00:20, viettrungluu wrote:
> I think this is okay. Things obviously currently don't/can't work properly in
> the window before the shutdown pipe is set up ... though I wonder if we
> shouldn't just make the signal handlers do something else sensible when the
> shutdown pipe hasn't been set up yet? (It could perhaps set up a deferred
> graceful shutdown.)
> 

It's ok in the sense that it's a reasonable behavior in a live use case,
certainly.  The problem is that we trigger this case during our integration
tests, which leads to us detecting a SIGABRT, which leads to us failing the
tests :-(

So, I'm hoping we can figure out a sensible, graceful thing to do that will
address that case as well :-)

>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> File chrome/browser/browser_main_posix.cc (right):
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> chrome/browser/browser_main_posix.cc:30: // See comment in
> |PreEarlyInitialization()|, where sigaction is called.
> Update this comment.
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> chrome/browser/browser_main_posix.cc:58: // See comment in
> |PreEarlyInitialization()|, where sigaction is called.
> Ditto, etc.
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> chrome/browser/browser_main_posix.cc:235: // Setup signal handlers AFTER
> shutodwn pipe is setup because
> s/shutodwn/shutdown/
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> chrome/browser/browser_main_posix.cc:238: // We need to accept SIGCHLD, even
> though our handler is a no-op because
> We can presumably set up the SIGCHLD handler earlier, right? It's only the
> signal handlers which call GracefulShutdownHandler() which need to be set up
> after the pipe.

[oshima, 2011-07-22 01:36:30]

We can catch signal early, but there is little we can do there other than simply
exit because message loop doesn't exit yet.
 Since this is extremely rare case that happens on test, 
I prefer simpler solution.

Chris, I found why chrome sometimes does not exit in 3s.
There is a race where processe's module ref count does not
become zero when the login process is created after closeallbrowsers get called.
See changes in login code.

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
File chrome/browser/browser_main_posix.cc (right):

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
chrome/browser/browser_main_posix.cc:30: // See comment in
|PreEarlyInitialization()|, where sigaction is called.
On 2011/07/21 18:00:20, viettrungluu wrote:
> Update this comment.

SIGCHLDHandler was moved back to PreEarlyInitialization

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
chrome/browser/browser_main_posix.cc:58: // See comment in
|PreEarlyInitialization()|, where sigaction is called.
On 2011/07/21 18:00:20, viettrungluu wrote:
> Ditto, etc.

Done.

http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
chrome/browser/browser_main_posix.cc:238: // We need to accept SIGCHLD, even
though our handler is a no-op because
On 2011/07/21 18:00:20, viettrungluu wrote:
> We can presumably set up the SIGCHLD handler earlier, right? It's only the
> signal handlers which call GracefulShutdownHandler() which need to be set up
> after the pipe.

You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.

[Chris Masone, 2011-07-22 16:53:12]

I got this CHECK() failure when I patched this into a build from yesterday:

[21181:21200:167439799220:FATAL:login_utils.cc(100)] Check failed: getter. 

Testing again with a ToT build.  Oshima, I put my login_manger.git patch up here
http://gerrit.chromium.org/gerrit/#change,4579 if you want to use that for
testing.

On 2011/07/22 01:36:30, oshima wrote:
> We can catch signal early, but there is little we can do there other than
simply
> exit because message loop doesn't exit yet.
>  Since this is extremely rare case that happens on test, 
> I prefer simpler solution.
> 
> Chris, I found why chrome sometimes does not exit in 3s.
> There is a race where processe's module ref count does not
> become zero when the login process is created after closeallbrowsers get
called.
> See changes in login code.
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> File chrome/browser/browser_main_posix.cc (right):
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> chrome/browser/browser_main_posix.cc:30: // See comment in
> |PreEarlyInitialization()|, where sigaction is called.
> On 2011/07/21 18:00:20, viettrungluu wrote:
> > Update this comment.
> 
> SIGCHLDHandler was moved back to PreEarlyInitialization
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> chrome/browser/browser_main_posix.cc:58: // See comment in
> |PreEarlyInitialization()|, where sigaction is called.
> On 2011/07/21 18:00:20, viettrungluu wrote:
> > Ditto, etc.
> 
> Done.
> 
>
http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_pos...
> chrome/browser/browser_main_posix.cc:238: // We need to accept SIGCHLD, even
> though our handler is a no-op because
> On 2011/07/21 18:00:20, viettrungluu wrote:
> > We can presumably set up the SIGCHLD handler earlier, right? It's only the
> > signal handlers which call GracefulShutdownHandler() which need to be set up
> > after the pipe.
> 
> You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.

[oshima, 2011-07-22 16:58:35]

On Fri, Jul 22, 2011 at 9:53 AM, <cmasone@chromium.org> wrote:

> I got this CHECK() failure when I patched this into a build from yesterday:
>

Can you send me CHECK message? i'm looking at blocker right now and don't
have
bandwidth to repro it myself.



>
> [21181:21200:167439799220:**FATAL:login_utils.cc(100)] Check failed:
> getter.
>
> Testing again with a ToT build.  Oshima, I put my login_manger.git patch up
> here
>
http://gerrit.chromium.org/**gerrit/#change,4579<http://gerrit.chromium.org/g...
you want to use that for
> testing.
>
> On 2011/07/22 01:36:30, oshima wrote:
>
>> We can catch signal early, but there is little we can do there other than
>>
> simply
>
>> exit because message loop doesn't exit yet.
>>  Since this is extremely rare case that happens on test,
>> I prefer simpler solution.
>>
>
>  Chris, I found why chrome sometimes does not exit in 3s.
>> There is a race where processe's module ref count does not
>> become zero when the login process is created after closeallbrowsers get
>>
> called.
>
>> See changes in login code.
>>
>
>
> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>
browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
>
>> File chrome/browser/browser_main_**posix.cc (right):
>>
>
>
> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>
browser/browser_main_posix.cc#**newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
>
>> chrome/browser/browser_main_**posix.cc:30: // See comment in
>> |PreEarlyInitialization()|, where sigaction is called.
>> On 2011/07/21 18:00:20, viettrungluu wrote:
>> > Update this comment.
>>
>
>  SIGCHLDHandler was moved back to PreEarlyInitialization
>>
>
>
> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>
browser/browser_main_posix.cc#**newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
>
>> chrome/browser/browser_main_**posix.cc:58: // See comment in
>> |PreEarlyInitialization()|, where sigaction is called.
>> On 2011/07/21 18:00:20, viettrungluu wrote:
>> > Ditto, etc.
>>
>
>  Done.
>>
>
>
> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>
browser/browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
>
>> chrome/browser/browser_main_**posix.cc:238: // We need to accept SIGCHLD,
>> even
>> though our handler is a no-op because
>> On 2011/07/21 18:00:20, viettrungluu wrote:
>> > We can presumably set up the SIGCHLD handler earlier, right? It's only
>> the
>> > signal handlers which call GracefulShutdownHandler() which need to be
>> set up
>> > after the pipe.
>>
>
>  You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.
>>
>
>
>
>
http://codereview.chromium.**org/7464013/<http://codereview.chromium.org/7464...
>

[Chris Masone, 2011-07-22 17:10:40]

I already did...[21181:21200:167439799220:FATAL:login_utils.cc(100)] Check
failed: getter.

On Fri, Jul 22, 2011 at 9:58 AM, oshima <oshima@chromium.org> wrote:

>
>
> On Fri, Jul 22, 2011 at 9:53 AM, <cmasone@chromium.org> wrote:
>
>> I got this CHECK() failure when I patched this into a build from
>> yesterday:
>>
>
> Can you send me CHECK message? i'm looking at blocker right now and don't
> have
> bandwidth to repro it myself.
>
>
>
>>
>> [21181:21200:167439799220:**FATAL:login_utils.cc(100)] Check failed:
>> getter.
>>
>> Testing again with a ToT build.  Oshima, I put my login_manger.git patch
>> up here
>>
http://gerrit.chromium.org/**gerrit/#change,4579<http://gerrit.chromium.org/g...
you want to use that for
>> testing.
>>
>> On 2011/07/22 01:36:30, oshima wrote:
>>
>>> We can catch signal early, but there is little we can do there other than
>>>
>> simply
>>
>>> exit because message loop doesn't exit yet.
>>>  Since this is extremely rare case that happens on test,
>>> I prefer simpler solution.
>>>
>>
>>  Chris, I found why chrome sometimes does not exit in 3s.
>>> There is a race where processe's module ref count does not
>>> become zero when the login process is created after closeallbrowsers get
>>>
>> called.
>>
>>> See changes in login code.
>>>
>>
>>
>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>
browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
>>
>>> File chrome/browser/browser_main_**posix.cc (right):
>>>
>>
>>
>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>
browser/browser_main_posix.cc#**newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
>>
>>> chrome/browser/browser_main_**posix.cc:30: // See comment in
>>> |PreEarlyInitialization()|, where sigaction is called.
>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>> > Update this comment.
>>>
>>
>>  SIGCHLDHandler was moved back to PreEarlyInitialization
>>>
>>
>>
>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>
browser/browser_main_posix.cc#**newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
>>
>>> chrome/browser/browser_main_**posix.cc:58: // See comment in
>>> |PreEarlyInitialization()|, where sigaction is called.
>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>> > Ditto, etc.
>>>
>>
>>  Done.
>>>
>>
>>
>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>
browser/browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
>>
>>> chrome/browser/browser_main_**posix.cc:238: // We need to accept
>>> SIGCHLD, even
>>> though our handler is a no-op because
>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>> > We can presumably set up the SIGCHLD handler earlier, right? It's only
>>> the
>>> > signal handlers which call GracefulShutdownHandler() which need to be
>>> set up
>>> > after the pipe.
>>>
>>
>>  You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/7464013/<http://codereview.chromium.org/7464...
>>
>
>

[oshima, 2011-07-22 17:12:33]

Oh, sorry about that. I guess i'm stressed out now :(

- oshima

On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone <cmasone@chromium.org> wrote:

> I already did...[21181:21200:167439799220:FATAL:login_utils.cc(100)] Check
> failed: getter.
>
>
> On Fri, Jul 22, 2011 at 9:58 AM, oshima <oshima@chromium.org> wrote:
>
>>
>>
>> On Fri, Jul 22, 2011 at 9:53 AM, <cmasone@chromium.org> wrote:
>>
>>> I got this CHECK() failure when I patched this into a build from
>>> yesterday:
>>>
>>
>> Can you send me CHECK message? i'm looking at blocker right now and don't
>> have
>> bandwidth to repro it myself.
>>
>>
>>
>>>
>>> [21181:21200:167439799220:**FATAL:login_utils.cc(100)] Check failed:
>>> getter.
>>>
>>> Testing again with a ToT build.  Oshima, I put my login_manger.git patch
>>> up here
>>>
http://gerrit.chromium.org/**gerrit/#change,4579<http://gerrit.chromium.org/g...
you want to use that for
>>> testing.
>>>
>>> On 2011/07/22 01:36:30, oshima wrote:
>>>
>>>> We can catch signal early, but there is little we can do there other
>>>> than
>>>>
>>> simply
>>>
>>>> exit because message loop doesn't exit yet.
>>>>  Since this is extremely rare case that happens on test,
>>>> I prefer simpler solution.
>>>>
>>>
>>>  Chris, I found why chrome sometimes does not exit in 3s.
>>>> There is a race where processe's module ref count does not
>>>> become zero when the login process is created after closeallbrowsers get
>>>>
>>> called.
>>>
>>>> See changes in login code.
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>
browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
>>>
>>>> File chrome/browser/browser_main_**posix.cc (right):
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>
browser/browser_main_posix.cc#**newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
>>>
>>>> chrome/browser/browser_main_**posix.cc:30: // See comment in
>>>> |PreEarlyInitialization()|, where sigaction is called.
>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>> > Update this comment.
>>>>
>>>
>>>  SIGCHLDHandler was moved back to PreEarlyInitialization
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>
browser/browser_main_posix.cc#**newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
>>>
>>>> chrome/browser/browser_main_**posix.cc:58: // See comment in
>>>> |PreEarlyInitialization()|, where sigaction is called.
>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>> > Ditto, etc.
>>>>
>>>
>>>  Done.
>>>>
>>>
>>>
>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>
browser/browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
>>>
>>>> chrome/browser/browser_main_**posix.cc:238: // We need to accept
>>>> SIGCHLD, even
>>>> though our handler is a no-op because
>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>> > We can presumably set up the SIGCHLD handler earlier, right? It's only
>>>> the
>>>> > signal handlers which call GracefulShutdownHandler() which need to be
>>>> set up
>>>> > after the pipe.
>>>>
>>>
>>>  You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.
>>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/7464013/<http://codereview.chromium.org/7464...
>>>
>>
>>
>

[Chris Masone, 2011-07-22 17:14:18]

Sorry, man :-(

On a ToT build I get this CHECK():

[22574:22574:172453820921:FATAL:content_settings_utils.cc(74)] Check failed:
pattern_pair.second.IsValid().
Unable to get symbols for backtrace. Dumping raw addresses in trace:


On Fri, Jul 22, 2011 at 10:12 AM, oshima <oshima@chromium.org> wrote:

> Oh, sorry about that. I guess i'm stressed out now :(
>
> - oshima
>
>
> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone <cmasone@chromium.org>wrote:
>
>> I already did...[21181:21200:167439799220:FATAL:login_utils.cc(100)] Check
>> failed: getter.
>>
>>
>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <oshima@chromium.org> wrote:
>>
>>>
>>>
>>> On Fri, Jul 22, 2011 at 9:53 AM, <cmasone@chromium.org> wrote:
>>>
>>>> I got this CHECK() failure when I patched this into a build from
>>>> yesterday:
>>>>
>>>
>>> Can you send me CHECK message? i'm looking at blocker right now and don't
>>> have
>>> bandwidth to repro it myself.
>>>
>>>
>>>
>>>>
>>>> [21181:21200:167439799220:**FATAL:login_utils.cc(100)] Check failed:
>>>> getter.
>>>>
>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git patch
>>>> up here
>>>>
http://gerrit.chromium.org/**gerrit/#change,4579<http://gerrit.chromium.org/g...
you want to use that for
>>>> testing.
>>>>
>>>> On 2011/07/22 01:36:30, oshima wrote:
>>>>
>>>>> We can catch signal early, but there is little we can do there other
>>>>> than
>>>>>
>>>> simply
>>>>
>>>>> exit because message loop doesn't exit yet.
>>>>>  Since this is extremely rare case that happens on test,
>>>>> I prefer simpler solution.
>>>>>
>>>>
>>>>  Chris, I found why chrome sometimes does not exit in 3s.
>>>>> There is a race where processe's module ref count does not
>>>>> become zero when the login process is created after closeallbrowsers
>>>>> get
>>>>>
>>>> called.
>>>>
>>>>> See changes in login code.
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>
browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
>>>>
>>>>> File chrome/browser/browser_main_**posix.cc (right):
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>
browser/browser_main_posix.cc#**newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
>>>>
>>>>> chrome/browser/browser_main_**posix.cc:30: // See comment in
>>>>> |PreEarlyInitialization()|, where sigaction is called.
>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>>> > Update this comment.
>>>>>
>>>>
>>>>  SIGCHLDHandler was moved back to PreEarlyInitialization
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>
browser/browser_main_posix.cc#**newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
>>>>
>>>>> chrome/browser/browser_main_**posix.cc:58: // See comment in
>>>>> |PreEarlyInitialization()|, where sigaction is called.
>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>>> > Ditto, etc.
>>>>>
>>>>
>>>>  Done.
>>>>>
>>>>
>>>>
>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>
browser/browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
>>>>
>>>>> chrome/browser/browser_main_**posix.cc:238: // We need to accept
>>>>> SIGCHLD, even
>>>>> though our handler is a no-op because
>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>>> > We can presumably set up the SIGCHLD handler earlier, right? It's
>>>>> only the
>>>>> > signal handlers which call GracefulShutdownHandler() which need to be
>>>>> set up
>>>>> > after the pipe.
>>>>>
>>>>
>>>>  You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.
>>>>>
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.**org/7464013/<http://codereview.chromium.org/7464...
>>>>
>>>
>>>
>>
>

[oshima, 2011-07-22 17:20:54]

Actually, http://codereview.chromium.org/7466030 might have fixed that
CHECK.
But again, it may have another CHECK error. I'll try it later.

- oshima

On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <cmasone@chromium.org> wrote:

> Sorry, man :-(
>
> On a ToT build I get this CHECK():
>
> [22574:22574:172453820921:FATAL:content_settings_utils.cc(74)] Check
> failed: pattern_pair.second.IsValid().
> Unable to get symbols for backtrace. Dumping raw addresses in trace:
>
>
> On Fri, Jul 22, 2011 at 10:12 AM, oshima <oshima@chromium.org> wrote:
>
>> Oh, sorry about that. I guess i'm stressed out now :(
>>
>> - oshima
>>
>>
>> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone <cmasone@chromium.org>wrote:
>>
>>> I already did...[21181:21200:167439799220:FATAL:login_utils.cc(100)]
>>> Check failed: getter.
>>>
>>>
>>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <oshima@chromium.org> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Jul 22, 2011 at 9:53 AM, <cmasone@chromium.org> wrote:
>>>>
>>>>> I got this CHECK() failure when I patched this into a build from
>>>>> yesterday:
>>>>>
>>>>
>>>> Can you send me CHECK message? i'm looking at blocker right now and
>>>> don't have
>>>> bandwidth to repro it myself.
>>>>
>>>>
>>>>
>>>>>
>>>>> [21181:21200:167439799220:**FATAL:login_utils.cc(100)] Check failed:
>>>>> getter.
>>>>>
>>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git
>>>>> patch up here
>>>>>
http://gerrit.chromium.org/**gerrit/#change,4579<http://gerrit.chromium.org/g...
you want to use that for
>>>>> testing.
>>>>>
>>>>> On 2011/07/22 01:36:30, oshima wrote:
>>>>>
>>>>>> We can catch signal early, but there is little we can do there other
>>>>>> than
>>>>>>
>>>>> simply
>>>>>
>>>>>> exit because message loop doesn't exit yet.
>>>>>>  Since this is extremely rare case that happens on test,
>>>>>> I prefer simpler solution.
>>>>>>
>>>>>
>>>>>  Chris, I found why chrome sometimes does not exit in 3s.
>>>>>> There is a race where processe's module ref count does not
>>>>>> become zero when the login process is created after closeallbrowsers
>>>>>> get
>>>>>>
>>>>> called.
>>>>>
>>>>>> See changes in login code.
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>>
browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
>>>>>
>>>>>> File chrome/browser/browser_main_**posix.cc (right):
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>>
browser/browser_main_posix.cc#**newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
>>>>>
>>>>>> chrome/browser/browser_main_**posix.cc:30: // See comment in
>>>>>> |PreEarlyInitialization()|, where sigaction is called.
>>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>>>> > Update this comment.
>>>>>>
>>>>>
>>>>>  SIGCHLDHandler was moved back to PreEarlyInitialization
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>>
browser/browser_main_posix.cc#**newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
>>>>>
>>>>>> chrome/browser/browser_main_**posix.cc:58: // See comment in
>>>>>> |PreEarlyInitialization()|, where sigaction is called.
>>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>>>> > Ditto, etc.
>>>>>>
>>>>>
>>>>>  Done.
>>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
>>>>>
browser/browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
>>>>>
>>>>>> chrome/browser/browser_main_**posix.cc:238: // We need to accept
>>>>>> SIGCHLD, even
>>>>>> though our handler is a no-op because
>>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>>>>> > We can presumably set up the SIGCHLD handler earlier, right? It's
>>>>>> only the
>>>>>> > signal handlers which call GracefulShutdownHandler() which need to
>>>>>> be set up
>>>>>> > after the pipe.
>>>>>>
>>>>>
>>>>>  You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
http://codereview.chromium.**org/7464013/<http://codereview.chromium.org/7464...
>>>>>
>>>>
>>>>
>>>
>>
>

[oshima, 2011-07-22 23:04:11]

Run suite_Smoke and passed.

Here is what i did.

* install latest test image
* gmerge chromeos-chrome with local_source +  my change
* cros_workon chromeos-login
* apply Chri's patch using the git command on gerrit
* gmerge chromeos-login
* run_remote_test.sh ... suite_Smoke

Chris, let me know if i missed something.









On 2011/07/22 17:20:54, oshima wrote:
> Actually, http://codereview.chromium.org/7466030 might have fixed that
> CHECK.
> But again, it may have another CHECK error. I'll try it later.
> 
> - oshima
> 
> On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <mailto:cmasone@chromium.org>
wrote:
> 
> > Sorry, man :-(
> >
> > On a ToT build I get this CHECK():
> >
> > [22574:22574:172453820921:FATAL:content_settings_utils.cc(74)] Check
> > failed: pattern_pair.second.IsValid().
> > Unable to get symbols for backtrace. Dumping raw addresses in trace:
> >
> >
> > On Fri, Jul 22, 2011 at 10:12 AM, oshima <mailto:oshima@chromium.org> wrote:
> >
> >> Oh, sorry about that. I guess i'm stressed out now :(
> >>
> >> - oshima
> >>
> >>
> >> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone <cmasone@chromium.org>wrote:
> >>
> >>> I already did...[21181:21200:167439799220:FATAL:login_utils.cc(100)]
> >>> Check failed: getter.
> >>>
> >>>
> >>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <mailto:oshima@chromium.org>
wrote:
> >>>
> >>>>
> >>>>
> >>>> On Fri, Jul 22, 2011 at 9:53 AM, <mailto:cmasone@chromium.org> wrote:
> >>>>
> >>>>> I got this CHECK() failure when I patched this into a build from
> >>>>> yesterday:
> >>>>>
> >>>>
> >>>> Can you send me CHECK message? i'm looking at blocker right now and
> >>>> don't have
> >>>> bandwidth to repro it myself.
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>> [21181:21200:167439799220:**FATAL:login_utils.cc(100)] Check failed:
> >>>>> getter.
> >>>>>
> >>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git
> >>>>> patch up here
> >>>>>
>
http://gerrit.chromium.org/**gerrit/#change%2C4579%3Chttp://gerrit.chromium.o...
> you want to use that for
> >>>>> testing.
> >>>>>
> >>>>> On 2011/07/22 01:36:30, oshima wrote:
> >>>>>
> >>>>>> We can catch signal early, but there is little we can do there other
> >>>>>> than
> >>>>>>
> >>>>> simply
> >>>>>
> >>>>>> exit because message loop doesn't exit yet.
> >>>>>>  Since this is extremely rare case that happens on test,
> >>>>>> I prefer simpler solution.
> >>>>>>
> >>>>>
> >>>>>  Chris, I found why chrome sometimes does not exit in 3s.
> >>>>>> There is a race where processe's module ref count does not
> >>>>>> become zero when the login process is created after closeallbrowsers
> >>>>>> get
> >>>>>>
> >>>>> called.
> >>>>>
> >>>>>> See changes in login code.
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> >>>>>
>
browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
> >>>>>
> >>>>>> File chrome/browser/browser_main_**posix.cc (right):
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> >>>>>
>
browser/browser_main_posix.cc#**newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
> >>>>>
> >>>>>> chrome/browser/browser_main_**posix.cc:30: // See comment in
> >>>>>> |PreEarlyInitialization()|, where sigaction is called.
> >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
> >>>>>> > Update this comment.
> >>>>>>
> >>>>>
> >>>>>  SIGCHLDHandler was moved back to PreEarlyInitialization
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> >>>>>
>
browser/browser_main_posix.cc#**newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
> >>>>>
> >>>>>> chrome/browser/browser_main_**posix.cc:58: // See comment in
> >>>>>> |PreEarlyInitialization()|, where sigaction is called.
> >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
> >>>>>> > Ditto, etc.
> >>>>>>
> >>>>>
> >>>>>  Done.
> >>>>>>
> >>>>>
> >>>>>
> >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> >>>>>
>
browser/browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
> >>>>>
> >>>>>> chrome/browser/browser_main_**posix.cc:238: // We need to accept
> >>>>>> SIGCHLD, even
> >>>>>> though our handler is a no-op because
> >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
> >>>>>> > We can presumably set up the SIGCHLD handler earlier, right? It's
> >>>>>> only the
> >>>>>> > signal handlers which call GracefulShutdownHandler() which need to
> >>>>>> be set up
> >>>>>> > after the pipe.
> >>>>>>
> >>>>>
> >>>>>  You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
>
http://codereview.chromium.**org/7464013/%3Chttp://codereview.chromium.org/74...>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >

[oshima, 2011-07-25 00:39:46]

Friendly ping.

On 2011/07/22 23:04:11, oshima wrote:
> Run suite_Smoke and passed.
> 
> Here is what i did.
> 
> * install latest test image
> * gmerge chromeos-chrome with local_source +  my change
> * cros_workon chromeos-login
> * apply Chri's patch using the git command on gerrit
> * gmerge chromeos-login
> * run_remote_test.sh ... suite_Smoke
> 
> Chris, let me know if i missed something.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> On 2011/07/22 17:20:54, oshima wrote:
> > Actually, http://codereview.chromium.org/7466030 might have fixed that
> > CHECK.
> > But again, it may have another CHECK error. I'll try it later.
> > 
> > - oshima
> > 
> > On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <mailto:cmasone@chromium.org>
> wrote:
> > 
> > > Sorry, man :-(
> > >
> > > On a ToT build I get this CHECK():
> > >
> > > [22574:22574:172453820921:FATAL:content_settings_utils.cc(74)] Check
> > > failed: pattern_pair.second.IsValid().
> > > Unable to get symbols for backtrace. Dumping raw addresses in trace:
> > >
> > >
> > > On Fri, Jul 22, 2011 at 10:12 AM, oshima <mailto:oshima@chromium.org>
wrote:
> > >
> > >> Oh, sorry about that. I guess i'm stressed out now :(
> > >>
> > >> - oshima
> > >>
> > >>
> > >> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone
<cmasone@chromium.org>wrote:
> > >>
> > >>> I already did...[21181:21200:167439799220:FATAL:login_utils.cc(100)]
> > >>> Check failed: getter.
> > >>>
> > >>>
> > >>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <mailto:oshima@chromium.org>
> wrote:
> > >>>
> > >>>>
> > >>>>
> > >>>> On Fri, Jul 22, 2011 at 9:53 AM, <mailto:cmasone@chromium.org> wrote:
> > >>>>
> > >>>>> I got this CHECK() failure when I patched this into a build from
> > >>>>> yesterday:
> > >>>>>
> > >>>>
> > >>>> Can you send me CHECK message? i'm looking at blocker right now and
> > >>>> don't have
> > >>>> bandwidth to repro it myself.
> > >>>>
> > >>>>
> > >>>>
> > >>>>>
> > >>>>> [21181:21200:167439799220:**FATAL:login_utils.cc(100)] Check failed:
> > >>>>> getter.
> > >>>>>
> > >>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git
> > >>>>> patch up here
> > >>>>>
> >
>
http://gerrit.chromium.org/**gerrit/#change%252C4579%253Chttp://gerrit.chromi...
> > you want to use that for
> > >>>>> testing.
> > >>>>>
> > >>>>> On 2011/07/22 01:36:30, oshima wrote:
> > >>>>>
> > >>>>>> We can catch signal early, but there is little we can do there other
> > >>>>>> than
> > >>>>>>
> > >>>>> simply
> > >>>>>
> > >>>>>> exit because message loop doesn't exit yet.
> > >>>>>>  Since this is extremely rare case that happens on test,
> > >>>>>> I prefer simpler solution.
> > >>>>>>
> > >>>>>
> > >>>>>  Chris, I found why chrome sometimes does not exit in 3s.
> > >>>>>> There is a race where processe's module ref count does not
> > >>>>>> become zero when the login process is created after closeallbrowsers
> > >>>>>> get
> > >>>>>>
> > >>>>> called.
> > >>>>>
> > >>>>>> See changes in login code.
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> > >>>>>
> >
>
browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
> > >>>>>
> > >>>>>> File chrome/browser/browser_main_**posix.cc (right):
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> > >>>>>
> >
>
browser/browser_main_posix.cc#**newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
> > >>>>>
> > >>>>>> chrome/browser/browser_main_**posix.cc:30: // See comment in
> > >>>>>> |PreEarlyInitialization()|, where sigaction is called.
> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
> > >>>>>> > Update this comment.
> > >>>>>>
> > >>>>>
> > >>>>>  SIGCHLDHandler was moved back to PreEarlyInitialization
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> > >>>>>
> >
>
browser/browser_main_posix.cc#**newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
> > >>>>>
> > >>>>>> chrome/browser/browser_main_**posix.cc:58: // See comment in
> > >>>>>> |PreEarlyInitialization()|, where sigaction is called.
> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
> > >>>>>> > Ditto, etc.
> > >>>>>>
> > >>>>>
> > >>>>>  Done.
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>> http://codereview.chromium.**org/7464013/diff/1/chrome/**
> > >>>>>
> >
>
browser/browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
> > >>>>>
> > >>>>>> chrome/browser/browser_main_**posix.cc:238: // We need to accept
> > >>>>>> SIGCHLD, even
> > >>>>>> though our handler is a no-op because
> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
> > >>>>>> > We can presumably set up the SIGCHLD handler earlier, right? It's
> > >>>>>> only the
> > >>>>>> > signal handlers which call GracefulShutdownHandler() which need to
> > >>>>>> be set up
> > >>>>>> > after the pipe.
> > >>>>>>
> > >>>>>
> > >>>>>  You're right. I moved SIGCHLDHandler back to PreEarlyInitialization.
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> >
>
http://codereview.chromium.**org/7464013/%253Chttp://codereview.chromium.org/...>
> > >>>>>
> > >>>>
> > >>>>
> > >>>
> > >>
> > >

[Chris Masone, 2011-07-25 03:20:19]

Ping to me?  It's the weekend, I've been away :-)

I just synced chrome os and chrome, built both with your patch and mine, and
then ran suite_Smoke in a VM:

Crashes detected during testing:
----------------------------------------------------
chrome sig 6
  suite_Smoke/login_BadAuthentication
  suite_Smoke/login_CryptohomeIncognitoUnmounted
  suite_Smoke/login_CryptohomeMounted
  suite_Smoke/desktopui_ChromeFirstRender
  suite_Smoke/login_LoginSuccess.apps
  suite_Smoke/login_CryptohomeUnmounted
supplied_chrome sig 6
  suite_Smoke/login_LoginSuccess.default
----------------------------------------------------


Are you running on a real machine?  Perhaps that avoids some race condition
that we can repro in a VM.  Whatever it is, this means that all the bots
will go down in a blaze of fail if we land these changes :-)


On Sun, Jul 24, 2011 at 5:39 PM, <oshima@chromium.org> wrote:

> Friendly ping.
>
>
> On 2011/07/22 23:04:11, oshima wrote:
>
>> Run suite_Smoke and passed.
>>
>
>  Here is what i did.
>>
>
>  * install latest test image
>> * gmerge chromeos-chrome with local_source +  my change
>> * cros_workon chromeos-login
>> * apply Chri's patch using the git command on gerrit
>> * gmerge chromeos-login
>> * run_remote_test.sh ... suite_Smoke
>>
>
>  Chris, let me know if i missed something.
>>
>
>
>
>
>
>
>
>
>
>  On 2011/07/22 17:20:54, oshima wrote:
>> > Actually,
http://codereview.chromium.**org/7466030<http://codereview.chromium.org/74660...
have fixed that
>> > CHECK.
>> > But again, it may have another CHECK error. I'll try it later.
>> >
>> > - oshima
>> >
>> > On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <mailto:
>> cmasone@chromium.org>
>> wrote:
>> >
>> > > Sorry, man :-(
>> > >
>> > > On a ToT build I get this CHECK():
>> > >
>> > > [22574:22574:172453820921:**FATAL:content_settings_utils.**cc(74)]
>> Check
>> > > failed: pattern_pair.second.IsValid().
>> > > Unable to get symbols for backtrace. Dumping raw addresses in trace:
>> > >
>> > >
>> > > On Fri, Jul 22, 2011 at 10:12 AM, oshima <mailto:oshima@chromium.org>
>>
> wrote:
>
>> > >
>> > >> Oh, sorry about that. I guess i'm stressed out now :(
>> > >>
>> > >> - oshima
>> > >>
>> > >>
>> > >> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone
>>
> <cmasone@chromium.org>wrote:
>
>> > >>
>> > >>> I already did...[21181:21200:**167439799220:FATAL:login_**
>> utils.cc(100)]
>> > >>> Check failed: getter.
>> > >>>
>> > >>>
>> > >>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <mailto:oshima@chromium.org
>> >
>> wrote:
>> > >>>
>> > >>>>
>> > >>>>
>> > >>>> On Fri, Jul 22, 2011 at 9:53 AM, <mailto:cmasone@chromium.org>
>> wrote:
>> > >>>>
>> > >>>>> I got this CHECK() failure when I patched this into a build from
>> > >>>>> yesterday:
>> > >>>>>
>> > >>>>
>> > >>>> Can you send me CHECK message? i'm looking at blocker right now and
>> > >>>> don't have
>> > >>>> bandwidth to repro it myself.
>> > >>>>
>> > >>>>
>> > >>>>
>> > >>>>>
>> > >>>>> [21181:21200:167439799220:****FATAL:login_utils.cc(100)] Check
>> failed:
>> > >>>>> getter.
>> > >>>>>
>> > >>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git
>> > >>>>> patch up here
>> > >>>>>
>> >
>>
>
> http://gerrit.chromium.org/****gerrit/#change%252C4579%**
>
253Chttp://gerrit.chromium.**org/gerrit/#change%252C4579%**253Eif<http://gerrit.chromium.org/**gerrit/#change%252C4579%253Chttp://gerrit.chromium.org/gerrit/%23change%252C4579%253Eif>
>
>  > you want to use that for
>> > >>>>> testing.
>> > >>>>>
>> > >>>>> On 2011/07/22 01:36:30, oshima wrote:
>> > >>>>>
>> > >>>>>> We can catch signal early, but there is little we can do there
>> other
>> > >>>>>> than
>> > >>>>>>
>> > >>>>> simply
>> > >>>>>
>> > >>>>>> exit because message loop doesn't exit yet.
>> > >>>>>>  Since this is extremely rare case that happens on test,
>> > >>>>>> I prefer simpler solution.
>> > >>>>>>
>> > >>>>>
>> > >>>>>  Chris, I found why chrome sometimes does not exit in 3s.
>> > >>>>>> There is a race where processe's module ref count does not
>> > >>>>>> become zero when the login process is created after
>> closeallbrowsers
>> > >>>>>> get
>> > >>>>>>
>> > >>>>> called.
>> > >>>>>
>> > >>>>>> See changes in login code.
>> > >>>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>> > >>>>>
>> >
>>
>
> browser/browser_main_posix.cc<**http://codereview.chromium.**
>
org/7464013/diff/1/chrome/**browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
> >
>
>> > >>>>>
>> > >>>>>> File chrome/browser/browser_main_****posix.cc (right):
>> > >>>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>> > >>>>>
>> >
>>
>
> browser/browser_main_posix.cc#****newcode30<http://codereview.**
> chromium.org/7464013/diff/1/**chrome/browser/browser_main_**
>
posix.cc#newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
> >
>
>> > >>>>>
>> > >>>>>> chrome/browser/browser_main_****posix.cc:30: // See comment in
>> > >>>>>> |PreEarlyInitialization()|, where sigaction is called.
>> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>> > >>>>>> > Update this comment.
>> > >>>>>>
>> > >>>>>
>> > >>>>>  SIGCHLDHandler was moved back to PreEarlyInitialization
>> > >>>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>> > >>>>>
>> >
>>
>
> browser/browser_main_posix.cc#****newcode58<http://codereview.**
> chromium.org/7464013/diff/1/**chrome/browser/browser_main_**
>
posix.cc#newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
> >
>
>> > >>>>>
>> > >>>>>> chrome/browser/browser_main_****posix.cc:58: // See comment in
>> > >>>>>> |PreEarlyInitialization()|, where sigaction is called.
>> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>> > >>>>>> > Ditto, etc.
>> > >>>>>>
>> > >>>>>
>> > >>>>>  Done.
>> > >>>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>> > >>>>>
>> >
>>
>
> browser/browser_main_posix.cc#****newcode238<http://**
> codereview.chromium.org/**7464013/diff/1/chrome/browser/**
>
browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
> >
>
>> > >>>>>
>> > >>>>>> chrome/browser/browser_main_****posix.cc:238: // We need to
>> accept
>> > >>>>>> SIGCHLD, even
>> > >>>>>> though our handler is a no-op because
>> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>> > >>>>>> > We can presumably set up the SIGCHLD handler earlier, right?
>> It's
>> > >>>>>> only the
>> > >>>>>> > signal handlers which call GracefulShutdownHandler() which need
>> to
>> > >>>>>> be set up
>> > >>>>>> > after the pipe.
>> > >>>>>>
>> > >>>>>
>> > >>>>>  You're right. I moved SIGCHLDHandler back to
>> PreEarlyInitialization.
>> > >>>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>>
>> >
>>
>
> http://codereview.chromium.****org/7464013/%253Chttp://codere**
> view.chromium.org/7464013/ <http://codereview.chromium.org/7464013/>>
>
>  > >>>>>
>> > >>>>
>> > >>>>
>> > >>>
>> > >>
>> > >
>>
>
>
>
>
http://codereview.chromium.**org/7464013/<http://codereview.chromium.org/7464...
>

[oshima, 2011-07-25 05:57:48]

On Mon, Jul 25, 2011 at 12:19 PM, Chris Masone <cmasone@chromium.org> wrote:

> Ping to me?  It's the weekend, I've been away :-)


I'm in Tokyo, and it's Monday 2pm :)

No, I didn't mean asking you to look at now. I was hoping that
both of you can look at Monday.


> I just synced chrome os and chrome, built both with your patch and mine,
> and then ran suite_Smoke in a VM:
>
> Crashes detected during testing:
> ----------------------------------------------------
> chrome sig 6
>   suite_Smoke/login_BadAuthentication
>   suite_Smoke/login_CryptohomeIncognitoUnmounted
>   suite_Smoke/login_CryptohomeMounted
>   suite_Smoke/desktopui_ChromeFirstRender
>   suite_Smoke/login_LoginSuccess.apps
>   suite_Smoke/login_CryptohomeUnmounted
> supplied_chrome sig 6
>   suite_Smoke/login_LoginSuccess.default
> ----------------------------------------------------
>
>
> Are you running on a real machine?
>

Yes


>   Perhaps that avoids some race condition that we can repro in a VM.
>  Whatever it is, this means that all the bots will go down in a blaze of
> fail if we land these changes :-)
>

It could be that shutdown process is not fast enough on VM to finish in 3
seconds.
Let me try with longer timeout.

- oshima

>
>
> On Sun, Jul 24, 2011 at 5:39 PM, <oshima@chromium.org> wrote:
>
>> Friendly ping.
>>
>>
>> On 2011/07/22 23:04:11, oshima wrote:
>>
>>> Run suite_Smoke and passed.
>>>
>>
>>  Here is what i did.
>>>
>>
>>  * install latest test image
>>> * gmerge chromeos-chrome with local_source +  my change
>>> * cros_workon chromeos-login
>>> * apply Chri's patch using the git command on gerrit
>>> * gmerge chromeos-login
>>> * run_remote_test.sh ... suite_Smoke
>>>
>>
>>  Chris, let me know if i missed something.
>>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>  On 2011/07/22 17:20:54, oshima wrote:
>>> > Actually,
http://codereview.chromium.**org/7466030<http://codereview.chromium.org/74660...
have fixed that
>>> > CHECK.
>>> > But again, it may have another CHECK error. I'll try it later.
>>> >
>>> > - oshima
>>> >
>>> > On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <mailto:
>>> cmasone@chromium.org>
>>>
>>> wrote:
>>> >
>>> > > Sorry, man :-(
>>> > >
>>> > > On a ToT build I get this CHECK():
>>> > >
>>> > > [22574:22574:172453820921:**FATAL:content_settings_utils.**cc(74)]
>>> Check
>>> > > failed: pattern_pair.second.IsValid().
>>> > > Unable to get symbols for backtrace. Dumping raw addresses in trace:
>>> > >
>>> > >
>>> > > On Fri, Jul 22, 2011 at 10:12 AM, oshima <mailto:oshima@chromium.org
>>> >
>>>
>> wrote:
>>
>>> > >
>>> > >> Oh, sorry about that. I guess i'm stressed out now :(
>>> > >>
>>> > >> - oshima
>>> > >>
>>> > >>
>>> > >> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone
>>>
>> <cmasone@chromium.org>wrote:
>>
>>> > >>
>>> > >>> I already did...[21181:21200:**167439799220:FATAL:login_**
>>> utils.cc(100)]
>>> > >>> Check failed: getter.
>>> > >>>
>>> > >>>
>>> > >>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <mailto:
>>> oshima@chromium.org>
>>> wrote:
>>>
>>> > >>>
>>> > >>>>
>>> > >>>>
>>> > >>>> On Fri, Jul 22, 2011 at 9:53 AM, <mailto:cmasone@chromium.org>
>>> wrote:
>>> > >>>>
>>> > >>>>> I got this CHECK() failure when I patched this into a build from
>>> > >>>>> yesterday:
>>> > >>>>>
>>> > >>>>
>>> > >>>> Can you send me CHECK message? i'm looking at blocker right now
>>> and
>>> > >>>> don't have
>>> > >>>> bandwidth to repro it myself.
>>> > >>>>
>>> > >>>>
>>> > >>>>
>>> > >>>>>
>>> > >>>>> [21181:21200:167439799220:****FATAL:login_utils.cc(100)] Check
>>> failed:
>>> > >>>>> getter.
>>> > >>>>>
>>> > >>>>> Testing again with a ToT build.  Oshima, I put my
>>> login_manger.git
>>> > >>>>> patch up here
>>> > >>>>>
>>> >
>>>
>>
>> http://gerrit.chromium.org/****gerrit/#change%252C4579%**
>>
253Chttp://gerrit.chromium.**org/gerrit/#change%252C4579%**253Eif<http://gerrit.chromium.org/**gerrit/#change%252C4579%253Chttp://gerrit.chromium.org/gerrit/%23change%252C4579%253Eif>
>>
>>  > you want to use that for
>>> > >>>>> testing.
>>> > >>>>>
>>> > >>>>> On 2011/07/22 01:36:30, oshima wrote:
>>> > >>>>>
>>> > >>>>>> We can catch signal early, but there is little we can do there
>>> other
>>> > >>>>>> than
>>> > >>>>>>
>>> > >>>>> simply
>>> > >>>>>
>>> > >>>>>> exit because message loop doesn't exit yet.
>>> > >>>>>>  Since this is extremely rare case that happens on test,
>>> > >>>>>> I prefer simpler solution.
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>  Chris, I found why chrome sometimes does not exit in 3s.
>>> > >>>>>> There is a race where processe's module ref count does not
>>> > >>>>>> become zero when the login process is created after
>>> closeallbrowsers
>>> > >>>>>> get
>>> > >>>>>>
>>> > >>>>> called.
>>> > >>>>>
>>> > >>>>>> See changes in login code.
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>>> > >>>>>
>>> >
>>>
>>
>> browser/browser_main_posix.cc<**http://codereview.chromium.**
>>
org/7464013/diff/1/chrome/**browser/browser_main_posix.cc<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc>
>> >
>>
>>  > >>>>>
>>> > >>>>>> File chrome/browser/browser_main_****posix.cc (right):
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>>> > >>>>>
>>> >
>>>
>>
>> browser/browser_main_posix.cc#****newcode30<http://codereview.**
>> chromium.org/7464013/diff/1/**chrome/browser/browser_main_**
>>
posix.cc#newcode30<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode30>
>> >
>>
>>  > >>>>>
>>> > >>>>>> chrome/browser/browser_main_****posix.cc:30: // See comment in
>>> > >>>>>> |PreEarlyInitialization()|, where sigaction is called.
>>> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>> > >>>>>> > Update this comment.
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>  SIGCHLDHandler was moved back to PreEarlyInitialization
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>>> > >>>>>
>>> >
>>>
>>
>> browser/browser_main_posix.cc#****newcode58<http://codereview.**
>> chromium.org/7464013/diff/1/**chrome/browser/browser_main_**
>>
posix.cc#newcode58<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode58>
>> >
>>
>>  > >>>>>
>>> > >>>>>> chrome/browser/browser_main_****posix.cc:58: // See comment in
>>> > >>>>>> |PreEarlyInitialization()|, where sigaction is called.
>>> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>> > >>>>>> > Ditto, etc.
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>  Done.
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>> http://codereview.chromium.****org/7464013/diff/1/chrome/**
>>> > >>>>>
>>> >
>>>
>>
>> browser/browser_main_posix.cc#****newcode238<http://**
>> codereview.chromium.org/**7464013/diff/1/chrome/browser/**
>>
browser_main_posix.cc#**newcode238<http://codereview.chromium.org/7464013/diff/1/chrome/browser/browser_main_posix.cc#newcode238>
>> >
>>
>>  > >>>>>
>>> > >>>>>> chrome/browser/browser_main_****posix.cc:238: // We need to
>>> accept
>>> > >>>>>> SIGCHLD, even
>>> > >>>>>> though our handler is a no-op because
>>> > >>>>>> On 2011/07/21 18:00:20, viettrungluu wrote:
>>> > >>>>>> > We can presumably set up the SIGCHLD handler earlier, right?
>>> It's
>>> > >>>>>> only the
>>> > >>>>>> > signal handlers which call GracefulShutdownHandler() which
>>> need to
>>> > >>>>>> be set up
>>> > >>>>>> > after the pipe.
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>  You're right. I moved SIGCHLDHandler back to
>>> PreEarlyInitialization.
>>> > >>>>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>>
>>> >
>>>
>>
>> http://codereview.chromium.****org/7464013/%253Chttp://codere**
>> view.chromium.org/7464013/ <http://codereview.chromium.org/7464013/>>
>>
>>  > >>>>>
>>> > >>>>
>>> > >>>>
>>> > >>>
>>> > >>
>>> > >
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/7464013/<http://codereview.chromium.org/7464...
>>
>
>

[Chris Masone, 2011-07-25 14:43:02]

On Sunday, July 24, 2011, oshima wrote:

>
> On Mon, Jul 25, 2011 at 12:19 PM, Chris Masone
<cmasone@chromium.org<javascript:_e({}, 'cvml', 'cmasone@chromium.org');>
> > wrote:
>
>> Ping to me?  It's the weekend, I've been away :-)
>
>
> I'm in Tokyo, and it's Monday 2pm :)
>
>
You are quite the world traveler these days


> No, I didn't mean asking you to look at now. I was hoping that
> both of you can look at Monday.
>
>
>> I just synced chrome os and chrome, built both with your patch and mine,
>> and then ran suite_Smoke in a VM:
>>
>> Crashes detected during testing:
>> ----------------------------------------------------
>> chrome sig 6
>>   suite_Smoke/login_BadAuthentication
>>   suite_Smoke/login_CryptohomeIncognitoUnmounted
>>   suite_Smoke/login_CryptohomeMounted
>>   suite_Smoke/desktopui_ChromeFirstRender
>>   suite_Smoke/login_LoginSuccess.apps
>>   suite_Smoke/login_CryptohomeUnmounted
>> supplied_chrome sig 6
>>   suite_Smoke/login_LoginSuccess.default
>> ----------------------------------------------------
>>
>>
>> Are you running on a real machine?
>>
>
> Yes
>
>
>>   Perhaps that avoids some race condition that we can repro in a VM.
>>  Whatever it is, this means that all the bots will go down in a blaze of
>> fail if we land these changes :-)
>>
>
> It could be that shutdown process is not fast enough on VM to finish in 3
> seconds.
> Let me try with longer timeout.
>

It could be.  I've actually noticed that a lot of processing tasks are
_faster_ in a VM on a Z600, though the lack of hardware acceleration makes
the UI much slower, and I think disk I/O is also slower.  Either way...there
are nontrivially different performance characteristics that seem to be
exposing some races here.


>
> - oshima
>
>
>
> On Sun, Jul 24, 2011 at 5:39 PM, <oshima@chromium.org> wrote:
>
> Friendly ping.
>
>
> On 2011/07/22 23:04:11, oshima wrote:
>
> Run suite_Smoke and passed.
>
>
>  Here is what i did.
>
>
>  * install latest test image
> * gmerge chromeos-chrome with local_source +  my change
> * cros_workon chromeos-login
> * apply Chri's patch using the git command on gerrit
> * gmerge chromeos-login
> * run_remote_test.sh ... suite_Smoke
>
>
>  Chris, let me know if i missed something.
>
>
>
>
>
>
>
>
>
>
>  On 2011/07/22 17:20:54, oshima wrote:
> > Actually,
http://codereview.chromium.**org/7466030<http://codereview.chromium.org/74660...
have fixed that
> > CHECK.
> > But again, it may have another CHECK error. I'll try it later.
> >
> > - oshima
> >
> > On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <mailto:
> cmasone@chromium.org>
>
> wrote:
> >
> > > Sorry, man :-(
> > >
> > > On a ToT build I get this CHECK():
> > >
> > > [22574:22574:172453820921:**FATAL:content_settings_utils.**cc(74)]
> Check
> > > failed: pattern_pair.second.IsValid().
> > > Unable to get symbols for backtrace. Dumping raw addresses in trace:
> > >
> > >
> > > On Fri, Jul 22, 2011 at 10:12 AM, oshima <mailto:oshima@chromium.org>
>
> wrote:
>
> > >
> > >> Oh, sorry about that. I guess i'm stressed out now :(
> > >>
> > >> - oshima
> > >>
> > >>
> > >> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone
>
> <cmasone@chromium.org>wrote:
>
> > >>
> > >>> I already did...[21181:21200:**167439799220:FATAL:login_**
> utils.cc(100)]
> > >>> Check failed: getter.
> > >>>
> > >>>
> > >>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <mailto:oshima@chromium.org>
> wrote:
>
> > >>>
> > >>>>
> > >>>>
> > >>>> On Fri, Jul 22, 2011 at 9:53 AM, <mailto:cmasone@chromium.org>
> wrote:
> > >>>>
> > >>>>> I got this CHECK() failure when I patched this into a build from
> > >>>>> yesterday:
> > >>>>>
> > >>>>
> > >>>> Can you send me CHECK message? i'm looking at blocker right now and
> > >>>> don't have
> > >>>> bandwidth to repro it myself.
> > >>>>
> > >>>>
> > >>>>
> > >>>>>
> > >>>>> [21181:21200:167439799220:****FATAL:login_utils.cc(100)] Check
> failed:
> > >>>>> getter.
> > >>>>>
> > >>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git
> > >>>>> patch up here
> > >>>>>
> >
>
>
>
http://gerrit.chromium.org/****gerrit/#change%252C4579%**2<http://gerrit.chro...
>
>
>

[oshima, 2011-07-27 01:13:00]

I did test on vm and it indeed had crash, although it was sig11.
I've got the same sig6 as yours without my change, so could be that
you didn't have my change.

In any case, there seems to be other issues that I have to fix. I wonder if
it'll be ever possible to handle this case (shutdown immediately) correctly
because the code was written without this case in mind :(

- oshima

On Mon, Jul 25, 2011 at 11:42 PM, Chris Masone <cmasone@chromium.org> wrote:

>
>
> On Sunday, July 24, 2011, oshima wrote:
>
>>
>> On Mon, Jul 25, 2011 at 12:19 PM, Chris Masone <cmasone@chromium.org>wrote:
>>
>>> Ping to me?  It's the weekend, I've been away :-)
>>
>>
>> I'm in Tokyo, and it's Monday 2pm :)
>>
>>
> You are quite the world traveler these days
>
>
>> No, I didn't mean asking you to look at now. I was hoping that
>> both of you can look at Monday.
>>
>>
>>> I just synced chrome os and chrome, built both with your patch and mine,
>>> and then ran suite_Smoke in a VM:
>>>
>>> Crashes detected during testing:
>>> ----------------------------------------------------
>>> chrome sig 6
>>>   suite_Smoke/login_BadAuthentication
>>>   suite_Smoke/login_CryptohomeIncognitoUnmounted
>>>   suite_Smoke/login_CryptohomeMounted
>>>   suite_Smoke/desktopui_ChromeFirstRender
>>>   suite_Smoke/login_LoginSuccess.apps
>>>   suite_Smoke/login_CryptohomeUnmounted
>>> supplied_chrome sig 6
>>>   suite_Smoke/login_LoginSuccess.default
>>> ----------------------------------------------------
>>>
>>>
>>> Are you running on a real machine?
>>>
>>
>> Yes
>>
>>
>>>   Perhaps that avoids some race condition that we can repro in a VM.
>>>  Whatever it is, this means that all the bots will go down in a blaze of
>>> fail if we land these changes :-)
>>>
>>
>> It could be that shutdown process is not fast enough on VM to finish in 3
>> seconds.
>> Let me try with longer timeout.
>>
>
> It could be.  I've actually noticed that a lot of processing tasks are
> _faster_ in a VM on a Z600, though the lack of hardware acceleration makes
> the UI much slower, and I think disk I/O is also slower.  Either way...there
> are nontrivially different performance characteristics that seem to be
> exposing some races here.
>
>
>>
>> - oshima
>>
>>
>>
>> On Sun, Jul 24, 2011 at 5:39 PM, <oshima@chromium.org> wrote:
>>
>> Friendly ping.
>>
>>
>> On 2011/07/22 23:04:11, oshima wrote:
>>
>> Run suite_Smoke and passed.
>>
>>
>>  Here is what i did.
>>
>>
>>  * install latest test image
>> * gmerge chromeos-chrome with local_source +  my change
>> * cros_workon chromeos-login
>> * apply Chri's patch using the git command on gerrit
>> * gmerge chromeos-login
>> * run_remote_test.sh ... suite_Smoke
>>
>>
>>  Chris, let me know if i missed something.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>  On 2011/07/22 17:20:54, oshima wrote:
>> > Actually,
http://codereview.chromium.**org/7466030<http://codereview.chromium.org/74660...
have fixed that
>> > CHECK.
>> > But again, it may have another CHECK error. I'll try it later.
>> >
>> > - oshima
>> >
>> > On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <mailto:
>> cmasone@chromium.org>
>>
>> wrote:
>> >
>> > > Sorry, man :-(
>> > >
>> > > On a ToT build I get this CHECK():
>> > >
>> > > [22574:22574:172453820921:**FATAL:content_settings_utils.**cc(74)]
>> Check
>> > > failed: pattern_pair.second.IsValid().
>> > > Unable to get symbols for backtrace. Dumping raw addresses in trace:
>> > >
>> > >
>> > > On Fri, Jul 22, 2011 at 10:12 AM, oshima <mailto:oshima@chromium.org>
>>
>> wrote:
>>
>> > >
>> > >> Oh, sorry about that. I guess i'm stressed out now :(
>> > >>
>> > >> - oshima
>> > >>
>> > >>
>> > >> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone
>>
>> <cmasone@chromium.org>wrote:
>>
>> > >>
>> > >>> I already did...[21181:21200:**167439799220:FATAL:login_**
>> utils.cc(100)]
>> > >>> Check failed: getter.
>> > >>>
>> > >>>
>> > >>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <mailto:oshima@chromium.org
>> >
>> wrote:
>>
>> > >>>
>> > >>>>
>> > >>>>
>> > >>>> On Fri, Jul 22, 2011 at 9:53 AM, <mailto:cmasone@chromium.org>
>> wrote:
>> > >>>>
>> > >>>>> I got this CHECK() failure when I patched this into a build from
>> > >>>>> yesterday:
>> > >>>>>
>> > >>>>
>> > >>>> Can you send me CHECK message? i'm looking at blocker right now and
>> > >>>> don't have
>> > >>>> bandwidth to repro it myself.
>> > >>>>
>> > >>>>
>> > >>>>
>> > >>>>>
>> > >>>>> [21181:21200:167439799220:****FATAL:login_utils.cc(100)] Check
>> failed:
>> > >>>>> getter.
>> > >>>>>
>> > >>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git
>> > >>>>> patch up here
>> > >>>>>
>> >
>>
>>
>>
http://gerrit.chromium.org/****gerrit/#change%252C4579%**2<http://gerrit.chro...
>>
>>
>>

[oshima, 2011-07-27 07:28:11]

I changed the code to use static instead of singleton and test no longer
crashes.

ChromeFirstRender fails due to timeout but I'm not sure if this is because the
way I changed the code, or there is another issue in this CL.
I also see sporadic logging_UserCrash failure. Other test seems to be passing
w/o any
flakyness.

I filed a bug (crosbug.com/18269) and will hold this until it is resolved.

On 2011/07/27 01:13:00, oshima wrote:
> I did test on vm and it indeed had crash, although it was sig11.
> I've got the same sig6 as yours without my change, so could be that
> you didn't have my change.
> 
> In any case, there seems to be other issues that I have to fix. I wonder if
> it'll be ever possible to handle this case (shutdown immediately) correctly
> because the code was written without this case in mind :(
> 
> - oshima
> 
> On Mon, Jul 25, 2011 at 11:42 PM, Chris Masone <mailto:cmasone@chromium.org>
wrote:
> 
> >
> >
> > On Sunday, July 24, 2011, oshima wrote:
> >
> >>
> >> On Mon, Jul 25, 2011 at 12:19 PM, Chris Masone <cmasone@chromium.org>wrote:
> >>
> >>> Ping to me?  It's the weekend, I've been away :-)
> >>
> >>
> >> I'm in Tokyo, and it's Monday 2pm :)
> >>
> >>
> > You are quite the world traveler these days
> >
> >
> >> No, I didn't mean asking you to look at now. I was hoping that
> >> both of you can look at Monday.
> >>
> >>
> >>> I just synced chrome os and chrome, built both with your patch and mine,
> >>> and then ran suite_Smoke in a VM:
> >>>
> >>> Crashes detected during testing:
> >>> ----------------------------------------------------
> >>> chrome sig 6
> >>>   suite_Smoke/login_BadAuthentication
> >>>   suite_Smoke/login_CryptohomeIncognitoUnmounted
> >>>   suite_Smoke/login_CryptohomeMounted
> >>>   suite_Smoke/desktopui_ChromeFirstRender
> >>>   suite_Smoke/login_LoginSuccess.apps
> >>>   suite_Smoke/login_CryptohomeUnmounted
> >>> supplied_chrome sig 6
> >>>   suite_Smoke/login_LoginSuccess.default
> >>> ----------------------------------------------------
> >>>
> >>>
> >>> Are you running on a real machine?
> >>>
> >>
> >> Yes
> >>
> >>
> >>>   Perhaps that avoids some race condition that we can repro in a VM.
> >>>  Whatever it is, this means that all the bots will go down in a blaze of
> >>> fail if we land these changes :-)
> >>>
> >>
> >> It could be that shutdown process is not fast enough on VM to finish in 3
> >> seconds.
> >> Let me try with longer timeout.
> >>
> >
> > It could be.  I've actually noticed that a lot of processing tasks are
> > _faster_ in a VM on a Z600, though the lack of hardware acceleration makes
> > the UI much slower, and I think disk I/O is also slower.  Either way...there
> > are nontrivially different performance characteristics that seem to be
> > exposing some races here.
> >
> >
> >>
> >> - oshima
> >>
> >>
> >>
> >> On Sun, Jul 24, 2011 at 5:39 PM, <mailto:oshima@chromium.org> wrote:
> >>
> >> Friendly ping.
> >>
> >>
> >> On 2011/07/22 23:04:11, oshima wrote:
> >>
> >> Run suite_Smoke and passed.
> >>
> >>
> >>  Here is what i did.
> >>
> >>
> >>  * install latest test image
> >> * gmerge chromeos-chrome with local_source +  my change
> >> * cros_workon chromeos-login
> >> * apply Chri's patch using the git command on gerrit
> >> * gmerge chromeos-login
> >> * run_remote_test.sh ... suite_Smoke
> >>
> >>
> >>  Chris, let me know if i missed something.
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>  On 2011/07/22 17:20:54, oshima wrote:
> >> > Actually,
>
http://codereview.chromium.**org/7466030%3Chttp://codereview.chromium.org/746...
> have fixed that
> >> > CHECK.
> >> > But again, it may have another CHECK error. I'll try it later.
> >> >
> >> > - oshima
> >> >
> >> > On Fri, Jul 22, 2011 at 10:13 AM, Chris Masone <mailto:
> >> mailto:cmasone@chromium.org>
> >>
> >> wrote:
> >> >
> >> > > Sorry, man :-(
> >> > >
> >> > > On a ToT build I get this CHECK():
> >> > >
> >> > > [22574:22574:172453820921:**FATAL:content_settings_utils.**cc(74)]
> >> Check
> >> > > failed: pattern_pair.second.IsValid().
> >> > > Unable to get symbols for backtrace. Dumping raw addresses in trace:
> >> > >
> >> > >
> >> > > On Fri, Jul 22, 2011 at 10:12 AM, oshima <mailto:oshima@chromium.org>
> >>
> >> wrote:
> >>
> >> > >
> >> > >> Oh, sorry about that. I guess i'm stressed out now :(
> >> > >>
> >> > >> - oshima
> >> > >>
> >> > >>
> >> > >> On Fri, Jul 22, 2011 at 10:10 AM, Chris Masone
> >>
> >> <cmasone@chromium.org>wrote:
> >>
> >> > >>
> >> > >>> I already did...[21181:21200:**167439799220:FATAL:login_**
> >> utils.cc(100)]
> >> > >>> Check failed: getter.
> >> > >>>
> >> > >>>
> >> > >>> On Fri, Jul 22, 2011 at 9:58 AM, oshima <mailto:oshima@chromium.org
> >> >
> >> wrote:
> >>
> >> > >>>
> >> > >>>>
> >> > >>>>
> >> > >>>> On Fri, Jul 22, 2011 at 9:53 AM, <mailto:cmasone@chromium.org>
> >> wrote:
> >> > >>>>
> >> > >>>>> I got this CHECK() failure when I patched this into a build from
> >> > >>>>> yesterday:
> >> > >>>>>
> >> > >>>>
> >> > >>>> Can you send me CHECK message? i'm looking at blocker right now and
> >> > >>>> don't have
> >> > >>>> bandwidth to repro it myself.
> >> > >>>>
> >> > >>>>
> >> > >>>>
> >> > >>>>>
> >> > >>>>> [21181:21200:167439799220:****FATAL:login_utils.cc(100)] Check
> >> failed:
> >> > >>>>> getter.
> >> > >>>>>
> >> > >>>>> Testing again with a ToT build.  Oshima, I put my login_manger.git
> >> > >>>>> patch up here
> >> > >>>>>
> >> >
> >>
> >>
> >>
>
http://gerrit.chromium.org/****gerrit/#change%25252C4579%25**2%3Chttp://gerri...>
> >>
> >>
> >>

[oshima, 2011-09-07 16:25:57]

crosbug.com/18269 has been resolved and this is ready.
viettrungluu, could you please review this again?

[viettrungluu, 2011-09-08 16:03:53]

LGTM on the change to browser_main_posix.cc; I have no idea about the change to
the other file.

[oshima, 2011-09-08 17:03:51]

+nikita as cmasone is on vacation.

Nikita, this fixes a crash that happens when
chrome tries to show login screen while it's shutting down.

[Nikita (slow), 2011-09-12 12:29:44]

LGTM chromeos/login