Encourage reliability of code using browser thread.

Encourage reliability of code using browser thread.

I had a leak that occurred because the return value from DeleteSoon was not being checked. It looked like this:

{
scoped_ptr<AThing> foo = new AThing;
BrowserThread browser_ui_thread(BrowserThread::UI, &loop);
....
}

Turned out the leak occurred because AThing was set to only delete itself on a UI browser thread, but browser_ui_thread was going out of scope
before delete was being called. Adding the DLOG to DeleteSoon helps expose these errors.

BUG=NONE
TEST=BUILD

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

[dmac, 2011-04-04 20:48:39]

PTAL

[jam, 2011-04-04 23:50:51]

I don't think we want WARN_UNUSED_RESULT for PostTask/DeleteSoon etc.  The
return code is somewhat misleading, since the task can be posted but it might
not run, so it's not actually guaranteeing anything (just that it didn't fail
right away).  Littering all the code that uses these functions, to handle cases
that will almost never occur (we're ok with leaks at shutdown) seems like it
would make the code less readable.

[dmac, 2011-04-05 00:34:33]

Are you ok though with the DLOG in DeleteOnThread::Destruct? That's the
important thing that would've caught my issue.

On 2011/04/04 23:50:51, John Abd-El-Malek wrote:
> I don't think we want WARN_UNUSED_RESULT for PostTask/DeleteSoon etc.  The
> return code is somewhat misleading, since the task can be posted but it might
> not run, so it's not actually guaranteeing anything (just that it didn't fail
> right away).  Littering all the code that uses these functions, to handle
cases
> that will almost never occur (we're ok with leaks at shutdown) seems like it
> would make the code less readable.

[jam, 2011-04-05 00:36:57]

On Mon, Apr 4, 2011 at 5:34 PM, <dmaclach@chromium.org> wrote:

> Are you ok though with the DLOG in DeleteOnThread::Destruct? That's the
> important thing that would've caught my issue.


Does DLOG(FATAL) crash in debug builds?  If so, I worry that this would
happen at random times for people during development.


>
> On 2011/04/04 23:50:51, John Abd-El-Malek wrote:
>
>> I don't think we want WARN_UNUSED_RESULT for PostTask/DeleteSoon etc.  The
>> return code is somewhat misleading, since the task can be posted but it
>> might
>> not run, so it's not actually guaranteeing anything (just that it didn't
>> fail
>> right away).  Littering all the code that uses these functions, to handle
>>
> cases
>
>> that will almost never occur (we're ok with leaks at shutdown) seems like
>> it
>> would make the code less readable.
>>
>
>
>
> http://codereview.chromium.org/6691044/
>

[dmaclach1, 2011-04-05 03:21:49]

DLOG(FATAL) does crash in debug builds, but I think everytime it does
crash it's indicating a potential memory leak that would cause
Valgrind issues. I would say that we want to clean these up.

Cheers,
Dave

On Mon, Apr 4, 2011 at 5:36 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
> On Mon, Apr 4, 2011 at 5:34 PM, <dmaclach@chromium.org> wrote:
>>
>> Are you ok though with the DLOG in DeleteOnThread::Destruct? That's the
>> important thing that would've caught my issue.
>
> Does DLOG(FATAL) crash in debug builds?  If so, I worry that this would
> happen at random times for people during development.
>>
>>
>> On 2011/04/04 23:50:51, John Abd-El-Malek wrote:
>>>
>>> I don't think we want WARN_UNUSED_RESULT for PostTask/DeleteSoon etc.
>>>  The
>>> return code is somewhat misleading, since the task can be posted but it
>>> might
>>> not run, so it's not actually guaranteeing anything (just that it didn't
>>> fail
>>> right away).  Littering all the code that uses these functions, to handle
>>
>> cases
>>>
>>> that will almost never occur (we're ok with leaks at shutdown) seems like
>>> it
>>> would make the code less readable.
>>
>>
>>
>> http://codereview.chromium.org/6691044/
>
>

[jam, 2011-04-05 04:43:59]

On Mon, Apr 4, 2011 at 8:21 PM, David Maclachlan <dmaclach@google.com>wrote:

> DLOG(FATAL) does crash in debug builds, but I think everytime it does
> crash it's indicating a potential memory leak that would cause
> Valgrind issues. I would say that we want to clean these up.
>

These would happen at shutdown, when we know and we're ok with having leaks.
 When people use DeleteSoon/ReleaseSoon, they either want the stack to
unwind on the current thread, or they want the destruction to happen on a
different thread.  If these methods return false, then destructing the
object afterwards will likely not be safe.


>
> Cheers,
> Dave
>
> On Mon, Apr 4, 2011 at 5:36 PM, John Abd-El-Malek <jam@chromium.org>
> wrote:
> >
> > On Mon, Apr 4, 2011 at 5:34 PM, <dmaclach@chromium.org> wrote:
> >>
> >> Are you ok though with the DLOG in DeleteOnThread::Destruct? That's the
> >> important thing that would've caught my issue.
> >
> > Does DLOG(FATAL) crash in debug builds?  If so, I worry that this would
> > happen at random times for people during development.
> >>
> >>
> >> On 2011/04/04 23:50:51, John Abd-El-Malek wrote:
> >>>
> >>> I don't think we want WARN_UNUSED_RESULT for PostTask/DeleteSoon etc.
> >>>  The
> >>> return code is somewhat misleading, since the task can be posted but it
> >>> might
> >>> not run, so it's not actually guaranteeing anything (just that it
> didn't
> >>> fail
> >>> right away).  Littering all the code that uses these functions, to
> handle
> >>
> >> cases
> >>>
> >>> that will almost never occur (we're ok with leaks at shutdown) seems
> like
> >>> it
> >>> would make the code less readable.
> >>
> >>
> >>
> >> http://codereview.chromium.org/6691044/
> >
> >
>

[dmac, 2011-04-05 14:26:59]

On 2011/04/05 04:43:59, John Abd-El-Malek wrote:
> On Mon, Apr 4, 2011 at 8:21 PM, David Maclachlan <dmaclach@google.com>wrote:
> 
> > DLOG(FATAL) does crash in debug builds, but I think everytime it does
> > crash it's indicating a potential memory leak that would cause
> > Valgrind issues. I would say that we want to clean these up.
> >
> 
> These would happen at shutdown, when we know and we're ok with having leaks.
>  When people use DeleteSoon/ReleaseSoon, they either want the stack to
> unwind on the current thread, or they want the destruction to happen on a
> different thread.  If these methods return false, then destructing the
> object afterwards will likely not be safe.

OK, it's pretty neutered now, but at least will allow folks to see that a leak
is occurring once they start looking for it.

[jam, 2011-04-05 17:35:28]

sorry to nit so much on this.  I'm just worried that people will
misinterpret the warning.  As written, it seems to imply that if it doesn't
fire, the object will be deleted, which is not true.

can we do this only in unit tests?  in that case, we usually only have one
thread anyways, and so it's easy and harmless to avoid the leak there. in
the browser process, deleteonthread failing is not really an error on
shutdown, since almost all the objects that use it wouldn't be able to be
deleted on the UI thread (where this error is most likely to appear).  So
logging this might cause people to do the wrong behavior, which is why I'm
apprehensive about this showing up on shutdown in the browser.

On Tue, Apr 5, 2011 at 7:26 AM, <dmaclach@chromium.org> wrote:

> On 2011/04/05 04:43:59, John Abd-El-Malek wrote:
>
>> On Mon, Apr 4, 2011 at 8:21 PM, David Maclachlan <dmaclach@google.com
>> >wrote:
>>
>
>  > DLOG(FATAL) does crash in debug builds, but I think everytime it does
>> > crash it's indicating a potential memory leak that would cause
>> > Valgrind issues. I would say that we want to clean these up.
>> >
>>
>
>  These would happen at shutdown, when we know and we're ok with having
>> leaks.
>>  When people use DeleteSoon/ReleaseSoon, they either want the stack to
>> unwind on the current thread, or they want the destruction to happen on a
>> different thread.  If these methods return false, then destructing the
>> object afterwards will likely not be safe.
>>
>
> OK, it's pretty neutered now, but at least will allow folks to see that a
> leak
> is occurring once they start looking for it.


>
>
> http://codereview.chromium.org/6691044/
>

[dmac, 2011-04-05 19:31:49]

OK. Only fires under unit-testing now, and doesn't log FATAL as that causes
issues with a test that is intending that the delete not happen.

[jam, 2011-04-05 20:25:17]

lgtm