Fix some race condition between idle and timeout events.

Fix some race condition between idle and timeout events. Also fix a memory leak.


BUG=http://crosbug.com/16387
TEST=Linux desktop

[Yusuke Sato, 2011-06-11 15:50:26]

http://codereview.appspot.com/4568072/diff/1/bus/engineproxy.c
File bus/engineproxy.c (right):

http://codereview.appspot.com/4568072/diff/1/bus/engineproxy.c#newcode808
bus/engineproxy.c:808: * And use HIGH priority to avoid timeout event happening
before
So if we use HIGH, it is guaranteed that cancelled_idle_cb() will never be
called after timeout_cb() is called, correct? That's fine.

But I'm still wondering whether the following scenario could still happen or
not:

1. cancelled_cb() is called.
2. (timeout happens here)
3. cancelled_idle_cb() is called.
4. timeout_cb() is called, and touches 'data' which is already freed.

Thoughts?

[Yusuke Sato, 2011-06-11 16:00:36]

> But I'm still wondering whether the following scenario could still happen or
> not:

(If you're certain that it will never happen, please feel free to submit it. The
CL is LG other than that question.)

[Peng, 2011-06-12 01:31:56]

http://codereview.appspot.com/4568072/diff/1/bus/engineproxy.c
File bus/engineproxy.c (right):

http://codereview.appspot.com/4568072/diff/1/bus/engineproxy.c#newcode808
bus/engineproxy.c:808: * And use HIGH priority to avoid timeout event happening
before
On 2011/06/11 15:50:26, Yusuke Sato wrote:
> So if we use HIGH, it is guaranteed that cancelled_idle_cb() will never be
> called after timeout_cb() is called, correct? That's fine.
> 
> But I'm still wondering whether the following scenario could still happen or
> not:
> 
> 1. cancelled_cb() is called.
> 2. (timeout happens here)
> 3. cancelled_idle_cb() is called.
> 4. timeout_cb() is called, and touches 'data' which is already freed.
> 
> Thoughts?

I think it step 3. It will call engine_proxy_new_data_free() that will remove
the timeout by g_source_remove(). So I believe the timeout_cb() should not be
called anymore.

[Yusuke Sato, 2011-06-12 04:05:48]

LGTM

On 2011/06/12 01:31:56, Peng wrote:
> http://codereview.appspot.com/4568072/diff/1/bus/engineproxy.c
> File bus/engineproxy.c (right):
> 
> http://codereview.appspot.com/4568072/diff/1/bus/engineproxy.c#newcode808
> bus/engineproxy.c:808: * And use HIGH priority to avoid timeout event
happening
> before
> On 2011/06/11 15:50:26, Yusuke Sato wrote:
> > So if we use HIGH, it is guaranteed that cancelled_idle_cb() will never be
> > called after timeout_cb() is called, correct? That's fine.
> > 
> > But I'm still wondering whether the following scenario could still happen or
> > not:
> > 
> > 1. cancelled_cb() is called.
> > 2. (timeout happens here)
> > 3. cancelled_idle_cb() is called.
> > 4. timeout_cb() is called, and touches 'data' which is already freed.
> > 
> > Thoughts?
> 
> I think it step 3. It will call engine_proxy_new_data_free() that will remove
> the timeout by g_source_remove(). So I believe the timeout_cb() should not be
> called anymore.