Comment on attachment 8463145 [details] [diff] [review] Part 3: Fix two refcounting errors in MediaRecorder code This looks like an actual bug to me...

Comment on attachment 8463142 [details] [diff] [review] Part 2: Make ServiceWorkerManager::GetServiceWorkerRegistration honor XPCOM rules huh

https://hg.mozilla.org/integration/mozilla-inbound/rev/43a7db7bf902 https://hg.mozilla.org/integration/mozilla-inbound/rev/c6691cff88a4

Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/39d33af8f0f1 https://tbpl.mozilla.org/php/getParsedLog.php?id=44715745&tree=Mozilla-Inbound

Ehsan relanded this: https://hg.mozilla.org/integration/mozilla-inbound/rev/16c39aa85350 https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fe7fa48e45 Then I'm rebacking out part 3 for many leaks in M1 (bug 1045335): https://hg.mozilla.org/integration/mozilla-inbound/rev/668f9ca60450

(In reply to Andrew McCreight [:mccr8] from comment #8) > Ehsan relanded this: > https://hg.mozilla.org/integration/mozilla-inbound/rev/16c39aa85350 > https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fe7fa48e45 > > Then I'm rebacking out part 3 for many leaks in M1 (bug 1045335): > https://hg.mozilla.org/integration/mozilla-inbound/rev/668f9ca60450 Holy crap!!! CJ, Randy, Ben, you guys have touched this code, can you please help me understand where the AddRef call on the thing that we pass to ExtractRunnable comes from? I basically don't understand where we AddRef |this|, so we end up passing |this| to ExtractRunnable, which accepts an already_AddRefed&&, so we construct a temporary already_AddRefed which gets passed to ExtractRunnable's constructor. My patch adds an explicit AddRef through the |thisSession| variable, but that causes leaks, so this must mean that the Session object was correctly addrefed somehow before my patch...

I'm not sure it's possible to support already_AddRefed<T>(nullptr), because this context permits looking at explicit constructors, and then you're off in the weeds. But it appears there are negligibly few instances of that, which we can replace with calls to the default constructor, so let's do that. https://tbpl.mozilla.org/?tree=Try&rev=ebeaa11adf90 https://tbpl.mozilla.org/?tree=Try&rev=b2e749ee7604 Incidentally, if you're wondering why |return nullptr| isn't ambiguous wrt (const already_AddRefed<T>& aOther) or (already_AddRefed<T>&& aOther), I believe it's because the returning context treats the returned expression as an rvalue (because obviously it can't be used past the function return point). Not 100% sure, but tbpl don't lie, right? We are not expected to understand this.

Comment on attachment 8463132 [details] [diff] [review] Don't undef MOZ_HAVE_CXX11_NULLPTR in NullPtr.h since nsCOMPtr.h depends on it Review of attachment 8463132 [details] [diff] [review]: ----------------------------------------------------------------- It was intentional to remove this, in bug 953296. Guess I missed an instance. See counter-patch for a fix that doesn't let people shoot themselves in the foot with the macro, as is all too easy to do when template pointer types, int/long, or other sadnesses get involved.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9) > (In reply to Andrew McCreight [:mccr8] from comment #8) > > Ehsan relanded this: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/16c39aa85350 > > https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fe7fa48e45 > > > > Then I'm rebacking out part 3 for many leaks in M1 (bug 1045335): > > https://hg.mozilla.org/integration/mozilla-inbound/rev/668f9ca60450 > > Holy crap!!! > > CJ, Randy, Ben, you guys have touched this code, can you please help me > understand where the AddRef call on the thing that we pass to > ExtractRunnable comes from? I basically don't understand where we AddRef > |this|, so we end up passing |this| to ExtractRunnable, which accepts an > already_AddRefed&&, so we construct a temporary already_AddRefed which gets > passed to ExtractRunnable's constructor. My patch adds an explicit AddRef > through the |thisSession| variable, but that causes leaks, so this must mean > that the Session object was correctly addrefed somehow before my patch... At the MediaRecorder::Start function. http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=mediarecorder.cpp&case=true#646 Here we create a Session and addref on it. The last reference will be destroyed at DestroyRunnable.

(In reply to Benjamin Chen [:bechen] from comment #12) > At the MediaRecorder::Start function. > http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder. > cpp?from=mediarecorder.cpp&case=true#646 > Here we create a Session and addref on it. The last reference will be > destroyed at DestroyRunnable. Is there a reason MediaRecorder::mSessions isn't an nsTArray<nsRefPtr<Session>>? It looks slightly fussy but doable (at a skim) to do that and have the array element be the owning reference in this code.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13) > (In reply to Benjamin Chen [:bechen] from comment #12) > > At the MediaRecorder::Start function. > > http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder. > > cpp?from=mediarecorder.cpp&case=true#646 > > Here we create a Session and addref on it. The last reference will be > > destroyed at DestroyRunnable. > > Is there a reason MediaRecorder::mSessions isn't an > nsTArray<nsRefPtr<Session>>? It looks slightly fussy but doable (at a skim) > to do that and have the array element be the owning reference in this code. Yes, if we change the mSession to nsTArray<nsRefPtr<Session>>, that will make a cycle reference between Session and MediaRecorder because Session owns a reference to MediaRecorder. And since the we have ensure the DestroyRunnable always be invoked for each Session object. We can break the cycle reference in DestroyRunnable. Currently the

(In reply to Benjamin Chen [:bechen] from comment #12) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #9) > > (In reply to Andrew McCreight [:mccr8] from comment #8) > > > Ehsan relanded this: > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/16c39aa85350 > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fe7fa48e45 > > > > > > Then I'm rebacking out part 3 for many leaks in M1 (bug 1045335): > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/668f9ca60450 > > > > Holy crap!!! > > > > CJ, Randy, Ben, you guys have touched this code, can you please help me > > understand where the AddRef call on the thing that we pass to > > ExtractRunnable comes from? I basically don't understand where we AddRef > > |this|, so we end up passing |this| to ExtractRunnable, which accepts an > > already_AddRefed&&, so we construct a temporary already_AddRefed which gets > > passed to ExtractRunnable's constructor. My patch adds an explicit AddRef > > through the |thisSession| variable, but that causes leaks, so this must mean > > that the Session object was correctly addrefed somehow before my patch... > > At the MediaRecorder::Start function. > http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder. > cpp?from=mediarecorder.cpp&case=true#646 > Here we create a Session and addref on it. The last reference will be > destroyed at DestroyRunnable. Aha, thanks! I find that code pretty bizarre, any reason why we can't remove that and just addref Session properly before passing it to ExtractRunnable/DestroyRunnable?

Comment on attachment 8463733 [details] [diff] [review] Throw more templates, SFINAE, overloading, explicit at already_AddRefed<T> constructors Review of attachment 8463733 [details] [diff] [review]: ----------------------------------------------------------------- For the record, I don't understand more than 20% of what's going on here. :/ ::: xpcom/glue/nsCOMPtr.h @@ +132,3 @@ > |nsCOMPtr_helper|. > */ > { Nit: please use 8 lines of context for patches. @@ +141,5 @@ > + * MatchOnly* overload below. Note that because the latter isn't explicit, > + * explicit already_AddRefed<T>(nullptr) or similar with older compilers ends > + * up ambiguous amongst (MatchOnlyEmulatedNullptr), (T*), and > + * (already_AddRefed<T>&&), so you'll have to use already_AddRefed<T>() > + * instead. You are not expected to understand this. Please remove "You are not expected to understand this." and maybe replace it with "I'm not sure about the language rules here" or something that explains the situation more accurately. @@ +150,3 @@ > */ > + template<typename MatchOnlyTrueNullptr> > + already_AddRefed(MatchOnlyTrueNullptr, So, which constructor makes it possible to return nullptr from functions returning a type of already_AddRefed<T>? I can't tell whether it's this one of |already_AddRefed(MatchOnlyEmulatedNullptr aRawPtr)| below. Please make that ctor MOZ_IMPLICIT and the other explicit. @@ +154,1 @@ > int>::Type aDummy = 0) Nit: indentation. @@ +155,5 @@ > : mRawPtr(nullptr) > { > } > > + typedef void (already_AddRefed::* MatchOnlyEmulatedNullptr)(double, float); Please add a comment explaining that the double and float arguments are only there to make sure this type doesn't accidentally match a member function.

https://hg.mozilla.org/mozilla-central/rev/16c39aa85350 https://hg.mozilla.org/mozilla-central/rev/c3fe7fa48e45

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e50fb773dff (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #16) > For the record, I don't understand more than 20% of what's going on here. :/ :-) "You are not expected to understand this." (Truthful, but also a sly reference to http://cm.bell-labs.com/cm/cs/who/dmr/odd.html if you weren't aware of it.) > Nit: please use 8 lines of context for patches. Blame bzexport. > Please remove "You are not expected to understand this." and maybe replace > it with "I'm not sure about the language rules here" or something that > explains the situation more accurately. Actually, while adjusting comments, I realized that true nullptr will target the implicit pointer-to-member-function overload just as well as it'll target decltype(nullptr). So really we can just have explicit (T*), (), and (pointer to member function) overloads and everything works fine, no need for SFINAE or other things. > @@ +150,3 @@ > > */ > > + template<typename MatchOnlyTrueNullptr> > > + already_AddRefed(MatchOnlyTrueNullptr, > > So, which constructor makes it possible to return nullptr from functions > returning a type of already_AddRefed<T>? I can't tell whether it's this one > of |already_AddRefed(MatchOnlyEmulatedNullptr aRawPtr)| below. Please make > that ctor MOZ_IMPLICIT and the other explicit. This is more pedagogical than useful now, with the above change. But it depends! If nullptr is emulated, then |return nullptr| is handled by the pointer-to-member-function overload. If it's true nullptr, it's handled by the templated IsNullPointer overload. I did make the constructor MOZ_IMPLICIT, tho. > @@ +154,1 @@ > > int>::Type aDummy = 0) > > Nit: indentation. I have no idea what you're complaining about here. The indentation aligns the second template parameter with the first, which seems the only sane way to readably indent here. :-) Not that it matters with this gone now anyway. > Please add a comment explaining that the double and float arguments are only > there to make sure this type doesn't accidentally match a member function. Done.

Oh, for what it's worth, I believe our use of |explicit (T*)| to prohibit |T* ptr; return ptr;| in functions returning |already_AddRefed<T>| will no longer work if Option 1 in this C++ proposal becomes part of the eventual standard: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4029.pdf Or so it reads to me, if I'm reading it correctly. This seems like input possibly worth sending to the C++ standards process.

Followup fix, since apparently my patch requires the Session/MediaRecorder change to happen (I think I was developing on a tree where it had landed, not the current tree where it's been backed out): https://hg.mozilla.org/integration/mozilla-inbound/rev/b0dd336a76c8 Basically instead of |new ExtractRunnable(this)| I put in an a_AR<Session> local, then passed a Move(session) into each place. Should get things compiling, at least, but someone should probably verify it's the ultimate desired fix. (I prefer not backing out my change because it's to nsCOMPtr.h which causes mega-clobbering every time I push/pop the patch in my tree.)

https://hg.mozilla.org/mozilla-central/rev/5e50fb773dff https://hg.mozilla.org/mozilla-central/rev/b0dd336a76c8

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15) > (In reply to Benjamin Chen [:bechen] from comment #12) > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > > comment #9) > > > (In reply to Andrew McCreight [:mccr8] from comment #8) > > > > Ehsan relanded this: > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/16c39aa85350 > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fe7fa48e45 > > > > > > > > Then I'm rebacking out part 3 for many leaks in M1 (bug 1045335): > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/668f9ca60450 > > > > > > Holy crap!!! > > > > > > CJ, Randy, Ben, you guys have touched this code, can you please help me > > > understand where the AddRef call on the thing that we pass to > > > ExtractRunnable comes from? I basically don't understand where we AddRef > > > |this|, so we end up passing |this| to ExtractRunnable, which accepts an > > > already_AddRefed&&, so we construct a temporary already_AddRefed which gets > > > passed to ExtractRunnable's constructor. My patch adds an explicit AddRef > > > through the |thisSession| variable, but that causes leaks, so this must mean > > > that the Session object was correctly addrefed somehow before my patch... > > > > At the MediaRecorder::Start function. > > http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder. > > cpp?from=mediarecorder.cpp&case=true#646 > > Here we create a Session and addref on it. The last reference will be > > destroyed at DestroyRunnable. > > Aha, thanks! I find that code pretty bizarre, any reason why we can't > remove that and just addref Session properly before passing it to > ExtractRunnable/DestroyRunnable? Yeah, we can remove that and addref at DoSessionEndTask and the first ExtractRunnable in AfterTracksAdded. http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=MediaRecorder.cpp&case=true#464 http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=MediaRecorder.cpp&case=true#458

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19) > Oh, for what it's worth, I believe our use of |explicit (T*)| to prohibit > |T* ptr; return ptr;| in functions returning |already_AddRefed<T>| will no > longer work if Option 1 in this C++ proposal becomes part of the eventual > standard: > > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4029.pdf > > Or so it reads to me, if I'm reading it correctly. This seems like input > possibly worth sending to the C++ standards process. Interesting! I was actually going to send some negative feedback on that proposal, and this gives me more material. Thanks.

Note that, as far as I could tell, only Option 1 would be problematic in this way. Maybe I'm reading it wrong, could easily be the case. I actually kind of like the proposal, as a general rule. But it does happen to break down for our particular use case. I'm not sure where that leaves things, except that I don't see a way that code could pick and choose whether return context is or isn't explicit, so it seems impossible to have mandatory explicitness when desired and discretionary explicitness when desired.