967364 - Change already_AddRefed semantics to make it harder to misuse.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Misusing already_AddRefed can cause serious leaks, like those in bug 965498.  Let's make it harder.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Rename already_AddRefed::get to already_AddRefed::takeOwnership

Let's give this function a more descriptive name.  In part 5 we will make it actually steal ownership.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Fix already_AddRefed handling in getUserMedia code.

I don't even know what to say about this it's so crazy.  Please reassign to your minions as necessary.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 3: Make already_AddRefed::mRawPtr private

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8369849 [details] [diff] [review]
Part 2: Fix already_AddRefed handling in getUserMedia code.

Passing this on to Jesup

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 4: takeOwnership from every already_AddRefed

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.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 5: Make takeOwnership actually steal and assert that all already_AddRefeds have their pointers taken

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.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, because of the copy constructor.  Read comment 6?

Comment 10

[Nathan Froyd [:froydnj]]

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?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

My goal was not to fix all of it.  I only tried to fix the stuff that touched mRawPtr directly, which blocker part 3.

Comment 12

[Benjamin Smedberg]

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 13

[Benjamin Smedberg]

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()

Comment 14

[Benjamin Smedberg]

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 15

[Benjamin Smedberg]

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?

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 17

[Benjamin Smedberg]

Yeah, I think that since we are in fact using move semantics, Base(Move(aNodeInfo)) is the correct thing to do.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 5: Use every already_AddRefed

I added an operator<< overload for unused_t.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 4: Pass already_AddRefed by reference

This is purely mechanical.  Most of it is just twiddling how we pass already_AddRefed<nsINodeInfo> through the hierarchy of node constructors.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 4.1: Pass every already_AddRefed by reference

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.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32f48d6d3389
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d78ec7d6f0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/49dfebffbf8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff034c181ae4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a07dde918187

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/32f48d6d3389
https://hg.mozilla.org/mozilla-central/rev/4d78ec7d6f0d
https://hg.mozilla.org/mozilla-central/rev/49dfebffbf8e
https://hg.mozilla.org/mozilla-central/rev/ff034c181ae4
https://hg.mozilla.org/mozilla-central/rev/a07dde918187

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And an attempt to make comm-central build: https://hg.mozilla.org/comm-central/rev/e67da9f094ff

Comment 24

[Philip Chee]

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

Still broken.

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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.