Clean up potentially leaky use of base::DeletePointer.

Clean up potentially leaky use of base::DeletePointer.

Posting a task to another thread doesn't guarantee that it will run,
and we should make sure to clean things up when it doesn't.

Most usage of base::DeletePointer seems to ignore this possibility,
so I changed DeletePointer to require explicit ownership of the pointer.

[hubbe, 2015-04-24 18:52:36]

hubbe@chromium.org changed reviewers:
+ darin@chromium.org

[hubbe, 2015-04-30 22:25:44]

hubbe@chromium.org changed reviewers:
+ danakj@chromium.org
- darin@chromium.org

[maniscalco, 2015-04-30 23:02:10]

(drive-by question)

When an object cannot be deleted on the thread on which it lives, is it better
to leak the object or invoke its dtor on the "wrong thread"?

Say we've got an object that lives on the foo thread and should be deleted on
the foo thread:

// Assuming we're executing on some thread other than the foo thread.
// Assuming old version of DeletePointer.
// object is a scoped_ptr<Object>
foo_task_runner->PostTask(FROM_HERE, 
    base::Bind(&base::DeletePointer<Object>, object.release()));

If the foo thread exists, things are good and the object is deleted on the foo
thread.  In the above code, if the foo thread doesn't exist, we leak the object.

// Assuming new version of DeletePointer.
foo_task_runner->PostTask(FROM_HERE, 
    base::Bind(&base::DeletePointer<Object>, object));

With the new version of DeletePointer, what happens when the foo thread doesn't
exist?  The PostTask will fail and the object will be deleted on the thread that
attempted to PostTask (the "wrong thread").

Leaking objects and destroying them on the wrong thread are both bad.  Unless
I'm misunderstanding, I believe this patch is trading one for the other.  IMO,
when we don't know if it's safe to delete the object on the current thread,
leaking seems like the lesser of two evils.  WDYT?

[chromium-reviews, 2015-04-30 23:05:09]

If the foo thread is gone, how can it be unsafe to delete it on another
thread?

    /Hubbe


On Thu, Apr 30, 2015 at 4:02 PM, <maniscalco@chromium.org> wrote:

> (drive-by question)
>
> When an object cannot be deleted on the thread on which it lives, is it
> better
> to leak the object or invoke its dtor on the "wrong thread"?
>
> Say we've got an object that lives on the foo thread and should be deleted
> on
> the foo thread:
>
> // Assuming we're executing on some thread other than the foo thread.
> // Assuming old version of DeletePointer.
> // object is a scoped_ptr<Object>
> foo_task_runner->PostTask(FROM_HERE,
>     base::Bind(&base::DeletePointer<Object>, object.release()));
>
> If the foo thread exists, things are good and the object is deleted on the
> foo
> thread.  In the above code, if the foo thread doesn't exist, we leak the
> object.
>
> // Assuming new version of DeletePointer.
> foo_task_runner->PostTask(FROM_HERE,
>     base::Bind(&base::DeletePointer<Object>, object));
>
> With the new version of DeletePointer, what happens when the foo thread
> doesn't
> exist?  The PostTask will fail and the object will be deleted on the
> thread that
> attempted to PostTask (the "wrong thread").
>
> Leaking objects and destroying them on the wrong thread are both bad.
> Unless
> I'm misunderstanding, I believe this patch is trading one for the other.
> IMO,
> when we don't know if it's safe to delete the object on the current thread,
> leaking seems like the lesser of two evils.  WDYT?
>
>
> https://codereview.chromium.org/1074963003/
>

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

[maniscalco, 2015-04-30 23:48:15]

It depends on what the dtor does (and dtors of the member variables, and base
classes).  A lot of classes are written assuming they will be destroyed on the
same thread on which they were created and may DCHECK (or in some cases even
CHECK) in their dtor to ensure it's executing on a specific thread.  Or maybe in
in the context of their dtor they invoke some method that checks to to see that
it's executing on the same thread on which it was constructed.

This search has a few examples:
https://code.google.com/p/chromium/codesearch#search/&q=~.*CHECK.*thread&sq=p...


On 2015/04/30 23:05:09, chromium-reviews wrote:
> If the foo thread is gone, how can it be unsafe to delete it on another
> thread?
> 
>     /Hubbe
> 
> 
> On Thu, Apr 30, 2015 at 4:02 PM, <mailto:maniscalco@chromium.org> wrote:
> 
> > (drive-by question)
> >
> > When an object cannot be deleted on the thread on which it lives, is it
> > better
> > to leak the object or invoke its dtor on the "wrong thread"?
> >
> > Say we've got an object that lives on the foo thread and should be deleted
> > on
> > the foo thread:
> >
> > // Assuming we're executing on some thread other than the foo thread.
> > // Assuming old version of DeletePointer.
> > // object is a scoped_ptr<Object>
> > foo_task_runner->PostTask(FROM_HERE,
> >     base::Bind(&base::DeletePointer<Object>, object.release()));
> >
> > If the foo thread exists, things are good and the object is deleted on the
> > foo
> > thread.  In the above code, if the foo thread doesn't exist, we leak the
> > object.
> >
> > // Assuming new version of DeletePointer.
> > foo_task_runner->PostTask(FROM_HERE,
> >     base::Bind(&base::DeletePointer<Object>, object));
> >
> > With the new version of DeletePointer, what happens when the foo thread
> > doesn't
> > exist?  The PostTask will fail and the object will be deleted on the
> > thread that
> > attempted to PostTask (the "wrong thread").
> >
> > Leaking objects and destroying them on the wrong thread are both bad.
> > Unless
> > I'm misunderstanding, I believe this patch is trading one for the other.
> > IMO,
> > when we don't know if it's safe to delete the object on the current thread,
> > leaking seems like the lesser of two evils.  WDYT?
> >
> >
> > https://codereview.chromium.org/1074963003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-05-04 22:53:26]

Perhaps all threads should name a successor, which inherits all tasks when
the thread dies.
(Then we can implement our out little Game of Threads in chrome to see who
can own all the tasks.)

   /Hubbe


On Mon, May 4, 2015 at 3:49 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Apr 30, 2015 at 4:05 PM, Fredrik Hubinette <hubbe@google.com>
> wrote:
>
>> If the foo thread is gone, how can it be unsafe to delete it on another
>> thread?
>>
>
> It's possible 2 objects are meant to be destroyed on the same thread but
> the threads gone, so they are destroyed in racy ways on 2 separate threads.
>
>
>>
>>     /Hubbe
>>
>>
>> On Thu, Apr 30, 2015 at 4:02 PM, <maniscalco@chromium.org> wrote:
>>
>>> (drive-by question)
>>>
>>> When an object cannot be deleted on the thread on which it lives, is it
>>> better
>>> to leak the object or invoke its dtor on the "wrong thread"?
>>>
>>> Say we've got an object that lives on the foo thread and should be
>>> deleted on
>>> the foo thread:
>>>
>>> // Assuming we're executing on some thread other than the foo thread.
>>> // Assuming old version of DeletePointer.
>>> // object is a scoped_ptr<Object>
>>> foo_task_runner->PostTask(FROM_HERE,
>>>     base::Bind(&base::DeletePointer<Object>, object.release()));
>>>
>>> If the foo thread exists, things are good and the object is deleted on
>>> the foo
>>> thread.  In the above code, if the foo thread doesn't exist, we leak the
>>> object.
>>>
>>> // Assuming new version of DeletePointer.
>>> foo_task_runner->PostTask(FROM_HERE,
>>>     base::Bind(&base::DeletePointer<Object>, object));
>>>
>>> With the new version of DeletePointer, what happens when the foo thread
>>> doesn't
>>> exist?  The PostTask will fail and the object will be deleted on the
>>> thread that
>>> attempted to PostTask (the "wrong thread").
>>>
>>> Leaking objects and destroying them on the wrong thread are both bad.
>>> Unless
>>> I'm misunderstanding, I believe this patch is trading one for the
>>> other.  IMO,
>>> when we don't know if it's safe to delete the object on the current
>>> thread,
>>> leaking seems like the lesser of two evils.  WDYT?
>>>
>>>
>>> https://codereview.chromium.org/1074963003/
>>>
>>
>>
>

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

[danakj, 2015-05-04 22:57:10]

On Thu, Apr 30, 2015 at 4:05 PM, Fredrik Hubinette <hubbe@google.com> wrote:

> If the foo thread is gone, how can it be unsafe to delete it on another
> thread?
>

It's possible 2 objects are meant to be destroyed on the same thread but
the threads gone, so they are destroyed in racy ways on 2 separate threads.


>
>     /Hubbe
>
>
> On Thu, Apr 30, 2015 at 4:02 PM, <maniscalco@chromium.org> wrote:
>
>> (drive-by question)
>>
>> When an object cannot be deleted on the thread on which it lives, is it
>> better
>> to leak the object or invoke its dtor on the "wrong thread"?
>>
>> Say we've got an object that lives on the foo thread and should be
>> deleted on
>> the foo thread:
>>
>> // Assuming we're executing on some thread other than the foo thread.
>> // Assuming old version of DeletePointer.
>> // object is a scoped_ptr<Object>
>> foo_task_runner->PostTask(FROM_HERE,
>>     base::Bind(&base::DeletePointer<Object>, object.release()));
>>
>> If the foo thread exists, things are good and the object is deleted on
>> the foo
>> thread.  In the above code, if the foo thread doesn't exist, we leak the
>> object.
>>
>> // Assuming new version of DeletePointer.
>> foo_task_runner->PostTask(FROM_HERE,
>>     base::Bind(&base::DeletePointer<Object>, object));
>>
>> With the new version of DeletePointer, what happens when the foo thread
>> doesn't
>> exist?  The PostTask will fail and the object will be deleted on the
>> thread that
>> attempted to PostTask (the "wrong thread").
>>
>> Leaking objects and destroying them on the wrong thread are both bad.
>> Unless
>> I'm misunderstanding, I believe this patch is trading one for the other.
>> IMO,
>> when we don't know if it's safe to delete the object on the current
>> thread,
>> leaking seems like the lesser of two evils.  WDYT?
>>
>>
>> https://codereview.chromium.org/1074963003/
>>
>
>

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

[danakj, 2015-05-04 23:12:19]

On Mon, May 4, 2015 at 3:53 PM, Fredrik Hubinette <hubbe@google.com> wrote:

> Perhaps all threads should name a successor, which inherits all tasks when
> the thread dies.
>

Maybe, but that's a lot more complicated than just having a weakptr. I
guess you'd have to tell all your weakptrs where to look next and give the
backrefs to the new thread so it could do the same.

Is this a problem outside of shutdown? Maybe users of patterns like
DeleteSoon should be (D)CHECKing that it succeeds, and we shouldn't just
blindly allow leaks. Proper shutdown should not be impossible?


> (Then we can implement our out little Game of Threads in chrome to see who
> can own all the tasks.)
>
>    /Hubbe
>
>
> On Mon, May 4, 2015 at 3:49 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Thu, Apr 30, 2015 at 4:05 PM, Fredrik Hubinette <hubbe@google.com>
>> wrote:
>>
>>> If the foo thread is gone, how can it be unsafe to delete it on another
>>> thread?
>>>
>>
>> It's possible 2 objects are meant to be destroyed on the same thread but
>> the threads gone, so they are destroyed in racy ways on 2 separate threads.
>>
>>
>>>
>>>     /Hubbe
>>>
>>>
>>> On Thu, Apr 30, 2015 at 4:02 PM, <maniscalco@chromium.org> wrote:
>>>
>>>> (drive-by question)
>>>>
>>>> When an object cannot be deleted on the thread on which it lives, is it
>>>> better
>>>> to leak the object or invoke its dtor on the "wrong thread"?
>>>>
>>>> Say we've got an object that lives on the foo thread and should be
>>>> deleted on
>>>> the foo thread:
>>>>
>>>> // Assuming we're executing on some thread other than the foo thread.
>>>> // Assuming old version of DeletePointer.
>>>> // object is a scoped_ptr<Object>
>>>> foo_task_runner->PostTask(FROM_HERE,
>>>>     base::Bind(&base::DeletePointer<Object>, object.release()));
>>>>
>>>> If the foo thread exists, things are good and the object is deleted on
>>>> the foo
>>>> thread.  In the above code, if the foo thread doesn't exist, we leak
>>>> the object.
>>>>
>>>> // Assuming new version of DeletePointer.
>>>> foo_task_runner->PostTask(FROM_HERE,
>>>>     base::Bind(&base::DeletePointer<Object>, object));
>>>>
>>>> With the new version of DeletePointer, what happens when the foo thread
>>>> doesn't
>>>> exist?  The PostTask will fail and the object will be deleted on the
>>>> thread that
>>>> attempted to PostTask (the "wrong thread").
>>>>
>>>> Leaking objects and destroying them on the wrong thread are both bad.
>>>> Unless
>>>> I'm misunderstanding, I believe this patch is trading one for the
>>>> other.  IMO,
>>>> when we don't know if it's safe to delete the object on the current
>>>> thread,
>>>> leaking seems like the lesser of two evils.  WDYT?
>>>>
>>>>
>>>> https://codereview.chromium.org/1074963003/
>>>>
>>>
>>>
>>
>

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

[chromium-reviews, 2015-05-04 23:21:03]

It's probably not a big issue, but there are places which create and stop
threads on demand in the code. (Such as the cast library, and tests...) I'm
not sure what's worth doing, but the current state of threading in chrome
is a bit scary I think.

    /Hubbe

On Mon, May 4, 2015 at 4:05 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Mon, May 4, 2015 at 3:53 PM, Fredrik Hubinette <hubbe@google.com>
> wrote:
>
>> Perhaps all threads should name a successor, which inherits all tasks
>> when the thread dies.
>>
>
> Maybe, but that's a lot more complicated than just having a weakptr. I
> guess you'd have to tell all your weakptrs where to look next and give the
> backrefs to the new thread so it could do the same.
>
> Is this a problem outside of shutdown? Maybe users of patterns like
> DeleteSoon should be (D)CHECKing that it succeeds, and we shouldn't just
> blindly allow leaks. Proper shutdown should not be impossible?
>
>
>> (Then we can implement our out little Game of Threads in chrome to see
>> who can own all the tasks.)
>>
>>    /Hubbe
>>
>>
>> On Mon, May 4, 2015 at 3:49 PM, Dana Jansens <danakj@chromium.org> wrote:
>>
>>> On Thu, Apr 30, 2015 at 4:05 PM, Fredrik Hubinette <hubbe@google.com>
>>> wrote:
>>>
>>>> If the foo thread is gone, how can it be unsafe to delete it on another
>>>> thread?
>>>>
>>>
>>> It's possible 2 objects are meant to be destroyed on the same thread but
>>> the threads gone, so they are destroyed in racy ways on 2 separate threads.
>>>
>>>
>>>>
>>>>     /Hubbe
>>>>
>>>>
>>>> On Thu, Apr 30, 2015 at 4:02 PM, <maniscalco@chromium.org> wrote:
>>>>
>>>>> (drive-by question)
>>>>>
>>>>> When an object cannot be deleted on the thread on which it lives, is
>>>>> it better
>>>>> to leak the object or invoke its dtor on the "wrong thread"?
>>>>>
>>>>> Say we've got an object that lives on the foo thread and should be
>>>>> deleted on
>>>>> the foo thread:
>>>>>
>>>>> // Assuming we're executing on some thread other than the foo thread.
>>>>> // Assuming old version of DeletePointer.
>>>>> // object is a scoped_ptr<Object>
>>>>> foo_task_runner->PostTask(FROM_HERE,
>>>>>     base::Bind(&base::DeletePointer<Object>, object.release()));
>>>>>
>>>>> If the foo thread exists, things are good and the object is deleted on
>>>>> the foo
>>>>> thread.  In the above code, if the foo thread doesn't exist, we leak
>>>>> the object.
>>>>>
>>>>> // Assuming new version of DeletePointer.
>>>>> foo_task_runner->PostTask(FROM_HERE,
>>>>>     base::Bind(&base::DeletePointer<Object>, object));
>>>>>
>>>>> With the new version of DeletePointer, what happens when the foo
>>>>> thread doesn't
>>>>> exist?  The PostTask will fail and the object will be deleted on the
>>>>> thread that
>>>>> attempted to PostTask (the "wrong thread").
>>>>>
>>>>> Leaking objects and destroying them on the wrong thread are both bad.
>>>>> Unless
>>>>> I'm misunderstanding, I believe this patch is trading one for the
>>>>> other.  IMO,
>>>>> when we don't know if it's safe to delete the object on the current
>>>>> thread,
>>>>> leaking seems like the lesser of two evils.  WDYT?
>>>>>
>>>>>
>>>>> https://codereview.chromium.org/1074963003/
>>>>>
>>>>
>>>>
>>>
>>
>

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