Closed Bug 977518 Opened 6 years ago Closed 6 years ago

[RTSP] Resource leak from RtspMediaResource, RtspController and RTSPSource.

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S4 (28mar)

People

(Reporter: ethan, Assigned: ethan)

Details

Attachments

(3 files, 10 obsolete files)

354.83 KB, image/svg+xml
Details
182.98 KB, image/svg+xml
Details
23.14 KB, patch
ethan
: review+
Details | Diff | Splinter Review
Story:
Bug 964132 tried to fix a resource leak in RtspMediaResource, however, it caused a regression in v1.4, which is bug 973840 (video app crash).
There is a decision to back out bug 964132 in v1.4, and we keep bug 973840 to track this back-out job.
When that is done, we won't encounter the crash, but we still have resource leaks while using RTSP streaming.

The target of this bug is to fix these resource leak issues.
Assignee: nobody → ettseng
Status: NEW → ASSIGNED
Attached patch rtsp-resource-leak-fix.patch (obsolete) — Splinter Review
What I did in this patch:
1. Resolve resource leaks in RTSP child side: RtspMediaResource and RtspControllerChild.
2. Resolve resource leaks in RTSP parent side: RtspControllerParent, RtspController and RTSPSource.
3. Introduce a mechanism to prevent the potential MsgRouteError in RTSP e10s.
Attachment #8384501 - Flags: review?(sworkman)
Sequence diagram depicting the events happened when we close the RTSP streaming normally, i.e., terminating the media document.
Sequence diagram depicting the events happened when the RTSP media is closed due to network side error.
Attached patch rtsp-resource-leak-fix.patch (obsolete) — Splinter Review
Remove a trailing space.
Attachment #8384501 - Attachment is obsolete: true
Attachment #8384501 - Flags: review?(sworkman)
Attachment #8384514 - Flags: review?(sworkman)
Hi Steve,

So far I found 3 circular references that were not broken and thus could cause resource leaks in our RTSP framework.
CR 1: RtspMediaResource-Listener
CR 2: RtspControllerParent-RtspController
CR 3: RtspController-RTSPSource

In essence, my patch is to break these circular references in the right timing and make sure these objects are all destructed after closing an RTSP media document.

After studying the event sequences involved, I decide to:
Break CR 1 in RtspMediaResource::OnDisconnected()
Break CR 2 in RtspController::OnDisconnected()
Break CR 3 in RTSPSource::onDisconnected()

This decision is predicated on the assumption that the OnDisconnected event will always be triggered and propagated from Rtsp parent side to child side, no matter the triggering source is HTMLVideoElement (abort loading) or RtspConnectionHandler (network error).
Breaking CR 3 is a little tricky since RtspController might be released from a non-main thread, but this is disallowed because RtspController has a member which must be destructed on the owning thread. Therefore I added some code to make sure RtspController is destructed on its owning thread, i.e., the main thread.

Besides, I also enhanced RtspControllerChild to ensure this object doesn't accept any command once it is either stopped by MediaResource or received an OnDisconnected event. Without this mechanism, we might encounter a crash caused by MsgRouteError in e10s. For example, currently RtspControllerChild would receive a Pause() immediately after a Stop().
Oh, I just realized that we can use some utilities in nsProxyRelease.h to help us handle the main-thread-only pointer off the main thread. I will give a try and might update the patch if it works.
Attached patch rtsp-resource-leak-fix-v2.patch (obsolete) — Splinter Review
Replace the type of RTSPSource::mListener from nsCOMPtr to nsMainThreadPtrHandle, which helps us safely release RtspController on the main thread.
Attachment #8384514 - Attachment is obsolete: true
Attachment #8384514 - Flags: review?(sworkman)
Attachment #8385193 - Flags: review?(sworkman)
Attached patch rtsp-resource-leak-fix-v2.patch (obsolete) — Splinter Review
Just remove an unused #include declaration from the previous patch.
Attachment #8385201 - Flags: review?(sworkman)
Attachment #8385193 - Attachment is obsolete: true
Attachment #8385193 - Flags: review?(sworkman)
Comment on attachment 8385201 [details] [diff] [review]
rtsp-resource-leak-fix-v2.patch

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

::: netwerk/protocol/rtsp/controller/RtspController.cpp
@@ +331,5 @@
>      nsRefPtr<SendOnDisconnectedTask> task =
>        new SendOnDisconnectedTask(mListener, index, reason);
> +    // Break the cycle reference between the Listener (RtspControllerParent) and
> +    // us.
> +    mListener = nullptr;

Do we also need to release mListener in main thread here ? The nsIStreamingProtocolListener is passed to RTSPSource when calling RtspController::AsyncOpen() in main thread, and is hold by mListener in RTSPSource. However, mListener is used on the looper thread which is created in RTSPSource::start(). So the main thread own the nsIStreamingProtocolListener but it is released in looper thread. I would like to suggest that we dispatch the event to main thread and calling mListener.onConnect/onDisconnect/OnMediaxxx in main thread.
(In reply to Vincent Chang[:vchang] from comment #9)
> Comment on attachment 8385201 [details] [diff] [review]
> ...
> I would like to suggest that we dispatch the event to main thread and calling
> mListener.onConnect/onDisconnect/OnMediaxxx in main thread.
That's a good idea.
I will see what we can do toward this way.
Comment on attachment 8385201 [details] [diff] [review]
rtsp-resource-leak-fix-v2.patch

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

Just some comments. I know you're still working on it, so f+.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +259,5 @@
>  RtspControllerChild::Play(void)
>  {
>    LOG(("RtspControllerChild::Play()"));
>    NS_ENSURE_TRUE(mIPCOpen, NS_ERROR_FAILURE);
> +  NS_ENSURE_FALSE(mIsStoppedOrDisconnected, NS_ERROR_FAILURE);

mIsStoppedOrDisconnected is main thread only, right? So, let's do a couple of things here:

1. Add a setter function, and MOZ_ASSERT that it's main thread only.
2. Make the getter function main thread only (MOZ_ASSERT).
3. Avoid using the variable directly - use the getter and setter.
4. In Play() etc. move the check for IsStoppedOrDisconnected() to the 'if (NS_IsMainThread())) { ... }' block. We'll be dispatching an event, but one or two is ok since they'll also have checks for IsStoppedOrDisconnected(). Otherwise, we would need to use a mutex.

Make sense?

@@ +325,5 @@
>    if (!mSuspendCount++) {
>      if (NS_IsMainThread()) {
>        if (!SendSuspend())
>          return NS_ERROR_FAILURE;
>      } else {

This is separate from your patch, but please check this - if mSuspendCount is main thread only, then these nested ifs may need adjusted.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.h
@@ +46,5 @@
>    void AddIPDLReference();
>    void ReleaseIPDLReference();
>    void AddMetaData(already_AddRefed<nsIStreamingProtocolMetaData> meta);
>    int  GetMetaDataLength();
> +  bool IsStoppedOrDisconnected() { return mIsStoppedOrDisconnected; }

Add a setter function too.

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +54,5 @@
>  
> +    // Use a main-thread pointer holder because mListener must be destructed on
> +    // the main thread. However, since mlistener will be used by non-main
> +    // threads from this object, we disable the strict enforcement of thread
> +    // invariants.

// Use main thread pointer, but allow access off main thread.

@@ +545,5 @@
>            CHECK(format->findInt32(kKeySampleRate, &int32Value));
>            meta->SetSampleRate(int32Value);
>          } else {
>            CHECK(format->findInt32(kKeyWidth, &int32Value));
> +          meta->SetWidth(int32Value);

Not sure what changed here :)

@@ +582,5 @@
>      CHECK(msg->findInt32("result", &err));
>  
>      if ((mLooper != NULL) && (mHandler != NULL)) {
> +        mLooper->unregisterHandler(mHandler->id());
> +        mHandler.clear();

Don't know if anything changed here.
Attachment #8385201 - Flags: review?(sworkman) → feedback+
Comment on attachment 8385201 [details] [diff] [review]
rtsp-resource-leak-fix-v2.patch

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

::: netwerk/protocol/rtsp/controller/RtspController.cpp
@@ +331,5 @@
>      nsRefPtr<SendOnDisconnectedTask> task =
>        new SendOnDisconnectedTask(mListener, index, reason);
> +    // Break the cycle reference between the Listener (RtspControllerParent) and
> +    // us.
> +    mListener = nullptr;

Vincent, thanks for your advice.
I will replace the type of mListener from nsCOMPtr to nsMainThreadPtrHandle to ensure it is destructed on the main thread. Just like what I did for RTSPSource::mListener.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +259,5 @@
>  RtspControllerChild::Play(void)
>  {
>    LOG(("RtspControllerChild::Play()"));
>    NS_ENSURE_TRUE(mIPCOpen, NS_ERROR_FAILURE);
> +  NS_ENSURE_FALSE(mIsStoppedOrDisconnected, NS_ERROR_FAILURE);

Yes, it is main thread only.
I will add a setter and make the getter/setter main thread only.

@@ +325,5 @@
>    if (!mSuspendCount++) {
>      if (NS_IsMainThread()) {
>        if (!SendSuspend())
>          return NS_ERROR_FAILURE;
>      } else {

MediaDecoder::Suspend/Resume() has MOZ_ASSERT() to make sure these two methods are called on the main thread. So I guess mSuspendCount should be main thread only too. But I am not quite sure how to deal with it here. I will do more investigation.

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +54,5 @@
>  
> +    // Use a main-thread pointer holder because mListener must be destructed on
> +    // the main thread. However, since mlistener will be used by non-main
> +    // threads from this object, we disable the strict enforcement of thread
> +    // invariants.

Thanks! Your comment is much more succinct. :)

@@ +545,5 @@
>            CHECK(format->findInt32(kKeySampleRate, &int32Value));
>            meta->SetSampleRate(int32Value);
>          } else {
>            CHECK(format->findInt32(kKeyWidth, &int32Value));
> +          meta->SetWidth(int32Value);

I just replaced a tab to 4 whitespaces.

@@ +582,5 @@
>      CHECK(msg->findInt32("result", &err));
>  
>      if ((mLooper != NULL) && (mHandler != NULL)) {
> +        mLooper->unregisterHandler(mHandler->id());
> +        mHandler.clear();

I just want to change 2-spaces indentation to 4-spaces, to make them consistent with the coding style of this source file. And I just found some other lines are indented using 2 spaces. Maybe I will fix them altogether.
Attached patch rtsp-resource-leak-fix-v3.patch (obsolete) — Splinter Review
Fix the issues according to reviewers' comments.
Attachment #8389755 - Flags: review?(sworkman)
Attached patch rtsp-resource-leak-fix-v3.patch (obsolete) — Splinter Review
Remove a trailing whitespace.
Attachment #8385201 - Attachment is obsolete: true
Attachment #8389755 - Attachment is obsolete: true
Attachment #8389755 - Flags: review?(sworkman)
Attachment #8389756 - Flags: review?(sworkman)
Comment on attachment 8389756 [details] [diff] [review]
rtsp-resource-leak-fix-v3.patch

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

::: content/media/RtspMediaResource.cpp
@@ +587,5 @@
>    }
>  
> +  if (mListener) {
> +    // Kill its reference to us since we're going away.
> +    mListener->Revoke();

I still think we need kungfuDeathGrip here since kill the reference here may also trigger destructor. Or we need to guarantee this line is always at the end of function.
Comment on attachment 8389756 [details] [diff] [review]
rtsp-resource-leak-fix-v3.patch

Steve, I canceled the review because of the following reasons.
Major reasons:
1. I just found RtspControllerChild's destructor is not called in a certain case - playing an audio-only RTSP streaming. I don't know why yet. Need to find out the root cause.
2. The target of this bug will be 1.5, the version in which we will render RTSP inside the browser.
   So far I still test this patch with the video app. It came to my mind that I should integrate these two works to verify the solution works well.

Minor reasons:
1. I feel "mIsStoppedOrDisconnected" is a little wordy. Try to figure out a better name (if I can).
2. I want to polish some codes, such as RtspMediaResource::OnDisconnected(). A colleague told me about the trick of "kungfuDeathGrip" and I am considering to use that technique in this method.

If you have time, please feedback this patch updated according to your comments. :)
Attachment #8389756 - Flags: review?(sworkman) → feedback?(sworkman)
Comment on attachment 8389756 [details] [diff] [review]
rtsp-resource-leak-fix-v3.patch

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

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +62,5 @@
>    : mIPCOpen(false)
>    , mChannel(channel)
>    , mTotalTracks(0)
>    , mSuspendCount(0)
> +  , mIsStoppedOrDisconnected(false)

Ideas on naming:
-- I think mDisconnected is ok here. I think the controller ends up being in a disconnected state for most cases anyway, right? If you don't think of anything else, that could be fine.
-- mShutdown? I can imagine a controller being shutdown, and this is a commonly used concept. However, if you use this, you should add a Shutdown function.

@@ +247,5 @@
> +      // RtspMediaResource and us are going to be destructed soon.
> +      // The NS_ENSURE_FALSE(IsStoppedOrDisconnected) in each function has the
> +      // same purpose.
> +      // This check here (in main thread) is necessary because the previous
> +      // checkpoint might be eschewed due to multi-threading condition.

// Don't send any more IPC events; no guarantee that parent objects are still alive.

@@ +284,5 @@
>    LOG(("RtspControllerChild::Play()"));
>    NS_ENSURE_TRUE(mIPCOpen, NS_ERROR_FAILURE);
>  
>    if (NS_IsMainThread()) {
> +    NS_ENSURE_FALSE(IsStoppedOrDisconnected(), NS_ERROR_FAILURE);

The logic here is fine. However, there is a movement away from NS_ENSURE_...  to "if (NS_WARN_IF(...". There is a little more code, but folks are saying that the actions following the condition are clearer.

I like NS_ENSURE_..., but I see the point. Probably best to convert to 'if (NS_WARN_IF...'.

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +259,5 @@
>    LOG(("RtspControllerParent::OnDisconnected() for track %d reason = 0x%x", index, reason));
>    if (!mIPCOpen || !SendOnDisconnected(index, reason)) {
>      return NS_ERROR_FAILURE;
>    }
> +  mController = nullptr;

For all these member variables that get nulled out, make sure that every use is null-checked first. And it's probably a good idea to think about places where it's ok to be null, vs those where you should never be null, i.e. where to MOZ_ASSERT or even MOZ_RELEASE_ASSERT.

These checks may already be there - it's just something to verify.
Just some initial thoughts. More later.
Comment on attachment 8389756 [details] [diff] [review]
rtsp-resource-leak-fix-v3.patch

Actually, I had no more comments in the remaining files. I'll just wait until you sort out the issues in comment 16.
Attachment #8389756 - Flags: feedback?(sworkman) → feedback+
(In reply to Benjamin Chen [:bechen] from comment #15)
> I still think we need kungfuDeathGrip here since kill the reference here may
> also trigger destructor. Or we need to guarantee this line is always at the
> end of function.
Hi Benjamin, I agree. Adding this could increase the maintainability of that method.
(In reply to Steve Workman [:sworkman] from comment #17)
> ::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
> > +  , mIsStoppedOrDisconnected(false)
> Ideas on naming:
> -- I think mDisconnected is ok here. I think the controller ends up being in
> a disconnected state for most cases anyway, right? If you don't think of
> anything else, that could be fine.
> -- mShutdown? I can imagine a controller being shutdown, and this is a
> commonly used concept. However, if you use this, you should add a Shutdown
> function.
mDisconnected sounds much terser.
I am just afraid it would be a little misleading since it could be set as true when RtspControllerChild::Stop() is called but the connection is not actually disconnected yet (as you know, from the perspective of ControllerChild, the connection is disconnected when RecvOnDisconnected() is called). 

The intention of this variable is just to avoid any IPC message to be sent when this flag is set as true. Nothing more.
For example, I want to avoid message no. 10 (SendPause) to be sent in attachment 8384512 [details], which could cause message no. 23 (SendOnDisconnected) to be sent from the parent to an non-existing actor.
Attached patch rtsp-resource-leak-fix-v4.patch (obsolete) — Splinter Review
Polished.
1. Add kungFuDeathGrip in RtspMediaResource::OnDisconnected().
2. Replace NS_ENSURE_FALSE() by if (NS_WARN_IF(...)).
3. Add null-checks for mController.
4. Refine some comments.
Attachment #8389756 - Attachment is obsolete: true
Component: General → RTSP
(In reply to Ethan Tseng [:ethan] from comment #16)
> Steve, I canceled the review because of the following reasons.
> Major reasons:
> 1. I just found RtspControllerChild's destructor is not called in a certain
> case - playing an audio-only RTSP streaming. I don't know why yet. Need to
> find out the root cause.
The reason is, Linux kernel killed the content process (video app) before RtspControllerChild's destructor is called. This is revealed by the fact that RtspControllerParent::ActorDestroy() is still called in this case, and it is called from ContentParent::ShutdownProcess().
I think the heap memory allocated by child process will be recycled by OS, thus we don't need to worry about resource leak for this case.

> 2. The target of this bug will be 1.5, the version in which we will render
> RTSP inside the browser.
>    So far I still test this patch with the video app. It came to my mind
> that I should integrate these two works to verify the solution works well.
I tested this patch while rendering RTSP inside the browser. The resource leak issue is not resolved in this situation because RtspChannel doesn't implement OnStopRequest, it neither has relationship with the media resource listener.
Since we are also refactoring RtspChannel in another bug (bug 949675), for clarity, I will deal with this remaining issue in that bug.
 
> Minor reasons:
> 1. I feel "mIsStoppedOrDisconnected" is a little wordy. Try to figure out a
> better name (if I can).
I will just rename is as "mStoppedOrDisconnected". Although wordy, it is the most clear name I can think of so far.

> 2. I want to polish some codes, such as RtspMediaResource::OnDisconnected().
> A colleague told me about the trick of "kungfuDeathGrip" and I am
> considering to use that technique in this method.
This is added.
Attached patch rtsp-resource-leak-v5.patch (obsolete) — Splinter Review
Rename mIsStoppedOrDisconnected as mStoppedOrDisconnected, and refine the comment describing this data member.

Steve, I already sorted out the issues in comment 16 (see comment 23). I think this patch is ready for review now.
Attachment #8392694 - Attachment is obsolete: true
Attachment #8394575 - Flags: review?(sworkman)
Comment on attachment 8394575 [details] [diff] [review]
rtsp-resource-leak-v5.patch

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

Looks good. Just a few things to check, but almost there.

::: content/media/RtspMediaResource.cpp
@@ +590,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
> +  // mListener::Revoke() can release this object. So hold a reference to this
> +  // until the end of the method.
> +  nsRefPtr<RtspMediaResource> kungFuDeathGrip = this;

Hmm. The mListener pointers are now main thread, so Revoke and release should be happening on the main thread too. So, that means you probably don't need kungFuDeathGrip.

For extra assurance, add an NS_IsMainThread() assertion in Revoke. Rather than include extra headers, you might want to move Revoke's implementationt to the cpp file now.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +241,5 @@
>  
>    NS_IMETHOD Run()
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +    if (NS_WARN_IF(mController->IsStoppedOrDisconnected() == true)) {

I'm wondering if we need these warnings. Yes, this is an error case, but it's one that we're handling and recovering from. In other words, we're expecting it to happen. If this case is going to happen a lot, then we might want to remove the warning. Maybe a LOG is better if we need it for debugging?

Not a huge issue - just think about it.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.h
@@ +69,5 @@
> +  // The purpose of this flag is to prevent sending IPC to a nonexistent actor.
> +  // Once nsIStreamingProtocolController::Stop or
> +  // nsIStreamingProtocolListener::OnDisconnected happens, we shall not send IPC
> +  // events.
> +  bool mStoppedOrDisconnected;

You wrote "The intention of this variable is just to avoid any IPC message to be sent when this flag is set as true. Nothing more."

So, I'm wondering if we should just rename this to mOKToSendIPC?

And if you want to avoid ANY message being sent, then this should be checked before all sends, right? So, it should be beside mIPCOpen. And it looks like you could fold both together.

s/mIPCOpen/mOKToSendIPC/g
mOKToSendIPC = false where needed (probably SetOKToSendIPC so you can do that main thread check).
And then checks like this:
  if (!mIPCOpen || !SendAsyncOpen(uriParams)) {
    return NS_ERROR_FAILURE;
  }

I prefer the idea of having one variable to control IPC sending if possible. Or if you really need two variables, then have a single function and use it throughout the file.
Attachment #8394575 - Flags: review?(sworkman) → review-
(In reply to Steve Workman [:sworkman] from comment #25)
> Comment on attachment 8394575 [details] [diff] [review]
> rtsp-resource-leak-v5.patch
> ::: content/media/RtspMediaResource.cpp
> @@ +590,5 @@
> >  {
> >    NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
> > +  // mListener::Revoke() can release this object. So hold a reference to this
> > +  // until the end of the method.
> > +  nsRefPtr<RtspMediaResource> kungFuDeathGrip = this;
> 
> Hmm. The mListener pointers are now main thread, so Revoke and release
> should be happening on the main thread too. So, that means you probably
> don't need kungFuDeathGrip.
I am a little confused about this comment.
The purpose of kungFuDeathGrip is to ensure the OnDisconnected() method invocation completes before the RtspMediaResource object is destructed. It is not used to guarantee the object is released on the main thread.
Do you mean that, if mListener::Revoke() is guaranteed to be executed on the main thread, we don't need worry about the "destruction happens before function completes" problem? If this is true, I will remove kungFuDeathGrip.
I recall the scenario we found it's necessary to add kungFuDeathGrip.
Originally I added mListener->Revoke() before the mDecoder block. In the normal case it is fine because mDecoder would be set as nullptr in RtspMediaResource::Close() and the mDecoder block doesn't matter.

But when I tested a network error case (e.g., connect to a nonexistent RTSP server), I was surprised that mDecoder->NetworkError() is not called and then identified the reason is RtspMediaResource object is destructed in mListener->Revoke() invocation. Thus I move this line to the end of this method.
Then we realized the technique of kungFuDeathGrip from other source file and though it's better to add it, in case when we changes codes in the future that do something after Revoke().
Attached patch rtsp-resource-leak-v6.patch (obsolete) — Splinter Review
Minor improvements according to comment 25:
1. Rename mStoppedOrDisconnected to mOKToSendIPC. Two relevant methods are also renamed to SetOKToSendIPC() and IsOKToSendIPC().
2. Remove NS_WARN_IF() checks for IsOKToSendIPC().
3. Add return code for SendXXX() in SendIPCEvent class.
Attachment #8394575 - Attachment is obsolete: true
Attachment #8396271 - Flags: review?(sworkman)
(In reply to Steve Workman [:sworkman] from comment #25)
> ::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
> @@ +241,5 @@
> > +    if (NS_WARN_IF(mController->IsStoppedOrDisconnected() == true)) {
> I'm wondering if we need these warnings. Yes, this is an error case, but
> it's one that we're handling and recovering from. In other words, we're
> expecting it to happen. If this case is going to happen a lot, then we might
> want to remove the warning. Maybe a LOG is better if we need it for
> debugging?
> Not a huge issue - just think about it.
After further consideration, I think you are right. These checks are expected to happen and we don't need to emit warnings for them. So I removed those NS_WARN_IFs.

> ::: netwerk/protocol/rtsp/controller/RtspControllerChild.h
> So, I'm wondering if we should just rename this to mOKToSendIPC?
> And if you want to avoid ANY message being sent, then this should be checked
> before all sends, right? So, it should be beside mIPCOpen. And it looks like
> you could fold both together. 
> I prefer the idea of having one variable to control IPC sending if possible.
> Or if you really need two variables, then have a single function and use it
> throughout the file.
Thanks! I like the name mOKToSendIPC. I also like the idea of having only one variable to control IPC.
However, I still keep the variable mIPCOpen because the IPDL Release function needs it to determine whether to Send__delete__() or not.
I integrate the checks for mIPCOpen and mOKToSendIPC into the method IsOKToSendIPC() and use it throughout the file like you suggested.
The fly in the ointment is IsOKToSendIPC and SetOKToSendIPC are a little asymmetric.
If you have any suggestion, please let me know. :)

BTW, I still keep kungFuDeathGrip in RtspMediaResource (reason stated in comment 27).
Comment on attachment 8396271 [details] [diff] [review]
rtsp-resource-leak-v6.patch

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

Looks good. r=me assuming you'll check a couple of things and make some (final) naming changes as requested.

::: content/media/RtspMediaResource.cpp
@@ +590,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
> +  // mListener::Revoke() can release this object. So hold a reference to this
> +  // until the end of the method.
> +  nsRefPtr<RtspMediaResource> kungFuDeathGrip = this;

Sorry, I think I had two things in my head and I wasn't clear. And I didn't understand the need for kungFuDeathGrip here.

Firstly, from comment 15 and comment 20, it looks like this local ref was need to avoid RtspMediaResource being released and deleted before the end of the function. I'd prefer you just do Revoke at the end of the function and add a comment. This should be enough for future maintenance.

I don't think comment 27 changes this, does it?

I'm not going to block the r+ on this, so just check again and see if it's really needed for the code as it is now. I think it's better to avoid the extra AddRef if possible. But a comment for future maintenance would be good.


Secondly, threading: You made other references to the Listener to be main thread, so all deletions should happen on the main thread, right? This suggests that Revoke should be main thread only too, no? So, if that's the case, then there is no chance for another thread to call Revoke and set mResource to nullptr before OnDisconnected finishes.

I don't think Revoke could be called on another thread, but please check. Assuming it is only called on the main thread, please add a main thread assertion.

Just take a look at these and decide what to do - I'll not block on it.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +91,5 @@
> +void
> +RtspControllerChild::SetOKToSendIPC(bool value)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mOKToSendIPC = value;

Thank you for your patience on this one (yes, sorry, that means another suggested change :) ).

You had concerns about IsOKToSendIPC() and SetOKToSendIPC being asymmetric. Valid concerns.

I'm concerned that the function names are still a little cumbersome. Better than StoppedOrDisconnected, so we're making progress.

These will be my final suggestions, because I don't want you to waste more time on it.

1. s/IsOKToSendIPC/OKToSendIPC/g - 'Is' is implicit and removing it is a little easier to read.

2. s/mOKToSendIPC/mIPCAllowed/g

3. SetOKToSendIPC --> AllowIPC() and DisallowIPC(). Those two set and unset mIPCAllowed.

So, this way, we couple mIPCAllowed, AllowIPC() and DisallowIPC() more clearly, and show them being related to mIPCOpen but used for a different reason. (Note: I suggest you keep Allow/DisallowIPC() rather than just un/setting mIPCAllowed directly - we still want that main thread assertion).

And then, OKToSendIPC() is more clearly an individual function which we're using to aggregate the states held in mIPCOpen and mIPCAllowed.

I think this answers all naming concerns :) and makes the code clearer. So, I won't make any more comments on this ;) (Well, unless there's something functionally wrong (!), but I think that's all ok).
Attachment #8396271 - Flags: review?(sworkman) → review+
Attached patch rtsp-resource-leak-v7.patch (obsolete) — Splinter Review
Resolved the issues in comment 30.
Attachment #8396271 - Attachment is obsolete: true
Attachment #8397631 - Flags: review+
(In reply to Steve Workman [:sworkman] from comment #30)
> ::: content/media/RtspMediaResource.cpp 
> I don't think comment 27 changes this, does it?
No. It doesn't change your inference.

> I'm not going to block the r+ on this, so just check again and see if it's
> really needed for the code as it is now. I think it's better to avoid the
> extra AddRef if possible. But a comment for future maintenance would be good.
> Secondly, threading: You made other references to the Listener to be main
> thread, so all deletions should happen on the main thread, right? This
> suggests that Revoke should be main thread only too, no? So, if that's the
> case, then there is no chance for another thread to call Revoke and set
> mResource to nullptr before OnDisconnected finishes.
> I don't think Revoke could be called on another thread, but please check.
> Assuming it is only called on the main thread, please add a main thread
> assertion.
I agree with you.
I removed the extra ref (kungFuDeathGrip), added comments for calling Revoke, and also added a main thread assertion for Revoke method.

> Thank you for your patience on this one (yes, sorry, that means another
> suggested change :) )
No, I should say thank you for your patience to help me refining this part.
The final code is quite much clearer and better than my previous patches. Thanks! :)
Fixed busts in B2G JB/KK Emulator builds (cannot convert 'bool' to 'nsresult').
Attachment #8397631 - Attachment is obsolete: true
Attachment #8397703 - Flags: review+
Keywords: checkin-needed
Target Milestone: --- → 1.4 S5 (11apr)
RTSP with enabling video is not a 1.4 feature.
Target Milestone: 1.4 S5 (11apr) → ---
https://hg.mozilla.org/mozilla-central/rev/0216e66ff63b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S4 (28mar)
You need to log in before you can comment on or make changes to this bug.