WeakPtrFactory member variable order rule weakly followed

[Samuel Huang]

Hi,

  I noticed that weak_ptr.h, contains the following example (note the rule "Member variables should appear before WeakPtrFactory"):

//  class Controller {
//   public:
//    void SpawnWorker() { Worker::StartNew(weak_factory_.GetWeakPtr()); }
//    void WorkComplete(const Result& result) { ... }
//   private:
//    // Member variables should appear before the WeakPtrFactory, to ensure
//    // that any WeakPtrs to Controller are invalidated before its members
//    // variable's destructors are executed, rendering them invalid.
//    WeakPtrFactory<Controller> weak_factory_;
//  };

  A quick sampling through the repository shows that this rule is not always followed. Should we :

- Update the comment, since it's doesn't matter?  (and/or/xor)

- Have a pre-submit check to ensure new code follow this?

- See if any crash arise because of the rule being broken?

- Comb through the code to detect / enforce this rule?

- Add compile-time (pointer arithmetic?) check in WeakPtr to ensure that it appears last, or near the end?

  Thanks!

--

Samuel Huang

[Rouslan Solomakhin]

On Tue, Oct 1, 2013 at 7:44 AM, Samuel Huang <hua...@chromium.org> wrote:

- Comb through the code to detect / enforce this rule?

How about combing through the code and filing bugs against the offenders? 

[Rachel Blum]

This occasionally happens because people know too much about lifetimes and hence know WeakPtr's will not be around at destruction time. So while it's technically not valid, there are instances where it's safe.

- Update the comment, since it's doesn't matter?  (and/or/xor)

It _does_ matter, so removing it is not a good idea.

 

- Have a pre-submit check to ensure new code follow this?

AFAIK, we're trying to not get overboard on presubmit checks. I wouldn't personally object, but you might want to listen to the presubmit gods. Or OWNERS ;)

 

- See if any crash arise because of the rule being broken?

 

If I understand WeakPtr correctly, adding a DCHECK(!HasWeakPtrs()) in the factory's dtor might work for that.
 

- Add compile-time (pointer arithmetic?) check in WeakPtr to ensure that it appears last, or near the end?

That'd be ugly. And wrong :) It's not guaranteed to be near the end. (See e.g. http://stackoverflow.com/questions/916600/can-a-c-compiler-re-order-elements-in-a-struct)

 - rachel

 

On Tue, Oct 1, 2013 at 7:44 AM, Samuel Huang <hua...@chromium.org> wrote:

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Wez]

On 1 October 2013 18:44, Rachel Blum <gr...@chromium.org> wrote:

This occasionally happens because people know too much about lifetimes and hence know WeakPtr's will not be around at destruction time. So while it's technically not valid, there are instances where it's safe.

- Update the comment, since it's doesn't matter?  (and/or/xor)

It _does_ matter, so removing it is not a good idea.

 

- Have a pre-submit check to ensure new code follow this?

AFAIK, we're trying to not get overboard on presubmit checks. I wouldn't personally object, but you might want to listen to the presubmit gods. Or OWNERS ;)

 

- See if any crash arise because of the rule being broken?

 

If I understand WeakPtr correctly, adding a DCHECK(!HasWeakPtrs()) in the factory's dtor might work for that.

One of the most common use-cases of WeakPtrFactory is to hand off WeakPtrs and let WeakPtrFactory's dtor invalidate them, so I expect you'd see DCHECK(HasWeakPtrs()) fire _a lot_ ;)

Crashes arising from bad ordering will manifest as use-after-dtor of members (note, not the same as use-after-free, though similar). They'll also have the containing class (the one the WeakPtr refers to)'s dtor on the stack.

However, as Rachel notes above, it's possible to get away with WeakPtrFactory being in the "wrong" place, just as race-conditions can exist in code without ever actually being hit.

[Wez]

WeakPtrFactory doesn't free anything - it just invalidates any WeakPtrs it was used to create.

To detect the use-after-dtor (again, note this is NOT use-after-free!) of members, you'd need to check all members for correctness every time they're accessed [via the WeakPtr].

On 3 October 2013 16:01, Samuel Huang <hua...@google.com> wrote:

Can we try to get the WrakPtrFactory destructor deadbeef the object that it frees, and make potential use-after-free bugs more visible?

--
Samuel Huang

[David Michael]

On Thu, Oct 3, 2013 at 9:10 AM, Wez <w...@chromium.org> wrote:

WeakPtrFactory doesn't free anything - it just invalidates any WeakPtrs it was used to create.

To detect the use-after-dtor (again, note this is NOT use-after-free!) of members, you'd need to check all members for correctness every time they're accessed [via the WeakPtr].

On 3 October 2013 16:01, Samuel Huang <hua...@google.com> wrote:

Can we try to get the WrakPtrFactory destructor deadbeef the object that it frees, and make potential use-after-free bugs more visible?

Hmm. I started playing with the following in WeakPtrFactory's constructor:

  explicit WeakPtrFactory(T* ptr) : ptr_(ptr) {

    DCHECK((reinterpret_cast<uintptr_t>(this) -

            reinterpret_cast<uintptr_t>(ptr) + sizeof(*this))

            == sizeof(*ptr));

  }

In english: the size of everything before the WeakPtrFactory member PLUS the size of the WeakPtrFactory should be equal to the total size of the object which owns the WeakPtrFactory. This is, of course, not safe in theory. As Rachel points out, members might not be layed out in the order they're written (even though initialization/destruction order is guaranteed). Furthermore, it might not be accurate due to alignment. Interestingly, it's working great so far locally, with no false positives.

But...  I realized while playing with this that a much safer & cleaner alternative would be an addition to the Clang style checker. I can look in to that.

[Wez]

Keep in mind that WeakPtrFactorys can refer to things other than the objects that contain them. :)

[David Michael]

On Thu, Oct 3, 2013 at 10:18 AM, Wez <w...@chromium.org> wrote:

Keep in mind that WeakPtrFactorys can refer to things other than the objects that contain them. :)

Yuck. I'd have to see some specific examples to be sure, but that sounds like something we shouldn't encourage (and maybe even should disallow). WeakPtrFactory loses all its usefulness if you don't guarantee that it gets destructed before its referee (or at least all other members of its referee). It's definitely possible to do that right, but we apparently (myself included) already mess up the easier WeakPtrFactory-as-member pattern already. I feel like if we can narrow it down to its 1 most common use case, and then guarantee (with a Clang checker) that we always get that pattern right, then we should do that.

A clang checker that accounts for only the factory-as-member case should be super easy. Dealing with other uses _statically_ might be hard or impossible.

[David Michael]

Filed: https://code.google.com/p/chromium/issues/detail?id=303818

[Chris Hopman]

One use for having a WeakPtrFactory refer to something other than the object that contains it would be for an object to supply weak pointers to some member it owns that does not on it's own supply weak pointers to itself. Then, you need to ensure that the WeakPtrFactory is declared after the member that it will be creating weak pointers to. This would be difficult to get right if ownership of that member is changed (e.g. member is created, member is destroyed, ownership passed to someone else) outside of the normal construction/destruction flow-- i.e. you would then want a scoped pointer to the factory so you could make sure the factory is in the correct state whenever the ownership changes.

I think if this is something that we actually do, maybe there should be something that handles this automatically (i.e. some smart pointer that provides weak pointers and invalidates them when the object is released).

[Rachel Blum]

On Thu, Oct 3, 2013 at 9:25 AM, David Michael <dmic...@chromium.org> wrote:

Yuck. I'd have to see some specific examples to be sure, but that sounds like something we shouldn't encourage (and maybe even should disallow).

IIRC that's intentional, so we can vend WeakPtrs on objects owned. See e.g. remoting/host/client_session.h. (I'm not sure that's the best design, but I do know too little about that code to be qualified to make any judgment). 

- rachel

 

[Wez]

On 3 October 2013 17:25, David Michael <dmic...@chromium.org> wrote:

On Thu, Oct 3, 2013 at 10:18 AM, Wez <w...@chromium.org> wrote:

Keep in mind that WeakPtrFactorys can refer to things other than the objects that contain them. :)

Yuck. I'd have to see some specific examples to be sure, but that sounds like something we shouldn't encourage (and maybe even should disallow). WeakPtrFactory loses all its usefulness if you don't guarantee that it gets destructed before its referee (or at least all other members of its referee).

It's actually a pretty useful construct, where you have:

- Object A that owns Object B.

- B has no reason to SupportsWeakPtr.

- A wants to pass Callbacks to other components that Bind() to methods on B.

- A wants to have those Callbacks become no-ops if they are executed after B has been deleted.

A then contains a WeakPtrFactory<Class B> that points to B, and Bind()s WeakPtrs from that in Callbacks it passes to other components, thereby avoiding polluting Class B with knowledge of [Supports]WeakPtr.

 

It's definitely possible to do that right, but we apparently (myself included) already mess up the easier WeakPtrFactory-as-member pattern already. I feel like if we can narrow it down to its 1 most common use case, and then guarantee (with a Clang checker) that we always get that pattern right, then we should do that.

A clang checker that accounts for only the factory-as-member case should be super easy. Dealing with other uses _statically_ might be hard or impossible.

We can cope with the factory-as-member easily, yes. We should also be able to sanity-check the scoped_ptr<ClassB> + WeakPtrFactory<ClassB> case above reasonably easily.

Related to this, adding a check that classes deriving from SupportsWeakPtr always call Invalidate() in the derived class dtor should be easy, and would trap that potential source of use-after-dtor on members. :)

[David Michael]

I have a simple Clang checker now that can find these:

https://codereview.chromium.org/26303002/

Before I can turn it on by default, I (we?) have to fix about 250 places where we have WeakPtrFactory in the wrong spot. If you want to help out, you can fix any WeakPtrFactory members for your favorite part of Chromium. I listed them in this spreadsheet (accessible from your chromium account):

https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AgmkMxnFvhjmdGFJcDlVbFZyMlFFY2h4bVNscEZOQkE#gid=0

Note, there are probably some cases where we have 2 or more WeakPtrFactory<>s at the end of the class. I believe that's safe, but I will have to update my checker to account for that. You might encounter some of these benign cases in the spreadsheet.

On Fri, Oct 4, 2013 at 7:02 AM, Wez <w...@chromium.org> wrote:

On 3 October 2013 17:25, David Michael <dmic...@chromium.org> wrote:

On Thu, Oct 3, 2013 at 10:18 AM, Wez <w...@chromium.org> wrote:

Keep in mind that WeakPtrFactorys can refer to things other than the objects that contain them. :)

Yuck. I'd have to see some specific examples to be sure, but that sounds like something we shouldn't encourage (and maybe even should disallow). WeakPtrFactory loses all its usefulness if you don't guarantee that it gets destructed before its referee (or at least all other members of its referee).

It's actually a pretty useful construct, where you have:

- Object A that owns Object B.

- B has no reason to SupportsWeakPtr.

- A wants to pass Callbacks to other components that Bind() to methods on B.

- A wants to have those Callbacks become no-ops if they are executed after B has been deleted.

A then contains a WeakPtrFactory<Class B> that points to B, and Bind()s WeakPtrs from that in Callbacks it passes to other components, thereby avoiding polluting Class B with knowledge of [Supports]WeakPtr.

 

It's definitely possible to do that right, but we apparently (myself included) already mess up the easier WeakPtrFactory-as-member pattern already. I feel like if we can narrow it down to its 1 most common use case, and then guarantee (with a Clang checker) that we always get that pattern right, then we should do that.

A clang checker that accounts for only the factory-as-member case should be super easy. Dealing with other uses _statically_ might be hard or impossible.

We can cope with the factory-as-member easily, yes. We should also be able to sanity-check the scoped_ptr<ClassB> + WeakPtrFactory<ClassB> case above reasonably easily.

I'm ignoring that case for now :-). There are enough of the "easy-to-check-for" cases wrong to deal with, that I'm focused on that for now.

I did a quick Clang check and found 22 total examples of having a "WeakPtrFactory<Foo> foo_" member, where Foo is not the owner of m_foo.

- 8 of those are from "remoting/", 2 of those are unit tests ;-)

- 9 of the rest are unit tests.

So it appears to not be a very common pattern. It might not be worth the trouble to check for it. Or, it might be worth taking the time to figure out an alternative way for those rare cases, and disallow the pattern entirely. I haven't looked to see how many of them are done correctly.

I stuck those on another tab in that spreadsheet, for the curious:
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AgmkMxnFvhjmdGFJcDlVbFZyMlFFY2h4bVNscEZOQkE#gid=1

BTW, to do the check properly in Clang, I would have to look at the actual initializer lists to make sure the WeakPtrFactory<> member referred to a member that was listed before it. Although maybe we could restrict the pattern so that the WeakPtrFactory<> has to appear immediately after the member it refers to, and just check that the type on which the WeakPtrFactory was specialized matches the type of the WeakPtrFactory's predecessor (allowing for ownership as a scoped_ptr too) in the outer class definition. That wouldn't be a perfect check, but might be good enough.

 

Related to this, adding a check that classes deriving from SupportsWeakPtr always call Invalidate() in the derived class dtor should be easy, and would trap that potential source of use-after-dtor on members. :)

I have my hands full ATM, but that sounds like another good idea if anybody else wants to tackle it.

[Chris Hopman]

It would also need to check that the member is never created, destroyed, or has its ownership passed outside of the containing objects construction/destruction.

[Chris Hopman]

Or rather, if any of that happens, it would need to check that the factory were reset correctly in those cases.

[Wez]

I've been moving some of these WeakPtrFactory instances and, alarmingly, it seems that GCC, at least, has no problems whatsoever with code like:

class Foo {

 public:

  Foo() : weak_ptr_(weak_ptr_factory_.GetWeakPtr()),

             weak_ptr_factory_(this) {

  }

 private:

  ...

};

Without the compiler providing the basic sanity-check that |weak_ptr_factory_| is initialized before the caller tries to create a WeakPtr from it, these patches appear to introduce more scope for bugs than they fix. :(

Wez

[Wez]

I think this is best handled via a dedicated helper combining scoped_ptr-like and WeakPtrFactory functions.

[Nico Weber]

On Sat, Nov 9, 2013 at 10:45 AM, Wez <w...@chromium.org> wrote:

I've been moving some of these WeakPtrFactory instances and, alarmingly, it seems that GCC, at least, has no problems whatsoever with code like:

class Foo {

 public:

  Foo() : weak_ptr_(weak_ptr_factory_.GetWeakPtr()),

             weak_ptr_factory_(this) {

  }

 private:

  ...

};

Without the compiler providing the basic sanity-check that |weak_ptr_factory_| is initialized before the caller tries to create a WeakPtr from it, these patches appear to introduce more scope for bugs than they fix. :(

clang warns on this, at least in some cases: https://codereview.chromium.org/26775010/