Closed Bug 968836 Opened 6 years ago Closed 6 years ago

fix remaining already_AddRefed members in MediaManager.cpp after bug 967364

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → nfroyd
Blocks: 968799
Depends on: 967364
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.
The only reason to use already_AddRefed instead of nsCOMPtr is if you're leaking.
leaking is less bad than releasing on a wrong thread.
Also, using already_AddRefed is self-documenting.
(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.
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 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??
Attachment #8371517 - Flags: review?(rjesup) → review+
(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.
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.
(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.
Apparently this can land prior to bug 967364.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd2e8c512f6
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/cbd2e8c512f6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
It sounds like we're releasing this object too soon, which sounds very bad.  Nathan, can you please take a look?
Flags: needinfo?(nfroyd)
Jesse may have a test case in bug 975771. ;)
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.
The right thing to do here unfortunately is to pass around nsCOMPtr<T>& and call .swap.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cbacdf83f1fb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer blocks: 975771
Depends on: 975771
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
(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
(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.
Flags: needinfo?(nfroyd)
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?
(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.
nsMainThreadPtrHandle also adds (usually) unnecessary overhead.
(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?
(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.
Nathan, this is still assigned to you, are you working on it? Unclear from comment 21. I can take it if you want.
Flags: needinfo?(nfroyd)
I have a patch that works in my queue.  I'll post it tomorrow.
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/2e6afd113f7a
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.