Avoid gpu watchdog crash on timeout if X is un-responsive.

On timeout, we wait for a XProperty change event with additional
timeout. If it arrives we know that X is responsive and is not
the cause of the watchdog trigger, so we should terminate. If it
times out, it may be due to X taking a long time, but terminating
won't help, so ignore the watchdog trigger.

BUG=432095
Committed: https://crrev.com/cf28abf824523e4d78b683680fdf87de4dc66901
Cr-Commit-Position: refs/heads/master@{#316757}

[sohanjg, 2015-01-02 11:48:30]

sohan.jyoti@samsung.com changed reviewers:
+ danakj@chromium.org

[sohanjg, 2015-01-02 11:51:48]

Can you please take a look, if this is what we want? thanks.

[danakj, 2015-01-06 00:29:19]

https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
content/gpu/gpu_watchdog_thread.cc:285: if (!XDisplayString(gfx::GetXDisplay()))
{
What does this do if the X server is not the active VT?

https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
File content/gpu/gpu_watchdog_thread.h (right):

https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
content/gpu/gpu_watchdog_thread.h:100: bool x_server_active;
member variables have _ suffix

[sohanjg, 2015-01-06 12:21:45]

https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
content/gpu/gpu_watchdog_thread.cc:285: if (!XDisplayString(gfx::GetXDisplay()))
{
On 2015/01/06 00:29:19, danakj wrote:
> What does this do if the X server is not the active VT?

Hmm..i was assuming this would return NULL in that case, but there is no
documentation for it.
But it seems, in OpenNewXDisplay, we send display string as NULL, so this query
wouldnt be of much help i guess.

My idea was to use a less complex X-api do that we do not need to plumb all the
params that XChangeProperty would have required in gpu wtachdog thread.

We may also use, XDefaultScreen, and XDisplayHeight to get display height. And i
assume (not documented though) we will get it as 0, if X is not active VT.
Please suggest what should be best here.

I checked fgconsole man page,
http://man7.org/linux/man-pages/man1/fgconsole.1.html 
It seems if we check --next-available param here, we may understand whether X is
the active VT.
But would that help?

[danakj, 2015-01-06 16:10:05]

On 2015/01/06 12:21:45, sohanjg wrote:
>
https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
> File content/gpu/gpu_watchdog_thread.cc (right):
> 
>
https://codereview.chromium.org/836473003/diff/1/content/gpu/gpu_watchdog_thr...
> content/gpu/gpu_watchdog_thread.cc:285: if
(!XDisplayString(gfx::GetXDisplay()))
> {
> On 2015/01/06 00:29:19, danakj wrote:
> > What does this do if the X server is not the active VT?
> 
> Hmm..i was assuming this would return NULL in that case, but there is no
> documentation for it.
> But it seems, in OpenNewXDisplay, we send display string as NULL, so this
query
> wouldnt be of much help i guess.
> 
> My idea was to use a less complex X-api do that we do not need to plumb all
the
> params that XChangeProperty would have required in gpu wtachdog thread.
> 
> We may also use, XDefaultScreen, and XDisplayHeight to get display height. And
i
> assume (not documented though) we will get it as 0, if X is not active VT.
> Please suggest what should be best here.

If you use a sync call to the Xserver it will just block the thread, won't it?
Maybe that would work? But when you wake up you need to know the time was spent
due to the X server not for some other reason. The XChangeProperty idea lets you
wait for a given amount of time to hear back from the server about the property
being updated without blocking the thread.

> I checked fgconsole man page,
> http://man7.org/linux/man-pages/man1/fgconsole.1.html 
> It seems if we check --next-available param here, we may understand whether X
is
> the active VT.
> But would that help?

Not unless you're assuming the X session Chrome is running on is the last VT
which does not have to be the case.

[danakj, 2015-01-06 16:10:22]

danakj@chromium.org changed reviewers:
+ sievers@chromium.org

[sohanjg, 2015-01-15 13:37:12]

On 2015/01/06 16:10:23, danakj wrote:

If we use XChangeProperty, say we use something like, 
int state = 1;
XChangeProperty(gfx::GetXDisplay(),
                  DefaultRootWindow(gfx::GetXDisplay()),
                  ui::GetAtom("_NET_WM_STATE_FULLSCREEN"),
                  XA_CARDINAL,
                  32,
                  PropModeReplace,
                  reinterpret_cast<unsigned char*>(&state),
                  1);

How should we plan to catch the PropertyNotify event ? some ProcessEvent in gpu
watchdog thread itself? or some other place ?

[danakj, 2015-01-22 17:56:10]

On 2015/01/15 13:37:12, sohanjg wrote:
> On 2015/01/06 16:10:23, danakj wrote:
> 
> If we use XChangeProperty, say we use something like, 
> int state = 1;
> XChangeProperty(gfx::GetXDisplay(),
>                   DefaultRootWindow(gfx::GetXDisplay()),
>                   ui::GetAtom("_NET_WM_STATE_FULLSCREEN"),
>                   XA_CARDINAL,
>                   32,
>                   PropModeReplace,
>                   reinterpret_cast<unsigned char*>(&state),
>                   1);
> 
> How should we plan to catch the PropertyNotify event ? some ProcessEvent in
gpu
> watchdog thread itself? or some other place ?

I guess the main message loop is a UI loop and would receive X events?

[sohanjg, 2015-01-28 14:44:43]

On 2015/01/22 17:56:10, danakj wrote:
> On 2015/01/15 13:37:12, sohanjg wrote:
> > On 2015/01/06 16:10:23, danakj wrote:
> > 
> > If we use XChangeProperty, say we use something like, 
> > int state = 1;
> > XChangeProperty(gfx::GetXDisplay(),
> >                   DefaultRootWindow(gfx::GetXDisplay()),
> >                   ui::GetAtom("_NET_WM_STATE_FULLSCREEN"),
> >                   XA_CARDINAL,
> >                   32,
> >                   PropModeReplace,
> >                   reinterpret_cast<unsigned char*>(&state),
> >                   1);
> > 
> > How should we plan to catch the PropertyNotify event ? some ProcessEvent in
> gpu
> > watchdog thread itself? or some other place ?
> 
> I guess the main message loop is a UI loop and would receive X events?

Will we able to catch the PropertyNotify event in
NativeViewGLSurfaceGLX::DispatchEvent ? Or we should check it in GpuMain ?

[danakj, 2015-01-28 18:02:51]

On Jan 28, 2015 6:44 AM, <sohan.jyoti@samsung.com> wrote:
>
> On 2015/01/22 17:56:10, danakj wrote:
>>
>> On 2015/01/15 13:37:12, sohanjg wrote:
>> > On 2015/01/06 16:10:23, danakj wrote:
>> >
>> > If we use XChangeProperty, say we use something like,
>> > int state = 1;
>> > XChangeProperty(gfx::GetXDisplay(),
>> >                   DefaultRootWindow(gfx::GetXDisplay()),
>> >                   ui::GetAtom("_NET_WM_STATE_FULLSCREEN"),
>> >                   XA_CARDINAL,
>> >                   32,
>> >                   PropModeReplace,
>> >                   reinterpret_cast<unsigned char*>(&state),
>> >                   1);
>> >
>> > How should we plan to catch the PropertyNotify event ? some
ProcessEvent in
>> gpu
>> > watchdog thread itself? or some other place ?
>
>
>> I guess the main message loop is a UI loop and would receive X events?
>
>
> Will we able to catch the PropertyNotify event in
> NativeViewGLSurfaceGLX::DispatchEvent ? Or we should check it in GpuMain ?

I don't actually know. Is this something you can test or read the code to
figure out?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sohanjg, 2015-01-30 12:54:01]

On 2015/01/28 18:02:51, danakj wrote:
> On Jan 28, 2015 6:44 AM, <mailto:sohan.jyoti@samsung.com> wrote:
> >
> > On 2015/01/22 17:56:10, danakj wrote:
> >>
> >> On 2015/01/15 13:37:12, sohanjg wrote:
> >> > On 2015/01/06 16:10:23, danakj wrote:
> >> >
> >> > If we use XChangeProperty, say we use something like,
> >> > int state = 1;
> >> > XChangeProperty(gfx::GetXDisplay(),
> >> >                   DefaultRootWindow(gfx::GetXDisplay()),
> >> >                   ui::GetAtom("_NET_WM_STATE_FULLSCREEN"),
> >> >                   XA_CARDINAL,
> >> >                   32,
> >> >                   PropModeReplace,
> >> >                   reinterpret_cast<unsigned char*>(&state),
> >> >                   1);
> >> >
> >> > How should we plan to catch the PropertyNotify event ? some
> ProcessEvent in
> >> gpu
> >> > watchdog thread itself? or some other place ?
> >
> >
> >> I guess the main message loop is a UI loop and would receive X events?
> >
> >
> > Will we able to catch the PropertyNotify event in
> > NativeViewGLSurfaceGLX::DispatchEvent ? Or we should check it in GpuMain ?
> 
> I don't actually know. Is this something you can test or read the code to
> figure out?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I am trying to send XChangeProp from GpuWatchdogThread::CheckXServerActive, like


    unsigned char text[20] = "check";
    Atom atom = XInternAtom(gfx::GetXDisplay(), "CHK", False);
    int status = XChangeProperty(gfx::GetXDisplay(),   
                             DefaultRootWindow(gfx::GetXDisplay()),    
                             my_atom,   
                             XA_STRING, 
                             8,         
                             PropModeReplace,
                             text, 
                             5         
                            );   
    LOG(ERROR)<<__FUNCTION__<<" XChangeProperty status = "<<status;
    XWindowAttributes attributes;	
    XGetWindowAttributes(gfx::GetXDisplay(),
DefaultRootWindow(gfx::GetXDisplay()), &attributes);
    long old_event_mask_ = attributes.your_event_mask;	
    XSelectInput(gfx::GetXDisplay(), DefaultRootWindow(gfx::GetXDisplay()),
old_event_mask_ | PropertyChangeMask);	
    XFlush(gfx::GetXDisplay());

But, GpuMain is not invoked at all, leave aside handling the PropertyNotify
event. The status for XChangeProperty is 1 though.

[sohanjg, 2015-01-30 12:54:45]

sohan.jyoti@samsung.com changed reviewers:
+ piman@chromium.org

[sohanjg, 2015-01-30 12:58:59]

On 2015/01/30 12:54:01, sohanjg wrote:
> On 2015/01/28 18:02:51, danakj wrote:
> > On Jan 28, 2015 6:44 AM, <mailto:sohan.jyoti@samsung.com> wrote:
> > >
> > > On 2015/01/22 17:56:10, danakj wrote:
> > >>
> > >> On 2015/01/15 13:37:12, sohanjg wrote:
> > >> > On 2015/01/06 16:10:23, danakj wrote:
> > >> >
> > >> > If we use XChangeProperty, say we use something like,
> > >> > int state = 1;
> > >> > XChangeProperty(gfx::GetXDisplay(),
> > >> >                   DefaultRootWindow(gfx::GetXDisplay()),
> > >> >                   ui::GetAtom("_NET_WM_STATE_FULLSCREEN"),
> > >> >                   XA_CARDINAL,
> > >> >                   32,
> > >> >                   PropModeReplace,
> > >> >                   reinterpret_cast<unsigned char*>(&state),
> > >> >                   1);
> > >> >
> > >> > How should we plan to catch the PropertyNotify event ? some
> > ProcessEvent in
> > >> gpu
> > >> > watchdog thread itself? or some other place ?
> > >
> > >
> > >> I guess the main message loop is a UI loop and would receive X events?
> > >
> > >
> > > Will we able to catch the PropertyNotify event in
> > > NativeViewGLSurfaceGLX::DispatchEvent ? Or we should check it in GpuMain ?
> > 
> > I don't actually know. Is this something you can test or read the code to
> > figure out?
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> I am trying to send XChangeProp from GpuWatchdogThread::CheckXServerActive,
like
> 
> 
>     unsigned char text[20] = "check";
>     Atom atom = XInternAtom(gfx::GetXDisplay(), "CHK", False);
>     int status = XChangeProperty(gfx::GetXDisplay(),   
>                              DefaultRootWindow(gfx::GetXDisplay()),    
>                              my_atom,   
>                              XA_STRING, 
>                              8,         
>                              PropModeReplace,
>                              text, 
>                              5         
>                             );   
>     LOG(ERROR)<<__FUNCTION__<<" XChangeProperty status = "<<status;
>     XWindowAttributes attributes;	
>     XGetWindowAttributes(gfx::GetXDisplay(),
> DefaultRootWindow(gfx::GetXDisplay()), &attributes);
>     long old_event_mask_ = attributes.your_event_mask;	
>     XSelectInput(gfx::GetXDisplay(), DefaultRootWindow(gfx::GetXDisplay()),
> old_event_mask_ | PropertyChangeMask);	
>     XFlush(gfx::GetXDisplay());
> 
> But, GpuMain is not invoked at all, leave aside handling the PropertyNotify
> event. The status for XChangeProperty is 1 though.

Do we need to poll the message loop in GpuMain ?

[piman, 2015-01-30 20:57:09]

On Fri, Jan 30, 2015 at 4:58 AM, <sohan.jyoti@samsung.com> wrote:

> On 2015/01/30 12:54:01, sohanjg wrote:
>
>> On 2015/01/28 18:02:51, danakj wrote:
>> > On Jan 28, 2015 6:44 AM, <mailto:sohan.jyoti@samsung.com> wrote:
>> > >
>> > > On 2015/01/22 17:56:10, danakj wrote:
>> > >>
>> > >> On 2015/01/15 13:37:12, sohanjg wrote:
>> > >> > On 2015/01/06 16:10:23, danakj wrote:
>> > >> >
>> > >> > If we use XChangeProperty, say we use something like,
>> > >> > int state = 1;
>> > >> > XChangeProperty(gfx::GetXDisplay(),
>> > >> >                   DefaultRootWindow(gfx::GetXDisplay()),
>> > >> >                   ui::GetAtom("_NET_WM_STATE_FULLSCREEN"),
>> > >> >                   XA_CARDINAL,
>> > >> >                   32,
>> > >> >                   PropModeReplace,
>> > >> >                   reinterpret_cast<unsigned char*>(&state),
>> > >> >                   1);
>> > >> >
>> > >> > How should we plan to catch the PropertyNotify event ? some
>> > ProcessEvent in
>> > >> gpu
>> > >> > watchdog thread itself? or some other place ?
>> > >
>> > >
>> > >> I guess the main message loop is a UI loop and would receive X
>> events?
>> > >
>> > >
>> > > Will we able to catch the PropertyNotify event in
>> > > NativeViewGLSurfaceGLX::DispatchEvent ? Or we should check it in
>> GpuMain ?
>> >
>> > I don't actually know. Is this something you can test or read the code
>> to
>> > figure out?
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>  I am trying to send XChangeProp from GpuWatchdogThread::
>> CheckXServerActive,
>>
> like
>
>
>       unsigned char text[20] = "check";
>>      Atom atom = XInternAtom(gfx::GetXDisplay(), "CHK", False);
>>      int status = XChangeProperty(gfx::GetXDisplay(),
>>                               DefaultRootWindow(gfx::GetXDisplay()),
>>                               my_atom,
>>                               XA_STRING,
>>                               8,
>>                               PropModeReplace,
>>                               text,
>>                               5
>>                              );
>>      LOG(ERROR)<<__FUNCTION__<<" XChangeProperty status = "<<status;
>>      XWindowAttributes attributes;
>>      XGetWindowAttributes(gfx::GetXDisplay(),
>> DefaultRootWindow(gfx::GetXDisplay()), &attributes);
>>      long old_event_mask_ = attributes.your_event_mask;
>>      XSelectInput(gfx::GetXDisplay(), DefaultRootWindow(gfx::
>> GetXDisplay()),
>> old_event_mask_ | PropertyChangeMask);
>>      XFlush(gfx::GetXDisplay());
>>
>
>  But, GpuMain is not invoked at all, leave aside handling the
>> PropertyNotify
>> event. The status for XChangeProperty is 1 though.
>>
>
> Do we need to poll the message loop in GpuMain ?
>

I'm not sure I'm following your questions entirely.

GpuMain starts a message loop, which, on linux + X11 is a UI message loop.
That means it will poll X events as part of the message loop, and dispatch
them is something is listening to it. You can register to receive the
events you care about by making a ui::PlatformEventDispatcher and adding it
to ui::PlatformEventSource::GetInstance(). See for example what we do
in NativeViewGLSurfaceGLX to listen to Expose events.

That being said there are 2 constraints:
1- You can only access the main loop's Display (that you get
via gfx::GetXDisplay()) on the main loop. Otherwise it causes races.
2- The main loop can be busy. The whole point of the watchdog is to
determine if it is busy for a long time or not. If it is blocked, it will
not dispatch X messages. So if the goal is to detect whether X is active or
not when the watchdog triggers, this won't do it.

It really sounds like you want to query this on the watchdog thread, which
implies opening another Display. But then the PlatformEventSource mechanism
can't work as is, because it's a singleton. Also, we can't have >1 UI
message loops as it is. I suspect what you want is explicitly poll this
other Display in GpuWatchdogThread::OnCheck, with a timeout.



> https://codereview.chromium.org/836473003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sohanjg, 2015-02-02 14:17:19]

Thank you for your direction piman@.
This is a bit rough, can you' please have a look, if this is what we want.
thanks.

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:200: message_loop()->PostDelayedTask(
Should we post a delayed task here, or explicitly call CheckXServerActive, and
poll there till timeout ?

[piman, 2015-02-04 01:03:05]

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:45: window_(1),
why 1?

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:291: &nbytes, &property_data) == Success) {
If X is blocked, this will block. I don't think that's what we want, is it?
I would think instead we'd want to check in
DeliberatelyTerminateToRecoverFromHang, after we think the main thread is
blocked.
Then you would want to do a non-blocking round trip to X. Maybe setting a
property, flushing, and then polling for a property change (i.e. XSelectInput
with PropertyChangeMask - prior to setting the property - and then checking for
events with XCheckWindowEvent in a poll() loop over the Display's
ConnectionNumber, with a timeout).

[sohanjg, 2015-02-06 10:40:58]

Can you ptal, if this is what we want ? thanks.

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:45: window_(1),
On 2015/02/04 01:03:05, piman (Very slow to review) wrote:
> why 1?

Done.

https://codereview.chromium.org/836473003/diff/20001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:291: &nbytes, &property_data) == Success) {
On 2015/02/04 01:03:05, piman (Very slow to review) wrote:
> If X is blocked, this will block. I don't think that's what we want, is it?
> I would think instead we'd want to check in
> DeliberatelyTerminateToRecoverFromHang, after we think the main thread is
> blocked.
> Then you would want to do a non-blocking round trip to X. Maybe setting a
> property, flushing, and then polling for a property change (i.e. XSelectInput
> with PropertyChangeMask - prior to setting the property - and then checking
for
> events with XCheckWindowEvent in a poll() loop over the Display's
> ConnectionNumber, with a timeout).

Done.

[piman, 2015-02-06 22:49:54]

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:253: int status = poll(fds, 1,
timeout_.InMilliseconds());
1- we need to handle error values. In particular EINTR would mean we need to
restart the poll, but other errors would mean we need to abort (e.g. X died).
2- we need to treat the timeout globally across the loop. E.g. if we received a
signal every 5 seconds but the timeout is 10 seconds we would never terminate.
Something along the lines of getting the time before the loop, computing a
deadline, and then comparing the current time to the deadline in the while loop,
akin to:
base::TimeTicks deadline = base::TimeTicks::Now() + timeout_;
while (true) {
  base::TimeDelta delta = deadline - base::TimeTicks::Now();
  if (delta < base::TimeDelta()) {
     // timeout
  } else {
    int status = poll(..., delta.InMilliseconds());
    if (status == -1) {
      if (errno == EINTR) {
        // restart
        continue;
      } else {
        // error, abort
      }
    } else if (status == 0) {
      // timeout
    } else {
      if (XCheckWindowEvent(...)) {
        // got property
      } else {
        // other event, restart.
        continue;
      }
    }
  }
}

3- I think we need to check for the event even before the first poll. It's
possible in theory.
4- We could get more than 1 event (the window manager may want to tag properties
on our window), so the XCheckWindowEvent may need to be in a loop (to make sure
we consume all events), and check for the exact event we expect (the right
Atom).

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:259: return;
So, if I follow the overall logic, you're testing if X is alive, if it is you
cancel the watchdog trigger, but if it is not you keep going and crash.
I think we want the opposite. If X is alive, but the watchdog triggers, it means
we're not blocked waiting on X, so we're blocked waiting on the GPU to execute
GL commands. This is the typical case where we want to abort, because that means
that chrome is not responsive, possibly because of webgl content or something
else. If X is not responsive, it could be due to several things, among others
the VT switch as suggested in the bug in which case we do not want to crash, but
either way chrome and the desktop will not be responsive, but crashing the GPU
process is unlikely to fix it. I think that's the case where we want to avoid
the trigger.

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:307: unsigned char text[20] = "check";
const unsigned char text[] = "check";

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:308: atom_ = XInternAtom(display_, "CHK",
False);
We shouldn't need to do this every time. Do you want to do this in SetupXServer?

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:310: 5);
strlen(text) instead of 5?
Do we need to change the property every time, or will we get an XPropertyEvent
even if it doesn't change?

[sohanjg, 2015-02-13 05:18:00]

Thanks for your direction. Sorry for sitting on this, was caught up with
something else.
PTAL if this is what we want.

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:253: int status = poll(fds, 1,
timeout_.InMilliseconds());
On 2015/02/06 22:49:53, piman (Very slow to review) wrote:
> 1- we need to handle error values. In particular EINTR would mean we need to
> restart the poll, but other errors would mean we need to abort (e.g. X died).
> 2- we need to treat the timeout globally across the loop. E.g. if we received
a
> signal every 5 seconds but the timeout is 10 seconds we would never terminate.
> Something along the lines of getting the time before the loop, computing a
> deadline, and then comparing the current time to the deadline in the while
loop,
> akin to:
> base::TimeTicks deadline = base::TimeTicks::Now() + timeout_;
> while (true) {
>   base::TimeDelta delta = deadline - base::TimeTicks::Now();
>   if (delta < base::TimeDelta()) {
>      // timeout
>   } else {
>     int status = poll(..., delta.InMilliseconds());
>     if (status == -1) {
>       if (errno == EINTR) {
>         // restart
>         continue;
>       } else {
>         // error, abort
>       }
>     } else if (status == 0) {
>       // timeout
>     } else {
>       if (XCheckWindowEvent(...)) {
>         // got property
>       } else {
>         // other event, restart.
>         continue;
>       }
>     }
>   }
> }
> 
> 3- I think we need to check for the event even before the first poll. It's
> possible in theory.
> 4- We could get more than 1 event (the window manager may want to tag
properties
> on our window), so the XCheckWindowEvent may need to be in a loop (to make
sure
> we consume all events), and check for the exact event we expect (the right
> Atom).

Done.

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:259: return;
On 2015/02/06 22:49:54, piman (Very slow to review) wrote:
> So, if I follow the overall logic, you're testing if X is alive, if it is you
> cancel the watchdog trigger, but if it is not you keep going and crash.
> I think we want the opposite. If X is alive, but the watchdog triggers, it
means
> we're not blocked waiting on X, so we're blocked waiting on the GPU to execute
> GL commands. This is the typical case where we want to abort, because that
means
> that chrome is not responsive, possibly because of webgl content or something
> else. If X is not responsive, it could be due to several things, among others
> the VT switch as suggested in the bug in which case we do not want to crash,
but
> either way chrome and the desktop will not be responsive, but crashing the GPU
> process is unlikely to fix it. I think that's the case where we want to avoid
> the trigger.

Acknowledged.

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:307: unsigned char text[20] = "check";
On 2015/02/06 22:49:53, piman (Very slow to review) wrote:
> const unsigned char text[] = "check";

Done.

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:308: atom_ = XInternAtom(display_, "CHK",
False);
On 2015/02/06 22:49:54, piman (Very slow to review) wrote:
> We shouldn't need to do this every time. Do you want to do this in
SetupXServer?

Done.

https://codereview.chromium.org/836473003/diff/40001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:310: 5);
On 2015/02/06 22:49:53, piman (Very slow to review) wrote:
> strlen(text) instead of 5?
> Do we need to change the property every time, or will we get an XPropertyEvent
> even if it doesn't change?

We are getting the event even if we dont change.

[piman, 2015-02-13 22:28:39]

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:33: #endif
nit: move into anonymous namespace above.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:257: // timeout
It would be more useful to have a comment describing the loop and the overall
logic, something along the lines of:
"We wait for the property change event with a timeout. If it arrives we know
that X is responsive and is not the cause of the watchdog trigger, so we should
terminate. If it times out, it may be due to X taking a long time, but
terminating won't help, so ignore the watchdog trigger."

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:262: // got property
nit: remove comment

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:272: // restart
nit: remove comment

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:275: // error, abort
nit: remove comment

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:276: break;
In this case, we should LOG(FATAL) << "Lost X connection, aborting." to separate
the case from the watchdog trigger.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:278: // timeout
nit: remove comment

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:281: while (true) {
I don't think this second block makes sense. Can we just continue, but maybe
replace the if on line 260 by a while?

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:343: strlen((char*)text));
nit: no c-style cast.
You should be able to use arraysize(text)-1.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:352: if (XGetWindowProperty(display_,
window_, atom_, 0, 65535, False,
I was more imagining that we'd check the X event, verifying that the atom is the
same as the one we set.
As is, this would check if the property is there at all, which would be if we
went through the loop once and then timed out.

[sohanjg, 2015-02-16 09:44:50]

Thanks. Can you please take a look.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
File content/gpu/gpu_watchdog_thread.cc (right):

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:257: // timeout
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> It would be more useful to have a comment describing the loop and the overall
> logic, something along the lines of:
> "We wait for the property change event with a timeout. If it arrives we know
> that X is responsive and is not the cause of the watchdog trigger, so we
should
> terminate. If it times out, it may be due to X taking a long time, but
> terminating won't help, so ignore the watchdog trigger."

Done.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:262: // got property
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> nit: remove comment

Done.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:272: // restart
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> nit: remove comment

Done.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:275: // error, abort
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> nit: remove comment

Done.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:276: break;
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> In this case, we should LOG(FATAL) << "Lost X connection, aborting." to
separate
> the case from the watchdog trigger.

Done.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:281: while (true) {
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> I don't think this second block makes sense. Can we just continue, but maybe
> replace the if on line 260 by a while?

Done.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:343: strlen((char*)text));
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> nit: no c-style cast.
> You should be able to use arraysize(text)-1.

Done.

https://codereview.chromium.org/836473003/diff/60001/content/gpu/gpu_watchdog...
content/gpu/gpu_watchdog_thread.cc:352: if (XGetWindowProperty(display_,
window_, atom_, 0, 65535, False,
On 2015/02/13 22:28:38, piman (Very slow to review) wrote:
> I was more imagining that we'd check the X event, verifying that the atom is
the
> same as the one we set.
> As is, this would check if the property is there at all, which would be if we
> went through the loop once and then timed out.

Done.

[piman, 2015-02-17 16:50:10]

lgtm

[sohanjg, 2015-02-18 01:53:28]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2015-02-18 01:54:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836473003/80001

[commit-bot: I haz the power, 2015-02-18 03:13:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-18 03:14:48]

Patchset 5 (id:??) landed as
https://crrev.com/cf28abf824523e4d78b683680fdf87de4dc66901
Cr-Commit-Position: refs/heads/master@{#316757}

[sohanjg, 2015-11-18 10:06:08]

Description was changed from

==========
On timeout, we wait for a XProperty change event with additional
timeout. If it arrives we know that X is responsive and is not
the cause of the watchdog trigger, so we should terminate. If it
times out, it may be due to X taking a long time, but terminating
won't help, so ignore the watchdog trigger.

BUG=432095

Committed: https://crrev.com/cf28abf824523e4d78b683680fdf87de4dc66901
Cr-Commit-Position: refs/heads/master@{#316757}
==========

to

==========
On timeout, we wait for a XProperty change event with additional
timeout. If it arrives we know that X is responsive and is not
the cause of the watchdog trigger, so we should terminate. If it
times out, it may be due to X taking a long time, but terminating
won't help, so ignore the watchdog trigger.

BUG=432095

Committed: https://crrev.com/cf28abf824523e4d78b683680fdf87de4dc66901
Cr-Commit-Position: refs/heads/master@{#316757}
==========