Change already_AddRefed semantics to make it harder to misuse.

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(6 attachments, 2 obsolete attachments)

154.11 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
22.31 KB, patch
jesup
: review+
froydnj
: feedback-
Details | Diff | Splinter Review
5.40 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
11.10 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
226.96 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
9.89 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Misusing already_AddRefed can cause serious leaks, like those in bug 965498.  Let's make it harder.
Let's give this function a more descriptive name.  In part 5 we will make it actually steal ownership.
Attachment #8369846 - Flags: review?(benjamin)
I don't even know what to say about this it's so crazy.  Please reassign to your minions as necessary.
Attachment #8369849 - Flags: review?(ekr)
Comment on attachment 8369849 [details] [diff] [review]
Part 2: Fix already_AddRefed handling in getUserMedia code.

Passing this on to Jesup
Attachment #8369849 - Flags: review?(ekr) → review?(rjesup)
I'm still waiting on full try results, but this is enough to get the entire test suite to pass on Linux.  There may be a few stragglers that we need to add.
Attachment #8369851 - Flags: review?(benjamin)
And the fun part.

The one bit that's slightly tricky here is the copy constructor.  One problem that arises is that of temporaries.  We either have to avoid creating temporaries or steal their pointers.  Because using r-value references requires doing Move(foo) to pass an already_AddRefed<T>&& along, and the fact that const semantics don't really make any sense on already_AddRefed anyways, I chose to just cast away the const and steal the underlying pointer.
Attachment #8369852 - Flags: review?(benjamin)
Whiteboard: [MemShrink]
Attachment #8369849 - Flags: review?(rjesup) → review+
Comment on attachment 8369852 [details] [diff] [review]
Part 5: Make takeOwnership actually steal and assert that all already_AddRefeds have their pointers taken

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

::: xpcom/glue/nsCOMPtr.h
@@ +177,5 @@
> +      MOZ_ASSERT(!mRawPtr);
> +    }
> +
> +    MOZ_WARN_UNUSED_RESULT
> +    T* takeOwnership() const

Does this need to be const?
Yes, because of the copy constructor.  Read comment 6?
Whiteboard: [MemShrink] → [MemShrink:P2]
Blocks: 968799
Comment on attachment 8369849 [details] [diff] [review]
Part 2: Fix already_AddRefed handling in getUserMedia code.

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

You missed a couple of places:

GetUserMediaDevicesRunnable: http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1090
DeviceSuccessCallbackRunnable: http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#273

Maybe we should have some sort of base class here for mSuccess and mError?
Attachment #8369849 - Flags: feedback-
My goal was not to fix all of it.  I only tried to fix the stuff that touched mRawPtr directly, which blocker part 3.
Blocks: 968836
I have a slight preference for .take() or .own() over .takeOwnership(). Probably not enough to insist on it, but the verbosity seems a little much.
Comment on attachment 8369846 [details] [diff] [review]
Part 1: Rename already_AddRefed::get to already_AddRefed::takeOwnership

Marking r+, but please consider the name .take()
Attachment #8369846 - Flags: review?(benjamin) → review+

Updated

5 years ago
Attachment #8369850 - Flags: review?(benjamin) → review+
Comment on attachment 8369851 [details] [diff] [review]
Part 4: takeOwnership from every already_AddRefed

This feels ugly and verbose. Is there a way to have a .forget() variant that can just be:

unused << comptr.forget();

?
Comment on attachment 8369852 [details] [diff] [review]
Part 5: Make takeOwnership actually steal and assert that all already_AddRefeds have their pointers taken

I must admit I didn't quite understand the discussion of copy/move constructors in the comments so far. It appears to me that already_AddRefed should not have a copy constructor. It should have a move constructor and takeOwnership shouldn't be const.

What combination of C++ magic can end us up with copying an already_AddRefed being illegal and only move-construction is legal?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> Comment on attachment 8369852 [details] [diff] [review]
> Part 5: Make takeOwnership actually steal and assert that all
> already_AddRefeds have their pointers taken
> 
> I must admit I didn't quite understand the discussion of copy/move
> constructors in the comments so far. It appears to me that already_AddRefed
> should not have a copy constructor. It should have a move constructor and
> takeOwnership shouldn't be const.
> 
> What combination of C++ magic can end us up with copying an already_AddRefed
> being illegal and only move-construction is legal?

The issue is that you would have to use Move explicitly to pass on already_AddRefed parameters, so every node constructor would be something like

Derived(already_AddRefed<nsINodeInfo>&& aNodeInfo)
 : Base(Move(aNodeInfo))
{}

which I think is annoying.  We could do that though, and disallow the copy ctor.  And new Base(foo.forget()) would work just fine.
Yeah, I think that since we are in fact using move semantics, Base(Move(aNodeInfo)) is the correct thing to do.

Updated

5 years ago
Attachment #8369852 - Flags: review?(benjamin) → review-

Updated

5 years ago
Attachment #8369851 - Flags: review?(benjamin) → review-
I added an operator<< overload for unused_t.
Attachment #8369851 - Attachment is obsolete: true
Attachment #8384055 - Flags: review?(benjamin)
This is purely mechanical.  Most of it is just twiddling how we pass already_AddRefed<nsINodeInfo> through the hierarchy of node constructors.
Attachment #8384058 - Flags: review?(benjamin)
Attachment #8384055 - Attachment description: Part 4: Use every already_AddRefed → Part 5: Use every already_AddRefed
These are the non-mechanical changes.  There are two:

nsTArray::AppendElement does not accept r-value references.  And nsCOMPtr no longer binds to const already_AddRefed<T>&.  So until nsTArray is modified to accept r-value references we have to make sure to pass in an l-value.

already_AddRefed<T>&/&& cannot be implicitly constructed from nullptr.  Rather than write already_AddRefed<T>(nullptr) I rewrote the code a little bit to avoid having to explicitly pass nullptr.

Parts 4 and 4.1 will be squashed when I check in.
Attachment #8369852 - Attachment is obsolete: true
Attachment #8384060 - Flags: review?(benjamin)

Updated

5 years ago
Attachment #8384055 - Flags: review?(benjamin) → review+

Updated

5 years ago
Attachment #8384058 - Flags: review?(benjamin) → review+

Updated

5 years ago
Attachment #8384060 - Flags: review?(benjamin) → review+

Updated

5 years ago
Depends on: 984064

Comment 24

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #23)
> And an attempt to make comm-central build:
> https://hg.mozilla.org/comm-central/rev/e67da9f094ff

Still broken.

Updated

5 years ago
Depends on: 984110
https://hg.mozilla.org/mozilla-central/rev/ff034c181ae4#l9.39

It looks like a leak to me if we just ignore the result of .forget(). Did I miss something?

E.g.

  struct MyFoo
  {
    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MyFoo)
    MyFoo()
    {
      printf("MyFoo ctor.\n");
    }
  private:
    ~MyFoo()
    {
      printf("MyFoo dtor.\n");
    }
  };

  RefPtr<MyFoo> f = new MyFoo;
  mozilla::Unused << f.forget();

will leak the object.
Flags: needinfo?(benjamin)
The comments there say the "update" will release itself.

But you're right that you _can_ still leak if you try.  But you have to try, complete with mozilla::Unused, etc.

Updated

2 years ago
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.