Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Tue, Jul 7, 2015 at 6:10 PM, Michael Layzell mich...@thelayzells.com wrote: No, I never checked if it happens on an optimized build, but as C++ follows an as-if principal, which means that code has to execute as if those temporaries had been created. Unfortunately, AddRef and Release are virtual, which, I'm pretty sure, means that the compiler can't optimize them out as they may have arbitrary side effects :(. So the temporaries are probably eliminated, but the calls to AddRef and Release are probably not. I could be wrong on this though. This is true except in special cases where copy elision is allowed, in which case the temporary may be removed entirely, with its constructor and destructor. According to 12.8.32 of http://www.open-std.org/Jtc1/sc22/wg21/docs/papers/2011/n3242.pdf, one of those cases is when a temporary class object that has not been bound to a reference (12.2) would be copied/moved to a class object with the same cv-unqualified type, the copy/move operation can be omitted by constructing the temporary object directly into the target of the omitted copy/move I'm not an expert in C++-ese, but this looks like exactly that, so compilers should be allowed to remove the AddRef/Release. Whether they do or not is a separate question -- I'd hope so, in such a simple-looking case. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
I brought this up on IRC, but I think this is worth taking to the list: Replacing TemporaryRef with already_AddRefed has already caused at least one leak because already_AddRefed does not call Release() on the pointer it holds if nothing takes ownership of that pointer before its destructor runs. TemporaryRef did do that, and this seems like a strictly safer approach. My question is: why doesn’t already_AddRefed call Release() in its destructor? The fact that it doesn’t seems to me to be a profoundly unintuitive footgun. I don’t think we can trust reviewers to catch this, either. The fact that Foo() is declared as: already_AddRefedBar Foo(); won’t necessarily be obvious from a call site, where the reviewer will just see: Foo(); That call will leak, and if we don’t happen to hit that code path in our tests, we may not find out about it until after shipping it. I’d prefer that already_AddRefed just call Release(), but if there are performance arguments against that, then we should be checking for this pattern with static analysis. I don’t think the situation as it stands should be allowed to remain for long. - Seth On Jun 30, 2015, at 12:02 PM, Nathan Froyd nfr...@mozilla.com wrote: Bug 1161627 has landed on inbound, which converts all uses of mozilla::TemporaryRef to already_AddRefed and removes TemporaryRef. (already_AddRefed moved to MFBT several months ago, in case you were wondering about the spreading of XPCOM concepts.) TemporaryRef was added for easier porting of code from other engines, but as it has not been used much for that purpose and it led to somewhat less efficient code in places, it was deemed a failed experiment. -Nathan ___ 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
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Tue, Jul 7, 2015 at 9:59 AM, Seth Fowler mfow...@mozilla.com wrote: I brought this up on IRC, but I think this is worth taking to the list: Replacing TemporaryRef with already_AddRefed has already caused at least one leak because already_AddRefed does not call Release() on the pointer it holds if nothing takes ownership of that pointer before its destructor runs. TemporaryRef did do that, and this seems like a strictly safer approach. My question is: why doesn’t already_AddRefed call Release() in its destructor? The fact that it doesn’t seems to me to be a profoundly unintuitive footgun. We have a version of already_AddRefed that automatically releases the value it holds when it's destroyed; it's called nsCOMPtr. I don’t think we can trust reviewers to catch this, either. The fact that Foo() is declared as: already_AddRefedBar Foo(); won’t necessarily be obvious from a call site, where the reviewer will just see: Foo(); Foo should be MOZ_WARN_UNUSED_RESULT (or whatever that annotation is) then, as should anything that returns an already_AddRefed. That call will leak, and if we don’t happen to hit that code path in our tests, we may not find out about it until after shipping it. Ensuring that code that is untested works is in general a hard thing to do. I'd prefer that people wrote tests. (yes, there may be edge cases in which this isn't possible) I’d prefer that already_AddRefed just call Release(), but if there are performance arguments against that, then we should be checking for this pattern with static analysis. I don’t think the situation as it stands should be allowed to remain for long. There is currently a dynamic analysis (in the form of a fatal assertion) in the already_AddRefed dtor that I added last year. - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
We're already working on a static analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=1180993 which will prevent code like that from passing try. Hopefully that will help with your concerns. On 2015-07-07 12:59 PM, Seth Fowler wrote: I brought this up on IRC, but I think this is worth taking to the list: Replacing TemporaryRef with already_AddRefed has already caused at least one leak because already_AddRefed does not call Release() on the pointer it holds if nothing takes ownership of that pointer before its destructor runs. TemporaryRef did do that, and this seems like a strictly safer approach. My question is: why doesn’t already_AddRefed call Release() in its destructor? The fact that it doesn’t seems to me to be a profoundly unintuitive footgun. I don’t think we can trust reviewers to catch this, either. The fact that Foo() is declared as: already_AddRefedBar Foo(); won’t necessarily be obvious from a call site, where the reviewer will just see: Foo(); That call will leak, and if we don’t happen to hit that code path in our tests, we may not find out about it until after shipping it. I’d prefer that already_AddRefed just call Release(), but if there are performance arguments against that, then we should be checking for this pattern with static analysis. I don’t think the situation as it stands should be allowed to remain for long. - Seth On Jun 30, 2015, at 12:02 PM, Nathan Froyd nfr...@mozilla.com wrote: Bug 1161627 has landed on inbound, which converts all uses of mozilla::TemporaryRef to already_AddRefed and removes TemporaryRef. (already_AddRefed moved to MFBT several months ago, in case you were wondering about the spreading of XPCOM concepts.) TemporaryRef was added for easier porting of code from other engines, but as it has not been used much for that purpose and it led to somewhat less efficient code in places, it was deemed a failed experiment. -Nathan ___ 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
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On 2015-07-07 6:37 AM, Aryeh Gregor wrote: Did you check whether this actually occurs in an optimized build? C++ allows temporaries to be optimized away under some circumstances, e.g., when returning a local variable. It would make a lot of sense to me if it allowed the temporary created by a ternary operator to be optimized away. No, I never checked if it happens on an optimized build, but as C++ follows an as-if principal, which means that code has to execute as if those temporaries had been created. Unfortunately, AddRef and Release are virtual, which, I'm pretty sure, means that the compiler can't optimize them out as they may have arbitrary side effects :(. So the temporaries are probably eliminated, but the calls to AddRef and Release are probably not. I could be wrong on this though. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Fri, Jul 3, 2015 at 6:22 PM, Michael Layzell mich...@thelayzells.com wrote: So the ternary actually causes an unnecessary AddRef/Release pair, neat. Did you check whether this actually occurs in an optimized build? C++ allows temporaries to be optimized away under some circumstances, e.g., when returning a local variable. It would make a lot of sense to me if it allowed the temporary created by a ternary operator to be optimized away. The problem here appears to be that when deciding the type of the ternary, c++ chooses nsRefPtrFoo, rather than Foo*. Adding the get() makes C++ choose the correct type for the ternary, and avoids the cast of the rvalue reference. I think it's pretty clear that nsRefPtrFoo must be the type of the ternary expression. I would be rather surprised if the type of the ternary depended on what you did with the result! ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Fri, Jul 3, 2015 at 3:56 PM, Neil n...@parkwaycc.co.uk wrote: Aryeh Gregor wrote: we still want a new type for function parameters that accepts implicit conversions from nsRefPtr/nsCOMPtr, to use instead of raw pointers. Sure, but that won't stop someone from writing ArgFoo foo = ReturnFoo2(); will it? No, but that's an extremely obvious bug that should be caught in review. If you want to be sure, you could do static analysis of some kind, but I don't expect it's necessary. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Fri, Jul 3, 2015 at 8:56 AM, Neil n...@parkwaycc.co.uk wrote: Sure, but that won't stop someone from writing ArgFoo foo = ReturnFoo2(); will it? Something like that is fairly trivial for a static analysis system like our clang plugin to catch, if we want to create a type like that. Restricting certain types to only exist in temporaries and parameter declarations would not be too hard. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
Nathan Froyd wrote: I guess we could fix this in current m-c to be smarter about the parameter passing, perhaps by declaring that ParameterStoragensCOMPtrT is StoreCopyPassByConstLRefnsCOMPtrT? (Similarly for nsRefPtrT.) So what's StorensRefPtrPassByPtr for? -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
Aryeh Gregor wrote: we still want a new type for function parameters that accepts implicit conversions from nsRefPtr/nsCOMPtr, to use instead of raw pointers. Sure, but that won't stop someone from writing ArgFoo foo = ReturnFoo2(); will it? -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Thu, Jul 2, 2015 at 3:50 PM, Neil n...@parkwaycc.co.uk wrote:
Would you mind reminding me what the failure case this avoids is?
already_AddRefedFoo
ReturnFoo1()
{
nsRefPtrFoo foo = new Foo();
return foo.forget();
}
nsRefPtrFoo
ReturnFoo2()
{
return new Foo();
}
// This doesn't compile
Foo* foo = ReturnFoo1();
// This happily compiles and causes use-after-free
Foo* foo = ReturnFoo2();
Naturally, the hard-to-catch case is when the function returns
something that usually has a refcount above one, so it works fine, but
sometimes will return something with a refcount of exactly one, which
causes a sec-critical arbitrary code execution on Firefox stable.
It's worth pointing out that if we only remove the implicit conversion
to T* for rvalues, you still won't be able to directly pass the
returned value to a function that wants T*, even though this is
perfectly safe (because the destructor for the temporary won't be
called until the function call returns, IIUC). So we still want a new
type for function parameters that accepts implicit conversions from
nsRefPtr/nsCOMPtr, to use instead of raw pointers. But you can't pass
an already_AddRefed directly to such a function right now anyway, so
this isn't actually a disadvantage relative to the status quo.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Thu, Jul 2, 2015 at 9:21 AM, Neil n...@parkwaycc.co.uk wrote: Nathan Froyd wrote: I tried this, fixed a few compilation errors, then decided this wasn't worth it just yet and threw my work into a bug. Comments appreciated: https://bugzilla.mozilla.org/show_bug.cgi?id=1179451 1. It's because QueryInterface has to addref, so we can't directly pass the returned pointer. (But otherwise it's effectively the same as 4.) Sure. This also comes up when passing the results of nsCOMPtrT-returning functions to other things, or returning them as raw pointers. This might point to a deficiency in APIs, of course. 2. Ugh, standard MSVC optimisation doesn't compile an nsCOMPtr in a ternary at all well. (If my x86_64 is up to scratch then I think it creates an internal flag so it remembers which arm of the ternary was constructed.) Hooray for forcing people to write the efficient code, then? :) 3. Why isn't the const nsCOMPtrT converting to a T*? (There's too much indirection for my liking, so I can't work out which storage class is getting applied to the argument.) Because for something like: nsCOMPtrnsIRunnable event = NS_NewRunnableMethodWithArgnsCOMPtrnsIDOMHTMLInputElement (this, nsFormFillController::MaybeStartControllingInput, focusedInput); the nsCOMPtrnsIDOMHTMLInputElement gets the storage class StoreCopyPassByValuensCOMPtrT, which means that the argument gets passed as a (copied!) nsCOMPtr. And that's a temporary, so the implicit conversion to T* is disallowed. I guess we could fix this in current m-c to be smarter about the parameter passing, perhaps by declaring that ParameterStoragensCOMPtrT is StoreCopyPassByConstLRefnsCOMPtrT? (Similarly for nsRefPtrT.) If people want something else, they can explicitly ask for it. 4. Maybe I'm confused, but isn't this the pattern we're trying to avoid so that we can remove already_AddRefed and use moves instead? That's a good point, I didn't consider that when just trying to make things compile. In the particular case I remember fixing (calling nsIDocument::GetBaseURI), the result was stored as a T* (!), so we would have had to construct something explicitly anyway. -Nathan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
Nathan Froyd wrote: I tried this, fixed a few compilation errors, then decided this wasn't worth it just yet and threw my work into a bug. Comments appreciated: https://bugzilla.mozilla.org/show_bug.cgi?id=1179451 1. It's because QueryInterface has to addref, so we can't directly pass the returned pointer. (But otherwise it's effectively the same as 4.) 2. Ugh, standard MSVC optimisation doesn't compile an nsCOMPtr in a ternary at all well. (If my x86_64 is up to scratch then I think it creates an internal flag so it remembers which arm of the ternary was constructed.) 3. Why isn't the const nsCOMPtrT converting to a T*? (There's too much indirection for my liking, so I can't work out which storage class is getting applied to the argument.) 4. Maybe I'm confused, but isn't this the pattern we're trying to avoid so that we can remove already_AddRefed and use moves instead? -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
Jeff Muizelaar wrote: I believe this is predicated on removing the implicit conversion from nsRefPtrT to T* Would you mind reminding me what the failure case this avoids is? -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Wed, Jul 1, 2015 at 6:24 AM, Joshua Cranmer pidgeo...@gmail.com wrote: You'd get the same benefit, I think, by making operator T*() = delete;, syntax which is accepted on gcc 4.8.1+, clang, and MSVC 2015 IIRC. I once tried this and found it had problematic side effects that made deploying it difficult in practice. I think one involved the ternary operator. Unfortunately, I can't remember exactly what they were. :( Anyone who's interested can try for themselves by just adding that line to nsCOMPtr.h, recompiling, and seeing what breaks. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
On Wed, Jul 1, 2015 at 7:39 AM, Aryeh Gregor a...@aryeh.name wrote: On Wed, Jul 1, 2015 at 6:24 AM, Joshua Cranmer pidgeo...@gmail.com wrote: You'd get the same benefit, I think, by making operator T*() = delete;, syntax which is accepted on gcc 4.8.1+, clang, and MSVC 2015 IIRC. I once tried this and found it had problematic side effects that made deploying it difficult in practice. I think one involved the ternary operator. Unfortunately, I can't remember exactly what they were. :( Anyone who's interested can try for themselves by just adding that line to nsCOMPtr.h, recompiling, and seeing what breaks. I tried this, fixed a few compilation errors, then decided this wasn't worth it just yet and threw my work into a bug. Comments appreciated: https://bugzilla.mozilla.org/show_bug.cgi?id=1179451 -Nathan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::TemporaryRef is gone; please use already_AddRefed
Will it ever be possible to eliminate TemporaryRef and use moves instead? Rob -- oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo owohooo osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o oioso oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo owohooo osoaoyoso,o o‘oYooouo ofolo!o’o owoiololo oboeo oiono odoaonogoeoro ooofo otohoeo ofoioroeo ooofo ohoeololo. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
