PSA: Delaying destruction is generally the wrong solution - Chromium-dev

The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Message from discussion PSA: Delaying destruction is generally the wrong solution
Received-SPF: pass (google.com: domain of kaiw...@google.com designates 209.85.213.169 as permitted sender) client-ip=209.85.213.169;
MIME-Version: 1.0
Sender: kaiw...@google.com
In-Reply-To: <CACvaWvaBFTTadtJUtgGjO26k2mDvCTjEkbw-gXBkt9fxf8n...@mail.gmail.com>
References: <CAA4WUYiP_A3sc6KNC2U=F0ig+MxBDsgiQbQRB+=LZ5gw-gx...@mail.gmail.com>
 <CAAzs-C8dzXb=SgBPWxzpnR+ZzD4_poaRbZaEQLmK+FGGokw...@mail.gmail.com>
 <CAHbUkqGz1gR5Rw7ndUbBs5HvM=0tFFDQVfWsX5NvVweHuLv...@mail.gmail.com>
 <CACvaWvaq1LrT+8s7d5wfhp=5Lq44wTmK_+BrK_6YTTh0zUe...@mail.gmail.com>
 <CAFa1-K3bhLDFnVUCDkFOdh+MdkiGGiz9adyg9VwbzbNMvp0...@mail.gmail.com> <CACvaWvaBFTTadtJUtgGjO26k2mDvCTjEkbw-gXBkt9fxf8n...@mail.gmail.com>
From: Kai Wang <kaiw...@chromium.org>
Date: Wed, 11 Jul 2012 16:49:44 -0700
Message-ID: <CAGbRA6w_YDStUDBi6rD4xqA=fJncjx==eyvWWhkEbQjQu2c...@mail.gmail.com>
Subject: Re: [chromium-dev] PSA: Delaying destruction is generally the wrong solution
To: rsle...@chromium.org
Cc: Dominic Battre <bat...@google.com>, lambroslamb...@chromium.org, e...@chromium.org, 
	willc...@chromium.org, chromium-dev <chromium-...@chromium.org>
Content-Type: multipart/alternative; boundary=14dae9340fe73f5aec04c49682c7

--14dae9340fe73f5aec04c49682c7
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

FYI, I just submitted a change modifying WeakPtr comments and unittests:
http://src.chromium.org/viewvc/chrome?view=3Drev&revision=3D146212

I think this makes the code easier to understand :)

On Tue, Jul 3, 2012 at 2:14 PM, Ryan Sleevi <rsle...@chromium.org> wrote:

>
>
> On Tue, Jul 3, 2012 at 12:01 PM, Dominic Battre <bat...@google.com> wrote=
:
>
>>
>>
>>
>> On Mon, Jul 2, 2012 at 8:35 PM, Ryan Sleevi <rsle...@chromium.org> wrote=
:
>>
>>> On Mon, Jul 2, 2012 at 10:33 AM, Lambros Lambrou <
>>> lambroslamb...@chromium.org> wrote:
>>>
>>>> Unfortunately, that document (
>>>> http://www.chromium.org/developers/coding-style/important-abstractions=
-and-data-structures)
>>>> describes WeakPtr as "mostly thread-unsafe", and that "it should only =
be
>>>> used on the original thread it was created on".  Also, the header file=
 says
>>>> "When you get a WeakPtr ... the WeakPtr becomes bound to the current
>>>> thread."  And, the fatal blow: "You may only dereference the WeakPtr o=
n
>>>> that thread".  Ouch ouch ouch!
>>>>
>>>> I don't know if I'm typical of most engineers here, but these
>>>> statements are enough to scare me off using WeakPtr altogether, in fav=
or of
>>>> using RefCountedThreadSafe (which feels impossible to use wrongly, and
>>>> therefore much safer).
>>>>
>>>> If I can't "use" (whatever that means) a WeakPtr on a different thread=
,
>>>> surely that makes the class almost useless?  What am I missing?
>>>>
>>>> If you are editing that document, and you understand WeakPtr, please
>>>> explain when it is safe to use, and why it's safe in those circumstanc=
es.
>>>>  Otherwise, I'm simply too scared to use it at all! :)
>>>>
>>>> Lambros
>>>>
>>>
>>> With WeakPtr, they can be copy-constructed and destructed on arbitrary
>>> threads. They're thread-safe in that regard, as they're necessary for
>>> base::Bind/base::Callback semantics. What they're not thread-safe about=
 is
>>> de-referencing, which the docs specifically call out.
>>>
>>> Largely, it means that this pattern is UNSAFE
>>>
>>> // Called on file thread
>>> void MyClass::DoSomethingOnFileThread() {
>>>   // Do some work
>>>   int result =3D DoSomeWork();
>>>
>>>   // NOT SAFE.
>>>   io_thread_->PostTask(FROM_HERE,
>>> base::Bind(&MyClass::DoSomethingOnIOThread, weak_ptr_factory_.GetWeakPt=
r(),
>>> result);
>>> }
>>>
>>> This is because the returned WeakPtr is created on the FILE thread, but
>>> will be dereferenced on the IO thread.
>>>
>>> However, the encouragement for WeakPtr is that it can be used to
>>> decouple the lifetimes between callers, particularly when used with
>>> PostTaskAndReply/PostTaskAndReplyWithResult.
>>>
>>> // Called on FILE thread
>>> void ClassA::DoSomething() {
>>>   class_b->DoSomethingElse(base::Bind(&ClassA::HandleResults,
>>> weak_ptr_factory_.GetWeakPtr()));
>>> }
>>>
>>> // Called on ANY thread.
>>> // |callback| will be invoked on the same thread this is called from. I=
n
>>> this case, the FILE thread.
>>> void ClassB::DoSomethingElse(const base::Callback<int>& calback) {
>>>   PostTaskAndReplyWithHelper(io_thread_, FROM_HERE,
>>> base::Bind(&ClassB::DoSomethingOnIOThread, base::Unretained(this)),
>>> callback);
>>> }
>>>
>>
>> Can you remind me why this call to base::Unretained(this) is safe? If th=
e
>> object of ClassB is deallocated while the task is in the queue, this wil=
l
>> fail, right?
>>
>
> In the above example, the concept being demonstrated was that ClassA can
> safely disappear while ClassB is still working and still has a callback
> that may eventually try to call ClassA (but won't, due to the WeakPtr). T=
he
> lifetime of ClassB wasn't really demonstrated. Minimally, it would be
> presumed that it must outlive all classes that a naked pointer has been
> given to (eg: it must outlive ClassA).
>
> Further, since ClassB seems (from the small example) to be doing all its
> work on the IO thread, it would likely be argued that it "lives" on the I=
O
> thread, and thus should be shut down / destroyed on that thread - such as
> using DeleteSoon, either by itself or by the object that truly owns the
> ClassB*.
>
> If ClassB were destroyed on the FILE thread, while tasks were
> pending/executing on the IO thread, yes, this would be problematic.
>
>
>> Or more general: What is the desired pattern to call yourself on another
>> thread?
>>
>
> The very-high-level answer is "In general, don't". That's the general
> preference for using objects that are single-threaded.
>
> However, in practice, this isn't always reasonable.
>
> - Does your interface have specific requirements on destruction ordering?
> Such as "There can be no pending operations" (eg: something you can DCHEC=
K
> in the destructor)? If so, you can use these guarantees to know if you ha=
ve
> messages pending that are posted to yourself.
> - Does your interface provide a thread-safe public-interface? Do you
> require all callers call you on a particular thread? Then having a
> WeakPtrFactory member and posting a WeakPtr between yourself may be
> appropriate.
> - Do you have a more complicated class that does need to work on multiple
> threads (mod caveats)? If it doesn't make sense to split the interface in=
to
> multiple public objects, then it may make more sense to use an internal
> Core that lives on one thread, while your public interface lives on
> another. To call the Core, the Owner uses PostTaskAndReply[WithResult],
> binding the Response callbacks as WeakPtrs to the Owner. To delete the
> Core, the Owner uses DeleteSoon to delete the Core on the appropriate
> thread. If the Core wishes to call itself (via MessageLoop::PostTask), th=
e
> Core uses a WeakPtrFactory<Core>, rather than this/base::Unretained(this)=
.
> The ownership semantics are clear - Owner always owns the Core - which
> resolves the concerns with RefCounting (especially if you end up
> refcounting both Core and Owner)
>
> Sorry, does that provide more context to the desired pattern?
>
>
>> Best regards,
>> Dominci
>>
>
>> // Called on IO thread
>>> int ClassB::DoSomethingOnIOThread() {
>>>   int rv =3D DoSomeWork();
>>>   return rv;
>>> }
>>>
>>> With such an approach, ClassA can disappear at /any/ time - even while
>>> ClassB is off busy doing work on the IO thread. The only requirements h=
ere
>>> are that ClassB remains valid for as long as a ClassA is around - aka, =
your
>>> normal threading/ownership guarantees.
>>>
>>> When ClassB::DoSomethingOnIOThread completes, the response will be
>>> posted back to the FILE thread. If ClassA has disappeared, the callback
>>> will no-op. With a RefCountedThreadSafe approach, the reference to Clas=
sA
>>> will be kept alive until ClassB completes - and further,
>>> ClassA::HandleResults will be invoked, which means you need to guarante=
e
>>> any objects needed by HandleResults will also be alive.
>>>
>>> Part of the WeakPtr approach is trying to encourage a design where each
>>> object has a single thread it lives on - both birth and death. While it=
 may
>>> be thread-safe (callable from multiple threads), the lifetime itself ha=
s
>>> strong guarantees. For classes that are refcounted and may live on mult=
iple
>>> threads, this often requires splitting up the class into distinct parts=
.
>>> That is, here's the part that lives on the FILE thread, here's the part
>>> that lives on the IO thread, and then wrapped by a super-class to manag=
e
>>> ownership of these helpers and provide the thread-safe interface.
>>>
>>> That is, WeakPtr is specifically trying to DISCOURAGE this pattern:
>>>
>>> // Not ideal. Ambiguous lifetime for ClassA.
>>> class ClassA : public base::RefCountedThreadSafe<ClassA> {
>>>   ...
>>> };
>>>
>>> // Called on IO thread.
>>> void ClassA::CalledOnIOThread() {
>>>   file_thread_->PostTask(FROM_HERE,
>>> base::Bind(&ClassA::CalledOnFileThread, this));
>>> }
>>>
>>> // Called on FILE thread
>>> void ClassA::CalledOnFileThread() {
>>>   int result =3D DoWork();
>>>   io_thread_->PostTask(FROM_HERE, base::Bind(&ClassA::HandleResults,
>>> this, result));
>>> }
>>>
>>> // Called on IO thread.
>>> void ClassA::HandleResults(int result) {
>>>   // ...
>>> }
>>>
>>>
>>>>
>>>>
>>>>
>>>> On Mon, Jul 2, 2012 at 10:01 AM, Elliot Glaysher (Chromium) <
>>>> e...@chromium.org> wrote:
>>>>
>>>>> +1
>>>>>
>>>>> On Fri, Jun 29, 2012 at 5:03 PM, William Chan (=E9=99=88=E6=99=BA=E6=
=98=8C)
>>>>> <willc...@chromium.org> wrote:
>>>>> > TL;DR: Don't delay destruction (PostTask or refcounting), fix your
>>>>> > destruction order / lifetime semantics.
>>>>> >
>>>>> > Let's say you have A which owns B which owns C. C accesses A via a
>>>>> Delegate
>>>>> > interface, which is the Chromium paradigm for calling back up the
>>>>> stack
>>>>> > without violating layering. In a normal sequence of events, A's
>>>>> destruction
>>>>> > would trigger B's destruction, which would trigger C's destruction,
>>>>> which
>>>>> > would prevent C from calling back into A. Now, let's say A is
>>>>> destroyed,
>>>>> > which *should* trigger B's destruction, but to fix another bug, you
>>>>> delay
>>>>> > B's destruction. This also delays the destruction of all the object=
s
>>>>> that B
>>>>> > transitively owns. These objects now will not get the destruction /
>>>>> shutdown
>>>>> > notification that would prevent them from calling back up the stack
>>>>> via a
>>>>> > Delegate interface. Now you have a use-after-free (C calling into t=
he
>>>>> > already freed A) that basically no one understands and it requires
>>>>> engineers
>>>>> > who understand the relationships across A, B, C, ... Z to spend man=
y
>>>>> > engineering hours to figure out what is causing the use-after-free
>>>>> bug.
>>>>> >
>>>>> > I understand why these destruction delay bandaids are initially
>>>>> alluring,
>>>>> > since they fix the problem, but lead to much more subtle ones in
>>>>> different
>>>>> > subsystems that aren't initially caught in normal testing. Also,
>>>>> fixing
>>>>> > destruction ordering / lifetime semantics is difficult because it m=
ay
>>>>> > require understanding lifetime semantics across different subsystem=
s.
>>>>> > Remember though, if you don't understand these semantics, you are
>>>>> forcing
>>>>> > incurring significant costs on the stability and security teams who
>>>>> get
>>>>> > these mysterious use-after-frees without obvious root causes, and
>>>>> have to
>>>>> > spend time hunting down the culprit. Also remember that, even if C
>>>>> does not
>>>>> > access A via a Delegate interface today, it's a very natural thing
>>>>> to happen
>>>>> > in the future, and if the author of the Delegate doesn't see any
>>>>> problems
>>>>> > during testing, then it'll result in mysterious crash reports that
>>>>> are no
>>>>> > fun to debug.
>>>>> >
>>>>> > I mentioned refcounting, because it's a variant of delaying
>>>>> destruction.
>>>>> > Let's say X owns Y, and it turns out the Z references Y (but Z
>>>>> doesn't
>>>>> > really own Y, which is why Y isn't already refcounted). The allurin=
g
>>>>> option
>>>>> > is to simply refcount Y, despite the fact that Z doesn't own Y. Z w=
as
>>>>> > supposed to be have been destroyed at the correct time during
>>>>> shutdown, but
>>>>> > that didn't happen. So we refcount Y, and now X gets deleted, but Y
>>>>> does
>>>>> > not, due to Z's destruction ordering being broken, so now objects
>>>>> that Y
>>>>> > owns that call into X via a Delegate interface or something cause
>>>>> > use-after-frees. Bad news. I hate refcounting.
>>>>> >
>>>>> > So please, stop delaying destruction. Spend the time to figure out
>>>>> why the
>>>>> > destruction order isn't occurring as expected, and fix it.
>>>>> >
>>>>> > PS: I'm off on vacation for a few weeks, so I probably won't respon=
d
>>>>> :) So
>>>>> > flame away!
>>>>> >
>>>>> > --
>>>>> > Chromium Developers mailing list: chromium-...@chromium.org
>>>>> > View archives, change email options, or unsubscribe:
>>>>> > http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>
>>>>> --
>>>>> Chromium Developers mailing list: chromium-...@chromium.org
>>>>> View archives, change email options, or unsubscribe:
>>>>>     http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>
>>>>
>>>>  --
>>>> Chromium Developers mailing list: chromium-...@chromium.org
>>>> View archives, change email options, or unsubscribe:
>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>
>>>
>>>  --
>>> Chromium Developers mailing list: chromium-...@chromium.org
>>> View archives, change email options, or unsubscribe:
>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>
>>
>>
>  --
> Chromium Developers mailing list: chromium-...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>

--14dae9340fe73f5aec04c49682c7
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

FYI, I just submitted a change modifying WeakPtr comments and unittests:<di=
v><a href=3D"http://src.chromium.org/viewvc/chrome?view=3Drev&amp;revision=
=3D146212">http://src.chromium.org/viewvc/chrome?view=3Drev&amp;revision=3D=
146212</a></div>

<div><br></div><div>I think this makes the code easier to understand :)<br>=
<br><div class=3D"gmail_quote">On Tue, Jul 3, 2012 at 2:14 PM, Ryan Sleevi =
<span dir=3D"ltr">&lt;<a href=3D"mailto:rsle...@chromium.org" target=3D"_bl=
ank">rsle...@chromium.org</a>&gt;</span> wrote:<br>

<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex">
<br><br><div class=3D"gmail_quote"><div><div>On Tue, Jul 3, 2012 at 12:01 P=
M, Dominic Battre <span dir=3D"ltr">&lt;<a href=3D"mailto:bat...@google.com=
" target=3D"_blank">bat...@google.com</a>&gt;</span> wrote:<br><blockquote =
class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid=
;padding-left:1ex">



<br><div class=3D"gmail_extra"><br><br><div class=3D"gmail_quote"><div>On M=
on, Jul 2, 2012 at 8:35 PM, Ryan Sleevi <span dir=3D"ltr">&lt;<a href=3D"ma=
ilto:rsle...@chromium.org" target=3D"_blank">rsle...@chromium.org</a>&gt;</=
span> wrote:<br>





<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex"><div>On Mon, Jul 2, 2012 at 10:33 AM, Lambro=
s Lambrou <span dir=3D"ltr">&lt;<a href=3D"mailto:lambroslamb...@chromium.o=
rg" target=3D"_blank">lambroslamb...@chromium.org</a>&gt;</span> wrote:<br>





</div><div class=3D"gmail_quote"><div><blockquote class=3D"gmail_quote" sty=
le=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Unfortunately, that document (<a href=3D"http://www.chromium.org/developers=
/coding-style/important-abstractions-and-data-structures" target=3D"_blank"=
>http://www.chromium.org/developers/coding-style/important-abstractions-and=
-data-structures</a>) describes WeakPtr as &quot;mostly thread-unsafe&quot;=
, and that &quot;it should only be used on the original thread it was creat=
ed on&quot;. =C2=A0Also, the header file says &quot;When you get a WeakPtr =
...=C2=A0the WeakPtr becomes bound to the current thread.&quot; =C2=A0And, =
the fatal blow: &quot;You may only=C2=A0dereference the WeakPtr on that thr=
ead&quot;. =C2=A0Ouch ouch ouch!<div>







<br></div><div>I don&#39;t know if I&#39;m typical of most engineers here, =
but these statements are enough to scare me off using WeakPtr altogether, i=
n favor of using RefCountedThreadSafe (which feels impossible to use wrongl=
y, and therefore much safer).</div>







<div><br></div><div>If I can&#39;t &quot;use&quot; (whatever that means) a =
WeakPtr on a different thread, surely that makes the class almost useless? =
=C2=A0What am I missing?</div><div><br></div><div>If you are editing that d=
ocument, and you understand WeakPtr, please explain when it is safe to use,=
 and why it&#39;s safe in those circumstances. =C2=A0Otherwise, I&#39;m sim=
ply too scared to use it at all! :)</div>






<span><font color=3D"#888888">
<div><br></div><div>Lambros</div></font></span></blockquote><div><br></div>=
</div><div>With WeakPtr, they can be copy-constructed and destructed on arb=
itrary threads. They&#39;re thread-safe in that regard, as they&#39;re nece=
ssary for base::Bind/base::Callback semantics. What they&#39;re not thread-=
safe about is de-referencing, which the docs specifically call out.</div>






<div><br></div><div>Largely, it means that this pattern is UNSAFE</div><div=
><br></div><div>// Called on file thread</div><div>void MyClass::DoSomethin=
gOnFileThread() {</div><div>=C2=A0 // Do some work</div><div>=C2=A0 int res=
ult =3D DoSomeWork();</div>






<div><br></div><div>=C2=A0 // NOT SAFE.</div><div>=C2=A0 io_thread_-&gt;Pos=
tTask(FROM_HERE, base::Bind(&amp;MyClass::DoSomethingOnIOThread, weak_ptr_f=
actory_.GetWeakPtr(), result);</div><div>}</div><div><br></div><div>This is=
 because the returned WeakPtr is created on the FILE thread, but will be de=
referenced on the IO thread.</div>






<div><br></div><div>However, the encouragement for WeakPtr is that it can b=
e used to decouple the lifetimes between callers, particularly when used wi=
th PostTaskAndReply/PostTaskAndReplyWithResult.</div><div><br></div><div>






// Called on FILE thread</div><div>void ClassA::DoSomething() {</div><div>=
=C2=A0 class_b-&gt;DoSomethingElse(base::Bind(&amp;ClassA::HandleResults, w=
eak_ptr_factory_.GetWeakPtr()));</div><div>}</div><div><br></div><div>// Ca=
lled on ANY thread.</div>






<div>// |callback| will be invoked on the same thread this is called from. =
In this case, the FILE thread.</div><div>void ClassB::DoSomethingElse(const=
 base::Callback&lt;int&gt;&amp; calback) {</div><div>=C2=A0 PostTaskAndRepl=
yWithHelper(io_thread_, FROM_HERE, base::Bind(&amp;ClassB::DoSomethingOnIOT=
hread, base::Unretained(this)), callback);</div>






<div>}</div></div></blockquote><div><br></div></div><div>Can you remind me =
why this call to base::Unretained(this) is safe? If the object of ClassB is=
 deallocated while the task is in the queue, this will fail, right?</div>



</div></div></blockquote><div><br></div></div></div><div><div>In the above =
example, the concept being demonstrated was that ClassA can safely disappea=
r while ClassB is still working and still has a callback that may eventuall=
y try to call ClassA (but won&#39;t, due to the WeakPtr). The lifetime of C=
lassB wasn&#39;t really demonstrated. Minimally, it would be presumed that =
it must outlive all classes that a naked pointer has been given to (eg: it =
must outlive ClassA).</div>



<div><br></div><div>Further, since ClassB seems (from the small example) to=
 be doing all its work on the IO thread, it would likely be argued that it =
&quot;lives&quot; on the IO thread, and thus should be shut down / destroye=
d on that thread - such as using DeleteSoon, either by itself or by the obj=
ect that truly owns the ClassB*.</div>



</div><div><br></div><div>If ClassB were destroyed on the FILE thread, whil=
e tasks were pending/executing on the IO thread, yes, this would be problem=
atic.</div><div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D=
"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">



<div class=3D"gmail_extra"><div class=3D"gmail_quote"><div>Or more general:=
 What is the desired pattern to call yourself on another thread?</div></div=
></div></blockquote><div><br></div></div><div>The very-high-level answer is=
 &quot;In general, don&#39;t&quot;. That&#39;s the general preference for u=
sing objects that are single-threaded.</div>



<div><br></div><div>However, in practice, this isn&#39;t always reasonable.=
</div><div><br></div><div>- Does your interface have specific requirements =
on destruction ordering? Such as &quot;There can be no pending operations&q=
uot; (eg: something you can DCHECK in the destructor)? If so, you can use t=
hese guarantees to know if you have messages pending that are posted to you=
rself.</div>



<div>- Does your interface provide a thread-safe public-interface? Do you r=
equire all callers call you on a particular thread? Then having a WeakPtrFa=
ctory member and posting a WeakPtr between yourself may be appropriate.</di=
v>



<div>- Do you have a more complicated class that does need to work on multi=
ple threads (mod caveats)? If it doesn&#39;t make sense to split the interf=
ace into multiple public objects, then it may make more sense to use an int=
ernal Core that lives on one thread, while your public interface lives on a=
nother. To call the Core, the Owner uses PostTaskAndReply[WithResult], bind=
ing the Response callbacks as WeakPtrs to the Owner. To delete the Core, th=
e Owner uses DeleteSoon to delete the Core on the appropriate thread. If th=
e Core wishes to call itself (via MessageLoop::PostTask), the Core uses a W=
eakPtrFactory&lt;Core&gt;, rather than this/base::Unretained(this). The own=
ership semantics are clear - Owner always owns the Core - which resolves th=
e concerns with RefCounting (especially if you end up refcounting both Core=
 and Owner)</div>



<div><br></div><div>Sorry, does that provide more context to the desired pa=
ttern?</div><div><div><div><br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=3D"gmail_extra">
<div class=3D"gmail_quote"><div><br></div><div>

Best regards,</div><div>Dominci</div></div></div></blockquote><blockquote c=
lass=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;=
padding-left:1ex"><div class=3D"gmail_extra"><div class=3D"gmail_quote"><di=
v><div>



<div><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex=
;border-left:1px #ccc solid;padding-left:1ex">

<div class=3D"gmail_quote"><div>// Called on IO thread</div><div>int ClassB=
::DoSomethingOnIOThread() {</div><div>=C2=A0 int rv =3D DoSomeWork();</div>=
<div>=C2=A0 return rv;</div><div>}</div><div><br></div><div>With such an ap=
proach, ClassA can disappear at /any/ time - even while ClassB is off busy =
doing work on the IO thread. The only requirements here are that ClassB rem=
ains valid for as long as a ClassA is around - aka, your normal threading/o=
wnership guarantees.</div>






<div><br></div><div>When ClassB::DoSomethingOnIOThread completes, the respo=
nse will be posted back to the FILE thread. If ClassA has disappeared, the =
callback will no-op. With a RefCountedThreadSafe approach, the reference to=
 ClassA will be kept alive until ClassB completes - and further, ClassA::Ha=
ndleResults will be invoked, which means you need to guarantee any objects =
needed by HandleResults will also be alive.</div>






<div><br></div><div>Part of the WeakPtr approach is trying to encourage a d=
esign where each object has a single thread it lives on - both birth and de=
ath. While it may be thread-safe (callable from multiple threads), the life=
time itself has strong guarantees. For classes that are refcounted and may =
live on multiple threads, this often requires splitting up the class into d=
istinct parts. That is, here&#39;s the part that lives on the FILE thread, =
here&#39;s the part that lives on the IO thread, and then wrapped by a supe=
r-class to manage ownership of these helpers and provide the thread-safe in=
terface.</div>






<div><br></div><div>That is, WeakPtr is specifically trying to DISCOURAGE t=
his pattern:</div><div><br></div><div>// Not ideal. Ambiguous lifetime for =
ClassA.</div><div>class ClassA : public base::RefCountedThreadSafe&lt;Class=
A&gt; {</div>






<div>=C2=A0 ...</div><div>};</div><div><br></div><div>// Called on IO threa=
d.</div><div>void ClassA::CalledOnIOThread() {</div><div>=C2=A0 file_thread=
_-&gt;PostTask(FROM_HERE, base::Bind(&amp;ClassA::CalledOnFileThread, this)=
);</div>






<div>}</div><div><br></div><div>// Called on FILE thread</div><div>void Cla=
ssA::CalledOnFileThread() {</div><div>=C2=A0 int result =3D DoWork();</div>=
<div>=C2=A0 io_thread_-&gt;PostTask(FROM_HERE, base::Bind(&amp;ClassA::Hand=
leResults, this, result));</div>






<div>}</div><div><br></div><div>// Called on IO thread.</div><div>void Clas=
sA::HandleResults(int result) {</div><div>=C2=A0 // ...</div><div><div><div=
>}</div><div><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0 =
0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">






<div><div><div><br></div><div><br></div><div class=3D"gmail_extra"><br><br>=
<div class=3D"gmail_quote">On Mon, Jul 2, 2012 at 10:01 AM, Elliot Glaysher=
 (Chromium) <span dir=3D"ltr">&lt;<a href=3D"mailto:e...@chromium.org" targe=
t=3D"_blank">e...@chromium.org</a>&gt;</span> wrote:<br>







<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex">+1<br>
<div><div><br>
On Fri, Jun 29, 2012 at 5:03 PM, William Chan (=E9=99=88=E6=99=BA=E6=98=8C)=
<br>
&lt;<a href=3D"mailto:willc...@chromium.org" target=3D"_blank">willchan@chr=
omium.org</a>&gt; wrote:<br>
&gt; TL;DR: Don&#39;t delay destruction (PostTask or refcounting), fix your=
<br>
&gt; destruction order / lifetime semantics.<br>
&gt;<br>
&gt; Let&#39;s say you have A which owns B which owns C. C accesses A via a=
 Delegate<br>
&gt; interface, which is the Chromium paradigm for calling back up the stac=
k<br>
&gt; without violating layering. In a normal sequence of events, A&#39;s de=
struction<br>
&gt; would trigger B&#39;s destruction, which would trigger C&#39;s destruc=
tion, which<br>
&gt; would prevent C from calling back into A. Now, let&#39;s say A is dest=
royed,<br>
&gt; which *should* trigger B&#39;s destruction, but to fix another bug, yo=
u delay<br>
&gt; B&#39;s destruction. This also delays the destruction of all the objec=
ts that B<br>
&gt; transitively owns. These objects now will not get the destruction / sh=
utdown<br>
&gt; notification that would prevent them from calling back up the stack vi=
a a<br>
&gt; Delegate interface. Now you have a use-after-free (C calling into the<=
br>
&gt; already freed A) that basically no one understands and it requires eng=
ineers<br>
&gt; who understand the relationships across A, B, C, ... Z to spend many<b=
r>
&gt; engineering hours to figure out what is causing the use-after-free bug=
.<br>
&gt;<br>
&gt; I understand why these destruction delay bandaids are initially alluri=
ng,<br>
&gt; since they fix the problem, but lead to much more subtle ones in diffe=
rent<br>
&gt; subsystems that aren&#39;t initially caught in normal testing. Also, f=
ixing<br>
&gt; destruction ordering / lifetime semantics is difficult because it may<=
br>
&gt; require understanding lifetime semantics across different subsystems.<=
br>
&gt; Remember though, if you don&#39;t understand these semantics, you are =
forcing<br>
&gt; incurring significant costs on the stability and security teams who ge=
t<br>
&gt; these mysterious use-after-frees without obvious root causes, and have=
 to<br>
&gt; spend time hunting down the culprit. Also remember that, even if C doe=
s not<br>
&gt; access A via a Delegate interface today, it&#39;s a very natural thing=
 to happen<br>
&gt; in the future, and if the author of the Delegate doesn&#39;t see any p=
roblems<br>
&gt; during testing, then it&#39;ll result in mysterious crash reports that=
 are no<br>
&gt; fun to debug.<br>
&gt;<br>
&gt; I mentioned refcounting, because it&#39;s a variant of delaying destru=
ction.<br>
&gt; Let&#39;s say X owns Y, and it turns out the Z references Y (but Z doe=
sn&#39;t<br>
&gt; really own Y, which is why Y isn&#39;t already refcounted). The alluri=
ng option<br>
&gt; is to simply refcount Y, despite the fact that Z doesn&#39;t own Y. Z =
was<br>
&gt; supposed to be have been destroyed at the correct time during shutdown=
, but<br>
&gt; that didn&#39;t happen. So we refcount Y, and now X gets deleted, but =
Y does<br>
&gt; not, due to Z&#39;s destruction ordering being broken, so now objects =
that Y<br>
&gt; owns that call into X via a Delegate interface or something cause<br>
&gt; use-after-frees. Bad news. I hate refcounting.<br>
&gt;<br>
&gt; So please, stop delaying destruction. Spend the time to figure out why=
 the<br>
&gt; destruction order isn&#39;t occurring as expected, and fix it.<br>
&gt;<br>
&gt; PS: I&#39;m off on vacation for a few weeks, so I probably won&#39;t r=
espond :) So<br>
&gt; flame away!<br>
&gt;<br>
&gt; --<br>
&gt; Chromium Developers mailing list: <a href=3D"mailto:chromium-dev@chrom=
ium.org" target=3D"_blank">chromium-...@chromium.org</a><br>
&gt; View archives, change email options, or unsubscribe:<br>
&gt; <a href=3D"http://groups.google.com/a/chromium.org/group/chromium-dev"=
 target=3D"_blank">http://groups.google.com/a/chromium.org/group/chromium-d=
ev</a><br>
<br>
--<br>
Chromium Developers mailing list: <a href=3D"mailto:chromium-...@chromium.o=
rg" target=3D"_blank">chromium-...@chromium.org</a><br>
View archives, change email options, or unsubscribe:<br>
=C2=A0 =C2=A0 <a href=3D"http://groups.google.com/a/chromium.org/group/chro=
mium-dev" target=3D"_blank">http://groups.google.com/a/chromium.org/group/c=
hromium-dev</a><br>
</div></div></blockquote></div><br></div>

<p></p>

-- <br>
Chromium Developers mailing list: <a href=3D"mailto:chromium-...@chromium.o=
rg" target=3D"_blank">chromium-...@chromium.org</a><br>
View archives, change email options, or unsubscribe: <br>
    <a href=3D"http://groups.google.com/a/chromium.org/group/chromium-dev" =
target=3D"_blank">http://groups.google.com/a/chromium.org/group/chromium-de=
v</a><br>
</div></div></blockquote></div></div></div><div><div><br>

<p></p>

-- <br>
Chromium Developers mailing list: <a href=3D"mailto:chromium-...@chromium.o=
rg" target=3D"_blank">chromium-...@chromium.org</a><br>
View archives, change email options, or unsubscribe: <br>
    <a href=3D"http://groups.google.com/a/chromium.org/group/chromium-dev" =
target=3D"_blank">http://groups.google.com/a/chromium.org/group/chromium-de=
v</a><br>
</div></div></blockquote></div></div></div><br></div>
</blockquote></div></div></div><div><div><br>

<p></p>

-- <br>
Chromium Developers mailing list: <a href=3D"mailto:chromium-...@chromium.o=
rg" target=3D"_blank">chromium-...@chromium.org</a><br>
View archives, change email options, or unsubscribe: <br>
    <a href=3D"http://groups.google.com/a/chromium.org/group/chromium-dev" =
target=3D"_blank">http://groups.google.com/a/chromium.org/group/chromium-de=
v</a><br>
</div></div></blockquote></div><br>
</div>

--14dae9340fe73f5aec04c49682c7--
Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
Account Options
