Open Bug 1194215 Opened 4 years ago Updated 2 years ago

Allow temporary smart pointers to be passed to functions without explicit casts

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

People

(Reporter: ayg, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

As of bug 1179451, temporary nsRefPtrs no longer get converted implicitly to raw pointers.  Bug 1193298 and bug 1193762 extend the treatment to RefPtr and nsCOMPtr.  This is meant to protect against incorrectly assigning a smart pointer that's about to be destroyed to a local variable that won't hold a reference, like

  // nsRefPtr<T> GetT();
  T* t = GetT();

which will make the pointer immediately invalid if it happened to have a refcount of one.

This has the annoying side effect that temporary smart pointers (including the result of the ternary operator) can no longer be passed to functions, since by convention, functions that take refcounted objects use raw pointers for the parameter type.  This is collateral damage -- a smart pointer passed to a function will never have its destructor called until after the function returns, so this conversion is no less safe than any conversion to a raw pointer.

This isn't a huge deal today, because conventionally, functions return raw pointers instead of smart pointers.  But in the cases where it crops up, the obvious workaround (.get()) is not so nice, because it looks unsafe when really it isn't, which desensitizes people to the risk of .get().

Possibly more importantly, there's been talk in the past about having more functions return smart pointers instead of raw pointers, and getting rid of implicit conversion to raw pointers even for lvalues (bug 1193762).  If either of these happens, the status quo will be entirely unreasonable.  Now that bug 1179451 is fixed, I see no reason at all why functions shouldn't return nsRefPtr instead of raw pointers routinely, especially if we make sure there are good move constructors/assignment operators.  (I'm not talking about changing functions that currently return already_AddRefed.)

In practice, if we outlawed functions returning smart pointers, and we aren't planning to remove implicit conversion to T* for lvalues anytime soon, I don't think RawParam is worth saving the occasional .get() in a ternary operator, which saves you an addref/release anyway.


However, I think it would be better to encourage people to write functions that return smart pointers instead of raw pointers, because it saves you from the same use-after-free addressed by bug 1179451:

  // void DoStuff(T*);
  // T* GetT();
  DoStuff(GetT());

Nothing is guaranteed to hold a strong reference inside DoStuff(), so this will be a use-after-free if DoStuff() releases its parameter by mistake.  DoStuff should be changed to take a RefParam<T> instead of T*, but realistically practically none of our functions do, and it's not always trivial to port them, because you have to update callers.  So it would be a good idea to insure against the bug in the return type as well.  If we write proper move constructors, returning a smart pointer will usually not add any addrefs/releases in cases where they're not really needed.

Returning smart pointers also helps against a similar use-after-free, which RefParam doesn't help with:

  // void T::DoStuff();
  // T* GetT();
  GetT()->DoStuff();

Changing GetT() to return a smart pointer will again save us from DoStuff() accidentally releasing |this|.  This won't help if you call it from a local variable, but then, local variables to ref-pointed types are normally smart pointers anyway.  In editor, there are some methods that have the incantation

  nsCOMPtr<nsIFoo*> kungFuDeathGrip(this);

at the beginning to prevent |this| from going away in the middle of the call.  If we rigorously stamped out all uses of T* that aren't strictly needed, this would cease to be an issue.  But at least in transition, RawParam is necessary.  If minimizing addref/release is an issue, it will always be necessary.
Attached patch Patch (obsolete) — Splinter Review
This consists basically of the latest version of bug 1179451 part 4 option 2 with requested revisions, with patches 9, 10, and 11 from bug 1193762 merged in, bug 1194195 subtracted from the diff, and bug 1179451 part 4 option 1 reverted (phew!).
Attachment #8647503 - Flags: review?(nfroyd)
Attached patch PatchSplinter Review
Better comments.
Attachment #8647503 - Attachment is obsolete: true
Attachment #8647503 - Flags: review?(nfroyd)
Attachment #8647521 - Flags: review?(nfroyd)
Flags: needinfo?(nfroyd)
Comment on attachment 8647521 [details] [diff] [review]
Patch

Review of attachment 8647521 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling for now given decisions on bug 1194195.
Attachment #8647521 - Flags: review?(nfroyd)
Flags: needinfo?(nfroyd)
Assignee: ayg → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.