It would appear to me that you really need to promote this to a strong
reference while you're using it.
Even `if (foo) foo->bar();` could theoretically have the value of `foo` change
between the branch and the deref.
Also, do we at least `MOZ_ASSERT(get())` in `operator->`? Yes, it will crash
immediately anyways, but it's always nicer to have something say 'Hey, you
tried to deref a null weak-ptr'.
-Jeff
----- Original Message -----
From: "David Major" <dma...@mozilla.com>
To: "Ehsan Akhgari" <ehsan.akhg...@gmail.com>
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:38:24 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
Does it? I don't think I'm any more or less likely to omit a validity check
using operator->() vs get(). Maybe it's just me.
It seems like get() might actually be *more* prone to failure. Imagine:
Class* object = foo.get();
if (object) {
object->Method();
}
// ... A lot of stuff happens and the ref blows up ...
if (object) {
object->Method(); // oops
}
----- Original Message -----
From: "Ehsan Akhgari" <ehsan.akhg...@gmail.com>
To: "David Major" <dma...@mozilla.com>
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:27:03 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:10 PM, David Major wrote:
> Isn't it ultimately up to the developer to get it right? Someone could just
> as well forget to use |if (object)| from your example.
>
> Here's a sample usage from the header file:
> * // Test a weak pointer for validity before using it.
> * if (weak) {
> * weak->num = 17;
> * weak->act();
> * }
Sure, but that "convenience" operator makes it natural to write
incorrect code by default. It's better to be explicit and correct, than
implicit and wrong. :-)
Cheers,
Ehsan
> ----- Original Message -----
> From: "Ehsan Akhgari" <ehsan.akhg...@gmail.com>
> To: dev-platform@lists.mozilla.org
> Sent: Tuesday, October 8, 2013 3:54:17 PM
> Subject: Audit your code if you use mozilla::WeakPtr
>
> I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
> usage unsafe by default. The issue is that WeakPtr provides convenience
> methods such as operator->, which mean that the consumer can directly
> dereference it without the required null checking first. This means that
> you can have code like the below:
>
> WeakPtr<Class> foo = realObject->asWeakPtr();
> // ...
> foo->Method();
>
> That will happily compile and will crash at runtime if the object behind
> the weak pointer is dead. The correct way of writing such code is:
>
> Class* object = foo.get();
> if (object) {
> object->Method();
> }
>
> I don't know enough about all of the places which use WeakPtr myself to fix
> them all, but if you have code using this in your module, please spend some
> time auditing the code, and fix it. Please file individual bugs for your
> components blocking bug 924658.
>
> Thanks!
> --
> Ehsan
> <http://ehsanakhgari.org/>
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
