968836 - fix remaining already_AddRefed members in MediaManager.cpp after bug 967364

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: fix remaining already_AddRefed members in MediaManager.cpp after bug 967364

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Why we want to do this? Using already_AddRefed as member variables tells clearly that the
object is addrefed, but makes sure we don't release it accidentally in a wrong thread.

Comment 3

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

The only reason to use already_AddRefed instead of nsCOMPtr is if you're leaking.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

leaking is less bad than releasing on a wrong thread.
Also, using already_AddRefed is self-documenting.

Comment 5

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #4)
> leaking is less bad than releasing on a wrong thread.
> Also, using already_AddRefed is self-documenting.

We don't need to make a choice between the two.  We can write the code properly so that we don't leak and we release on the correct thread.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Sure, and Already_AddRefed is just a fine way to write such code. And it makes the code less risky too.
And self-documenting is a plus.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8371517 [details] [diff] [review]
fix remaining already_AddRefed members in MediaManager.cpp after bug 967364

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

r+, though I have similar comments to smaug's

::: dom/media/MediaManager.cpp
@@ +1,1 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */

tab-width 40??

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Olli Pettay [:smaug] from comment #6)
> Sure, and Already_AddRefed is just a fine way to write such code. And it
> makes the code less risky too.
> And self-documenting is a plus.

It is not clear to me that something like:

class MyRunnable : public nsRunnable
{
   MyRunnable(already_AddRefed<Thing> aThing) : mThing(aThing) {}
   NS_IMETHOD Run()
   {
      nsRefPtr<Thing> thing(mThing);
      ...
   }
   already_AddRefed<Thing> mThing;
};

is any clearer, safer, or more self-documenting than:

class MyRunnable : public nsRunnable
{
   MyRunnable(already_AddRefed<Thing> aThing) : mThing(aThing) {}
   NS_IMETHOD Run()
   {
      ...
   }
   nsRefPtr<Thing> mThing;
};

(The latter is even shorter!)

The patch in this bug and Kyle's patch in the latter bug convert the former into the latter.  I think the latter is an improvement.  With the former, it is all too easy to forget to root the already_AddRefed during Run() and leak; with the latter, constructors and destructors handle it for you automatically.  And already_AddRefed isn't a guarantee that "I have the last ref and releasing the ref will destroy this thing", which is what it sounds like you are saying is the self-documentation bit of the former.

I think of already_AddRefed as a temporary thing only, meant to show how refs flow from one holder to another; they're not meant to be long-lived things.  Disallowing them as members (and as global variables!) is a way to enforce that.

Runnables are the only place where already_AddRefed is ever used as a member.  I don't think that requiring runnables to use strong pointers to show ownership is any sort of hardship or code obfuscation.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Leak is less bad than releasing on a wrong thread. leak vs. security bug.

And I'm not suggesting that already_AddRefed has anything to do with the last ref. All I'm saying 
it hints that someone needs to release the ref, and do it explicitly, and that hopefully means
explicitly in the right thread.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Olli Pettay [:smaug] from comment #9)
> Leak is less bad than releasing on a wrong thread. leak vs. security bug.

True.

> And I'm not suggesting that already_AddRefed has anything to do with the
> last ref. All I'm saying 
> it hints that someone needs to release the ref, and do it explicitly, and
> that hopefully means
> explicitly in the right thread.

Maybe.  The only instances of already_AddRefed members in the tree are in runnables whose explicit purpose is to make sure things are released on the right thread.  I don't see that the already_AddRefed is really helping in that department.

Comment 11

[Nathan Froyd [:froydnj]]

Apparently this can land prior to bug 967364.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd2e8c512f6

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cbd2e8c512f6

Comment 13

[Phil Ringnalda (:philor)]

Permabusted debug b2g emulator mochitest-1, https://tbpl.mozilla.org/php/getParsedLog.php?id=35073376&tree=Mozilla-Inbound

Comment 14

[(no longer active)]

It sounds like we're releasing this object too soon, which sounds very bad.  Nathan, can you please take a look?

Comment 15

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Jesse may have a test case in bug 975771. ;)

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Ok, this is wrong for the reasons smaug and I were concerned about (but now I realize why).

The point of this was to avoid addref/releasing wrapped JS code off mainthread, where this runnable is created.  By using already_AddRefed for both, we avoid the ref-bumping.  In theory this does too, but....

Perhaps the issue is that this is already_AddRefed<nsIFoo> aFoo; nsCOMPtr<nsIFoo> foo; and foo(aFoo) causes a QI.  And in fact:

NSCAP_FEATURE_TEST_DONTQUERY_CASES causes it to QI anyways in Assert_NoQueryNeeded() - but we're on the wrong thread.

We need to back this out for any off-main-thread nsCOMPtrs.

Comment 17

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

The right thing to do here unfortunately is to pass around nsCOMPtr<T>& and call .swap.

Comment 18

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cbacdf83f1fb

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Perhaps instead of this pattern with already_AddRefed<> in the patch we use this:

class MyRunnable : public nsRunnable
{
   MyRunnable(already_AddRefed<Thing> aThing) : mThing(aThing.forget()) {}
   NS_IMETHOD Run()
   {
      ...
   }
   MainThreadPtrHandle<Thing> mThing;
};

and new MyRunnable(mFoo.forget())

This also requires wrapping it in MainThreadPtrHolder before sending it off-main-thread, so the patch would be more intrusive

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(I didn't see this bug till today as I only monitor WebRTC component)

(In reply to Randell Jesup [:jesup] from comment #19)
>  MainThreadPtrHolder

Nice! NS_ProxyRelease back to main beats both leaking and wrong-thread release.

> This also requires wrapping it in MainThreadPtrHolder before sending it
> off-main-thread, so the patch would be more intrusive

Right. This should remove the need for the Arm() method as well: http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#769

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #14)
> It sounds like we're releasing this object too soon, which sounds very bad. 
> Nathan, can you please take a look?

Top People have already addressed the issues here.

Comment 22

[Nathan Froyd [:froydnj]]

It sounds like MainThreadPtrHandle is the smarter, safer thing to do here; do we want to try to enforce that nsRunnable subclasses can't have nsCOMPtr members (nsRefPtr?  RefPtr?) to not-so-gently encourage people to use MainThreadPtrHandle?

Comment 23

[Josh Matthews [:jdm]]

(In reply to Nathan Froyd (:froydnj) from comment #22)
> It sounds like MainThreadPtrHandle is the smarter, safer thing to do here;
> do we want to try to enforce that nsRunnable subclasses can't have nsCOMPtr
> members (nsRefPtr?  RefPtr?) to not-so-gently encourage people to use
> MainThreadPtrHandle?

Please no? Runnables are extremely common for dispatching to the same thread where this is not a concern.

Comment 24

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

nsMainThreadPtrHandle also adds (usually) unnecessary overhead.

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Josh Matthews [:jdm] (on vacation until 2/19) from comment #23)
> Please no? Runnables are extremely common for dispatching to the same thread
> where this is not a concern.

Agree. Maybe an nsCOMPtr/nsRefPtr specialization that throws a runtime error if sent to a thread other than main, would do?

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #25)
> Agree. Maybe an nsCOMPtr/nsRefPtr specialization that throws a runtime error
> if sent to a thread other than main, would do?

Let me take that back, as not all things have this limitation that they must be freed on main.

Comment 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Nathan, this is still assigned to you, are you working on it? Unclear from comment 21. I can take it if you want.

Comment 28

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

I have a patch that works in my queue.  I'll post it tomorrow.

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5a9d48b90c

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/cf81f7938209 (along with bug 619487 and bug 619487) for m-bc bustage on at least OSX: https://tbpl.mozilla.org/php/getParsedLog.php?id=35695094&tree=Mozilla-Inbound

Comment 31

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

I left off a & :P

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6afd113f7a

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e6afd113f7a