973840 - [RTSP] Video app crashed when leaving video app that is rendering RTSP media (regression caused by bug 964132)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ethan Tseng [:ethan]]

Attachment: video_app_crash.log

Reproduction Steps:
1. Open the browser app.
2. Go to a web page which contains <a> links to RTSP sources.
   Such as William Hsu's test page: http://goo.gl/lE2eE3
3. Click an RTSP link to launch the video app to play RTSP streaming.
   Such as "Video test page (3GP)" on the above web page.
4. While the video app is playing, either press the "back" or "home" button to leave the video app.
5. An error message "just crashed." is shown on the bottom of the screen.

Reproduction Rate:
Very high (almost 100%).

Note:
This bug does not occur in v1.3.

Attachment:
The attached is the GDB backtrace log of the video app process.

Comment 1

[Ethan Tseng [:ethan]]

Hi Ken,
I already identified this bug is caused by the patch of bug 964132, which tried to fix a cycle reference problem of RtspMediaResource but had this side effect. I will provide a patch in this bug in a few days.
Obviously this bug is a blocker for v1.4. Could you raise the flag?

Comment 2

[Ethan Tseng [:ethan]]

Attachment: Close RtspMediaResource.svg

This is the sequence diagram depicting the release procedure of RTSP media.

Comment 3

[Ethan Tseng [:ethan]]

Hi Steve,
I think this crash was caused by a missing Child Actor in IPC.

The patch of bug 964132 resolved a cycle reference between RtspMediaResource and its internal Listener by changing the Listener::mResource from a nsRefPtr to a raw pointer.
The problem is, when RtspControllerParent sends an OnDisconnected message to its child side (see message no. 16 in attachment 8378399 [details]), RtspControllerChild was destructed already and then caused the "MsgRouteError".

My idea is, we should still use nsRefPtr for Listener::mResource to keep RtspMediaResource and RtspControllerChild alive before receiving the OnDisconnected message. Once OnDisconnected() is called, we could break the cycle reference then.
I found ChannelMediaResource::CloseChannel() is doing something like that.
How do you think?

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

That seems logical to me. Or another way is to have RtspControllerChild add an nsRefPtr to itself. Something like RtspControllerChild::mRefForParent. Make sure it is initialized when the child knows that the parent is created, and then any time the parent signals that it is done.

Add a couple of functions: AddRefForParent { mRefForParent = this; } and ReleaseForParent { mRefForParent = nullptr; }. These are then self-commenting.

I'd call AddRefForParent after the constructor send function is called and returns successfully.

It seems like this decouples the ControllerChild-Resource relationship from the ControllerParent-ControllerChild relationship, and avoids circular referencing. What do you think? Do you see anything wrong with this?

Make sure that OnDisconnected is the only function in which you need to release the reference.

Comment 5

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #4)
> It seems like this decouples the ControllerChild-Resource relationship from
> the ControllerParent-ControllerChild relationship, and avoids circular
> referencing. What do you think? Do you see anything wrong with this?
Great! I like this idea.
Decoupling the relationship between ControllerChild and MediaResource sounds quite good to me.
 
> Make sure that OnDisconnected is the only function in which you need to
> release the reference.
I traced the possible code path and I think RtspControllerChild::OnDisconnected() would be called no matter how we release the resource. I will double check this. Thanks for reminder.

Comment 6

[Ethan Tseng [:ethan]]

Attachment: bug-973840-wip.patch

Hi Steve, this is a WIP patch. There are some codes I am not sure they are proper or not.
Could you review and provide some feedback?

Basically I applied your suggestion in the comment above, and I added something more. They are:
1. Since we decouple the ControllerChild-Resource relationship, I added a Stop invocation in the destructor of RtspMediaResource, in order to make sure the Controller part is guaranteed to be released.
2. In the case of exiting Video app, I found RtspControllerChild::Stop() would be followed by a Pause() call, which is called by RtspOmxReader. This is from bug 947113, which fixed a seek problem. This means if we ReleaseForParent in RecvOnDisconnected, the parent side would still send a message due to the RecvPause message and the MsgRouteError would still happen.
In order to fix this, I added mIsStopped check before every Send IPC function in ControllerChild.
My idea is, once ControllerChild::Stop() is called, it should not handle any further command and just expect the OnDisconnected event to come.
3. I found ControllerChild::ReleaseIPDLReference() was never called before, which seems should be called by NeckoChild. I don't know why and added this call in RecvOnDisconnected().

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8379601 [details] [diff] [review]
bug-973840-wip.patch

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

Sorry, Ethan, I'm a bit confused here and I forgot about Add|ReleaseIPDLReference.

Looking at ChannelMediaResource again, I think your first suggestion may be right, i.e. restoring the circular reference, and breaking it in OnDisconnected - like ChannelMediaResource::CloseChannel. Or at least I think it is worthwhile looking at that solution again.

Also, Add|ReleaseIPDLReference does the job that Add|ReleaseRefForParent would do, so we shouldn't need new functions. AddIPDLReference is called in the constructor, just before sending the message to construct the parent. And then it is released in NeckoChild::DeallocPRtspControllerChild. And Send__delete__ is only called on the last release.

I think some more analysis is needed here to fully understand which objects have a reference to the child and when it is deleted.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +67,5 @@
>      gRtspChildLog = PR_NewLogModule("nsRtspChild");
>  #endif
>    AddIPDLReference();
>    gNeckoChild->SendPRtspControllerConstructor(this);
> +  AddRefForParent();

Ah, something else is wrong here, I think.

RtspControllerChild::AddIPDLReference() already adds a ref for the parent, so we don't need AddRefForParent().

@@ +143,5 @@
>    if (mListener) {
>      mListener->OnDisconnected(index, reason);
>    }
> +  ReleaseIPDLReference();
> +  ReleaseForParent();

ReleaseIPDLReference is called in NeckoChild::Dealloc~ so I don't think you need another one here. Plus, this would help if the problem was a leaking child, not a use-after-free.

@@ +250,5 @@
>  RtspControllerChild::Play(void)
>  {
>    LOG(("RtspControllerChild::Play()"));
>    NS_ENSURE_TRUE(mIPCOpen, NS_ERROR_FAILURE);
> +  NS_ENSURE_FALSE(mIsStopped, NS_ERROR_FAILURE);

You *may* be able to merge mIPCOpen and mIsStopped - it seems like they're used in a lot of the same places.

Comment 8

[Jason Smith [:jsmith]]

RTSP video is being moved to 1.4, so moving to backlog.

Comment 9

[Ken Chang[:kenkai]]

Lets focus on qualities issues before 3/17. Thanks.

Comment 10

[Ethan Tseng [:ethan]]

Attachment: Pause after Stopping RtspMediaResource.svg

This sequence diagram depicts the scenario of stopping RTSP media from the front-end (such as exiting the video app). It shows an additional RtspControllerChild::Pause() is called after Stop(), which might be a trouble for RtspController.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: Close RtspMediaResource on NetworkError.svg

This sequence diagram depicts another category of stopping RTSP media: Closing RTSP due to network error.

Comment 12

[Ethan Tseng [:ethan]]

Attachment: bug-973840-wip.patch

An updated WIP patch.

Comment 13

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #7)
Hi Steve,
Yes, from your comments I realized Add|ReleaseIPDLReference already do the jobs of Add|RefForParent.
I removed those unnecessary Add|RefForParent codes in the previous patch.

After more investigation on this bug, I found the real problem is more tricky than I thought.
We are dealing with two main issues around here:
1) Resource leak
2) Missing actor in e10s

First, before bug 964132 was landed, RtspMediaResource and RtspMediaResource::Listener have circular references. The only way to break this cycle is in Listener::Revoke(), which is only called by ~RtspMediaResource (and it will never be called). So the cycle was never broken and consequently we have a resource leak there.
When bug 964132 removed this cycle reference, ~RtspMediaResource would be called by ~MediaDecoder (the owner of RtspMediaResource), and since RtspMediaResource owns RtspControllerChild, its destruction would also destroy RtspControllerChild.

Issue 2) happens when RtspControllerChild was destroyed but an IPDL message from parent side is still on the fly.
I try to explain the situation in attachment 8381214 [details].
When we exit the video app, i.e., the media document, RtspControllerChild::Stop() and Pause() would be called by main thread and decoder thread respectively. The Stop() will propagate to the parent side to tear down the RTSP connection and reply an OnDisconnected event back to the child. 
At this moment, the child could possibly already be destructed by RtspMediaResource, like I said in the last paragraph. Even if the child is alive now, another IPC event trigger by Pause() would still come and encounter the MsgRouteError problem.

In the meanwhile, I realize the similar problem would occur when the RTSP connection is disconnected on network error.
Please see attachment 8381215 [details].
This time the OnDisconnected event is actively triggered from RtspControllerParent due to some kind of network error.
When the event propagates to MediaResource/Decoder, in the end it will invoke RtspControllerChild::Stop() again and send an IPC event to a non-existing actor. (This bug doesn't occur in v1.3 because of the resource leak of RtspMediaResource!)

In order to fix these issues, I introduce a new flag to determine whether Stop or OnDisconnected event is happened.
The intention of the mIsStopped in my previous patch is almost the same. It's different from mIPCOpen. Its purpose is prevent improper IPC events to be sent. Maybe you have better idea of how to avoid this kind of problem.
I also restore the cycle reference in RtspMediaResource and break it in RtspMediaResource::OnDisconnected().

I feel we need more careful consideration for the e10s of RtspController.
This patch demonstrates what I can think of right now. Hope you can help to work out a better solution. :)

Comment 14

[Ethan Tseng [:ethan]]

Attachment: bug-973840-wip.patch

Fixed a typo in a comment.

Comment 15

[Ethan Tseng [:ethan]]

Since we cannot land RTSP fixes into v1.4, we decided to back out bug 964132, which caused this regression bug. It shall be done after 2/28 and before 3/17.

Steve, once we back out bug 964132, this regression bug would disappear but leaving the underground resource leak from RTSP stuff. I will file a new bug to track and fix these resource leak problems.

Comment 16

[Ethan Tseng [:ethan]]

I will move the patch in this bug to the new bug dedicated to fix RTSP resource leaks.

BTW, while working on this bug, I discovered there are also resource leaks in the RTSP parent side. I will fix them in the same new bug.

Comment 17

[Ethan Tseng [:ethan]]

Comment on attachment 8381464 [details] [diff] [review]
bug-973840-wip.patch

This bug won't fix anything. It will be used to track the task to back out bug 964132.

Comment 18

[Jason Smith [:jsmith]]

Ethan - Could this crash potentially happen with audio RTSP streams?

Comment 19

[Ethan Tseng [:ethan]]

(In reply to Jason Smith [:jsmith] from comment #18)
> Ethan - Could this crash potentially happen with audio RTSP streams?
Yes, it could.

Comment 20

[Jason Smith [:jsmith]]

(In reply to Ethan Tseng [:ethan] from comment #19)
> (In reply to Jason Smith [:jsmith] from comment #18)
> > Ethan - Could this crash potentially happen with audio RTSP streams?
> Yes, it could.

Okay - back to nom then as this can impact RTSP audio streams.

Comment 21

[Preeti Raghunath(:Preeti)]

Blocking 1.4 for RTSP audio stream

Comment 22

[Ethan Tseng [:ethan]]

Attachment: bug-973840-fix.patch

Hi Steve, since bug 964132 only changed one line of code. I think it's easier to back out that change with a new patch. Thus we don't need to involve a more complicated back-out process.
Do you think this is fine?

Comment 23

[Jason Duell]

> The intention of the mIsStopped in my previous patch is almost the same. 
> It's different from mIPCOpen. Its purpose is prevent improper IPC events
> to be sent. Maybe you have better idea of how to avoid this kind of problem.

The way we handle this for HTTP/FTP/etc is by keeping a "mIPCClosed" variable, which we set to true in Parent::ActorDestroy (which is called whenever the other end of an IPDL connection goes away):

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#67

We then check mIPCClosed before every call to any SendFOO function on the parent (we don't bother checking on the child, because in the necko channel case, if the parent has gone away, it would be because the parent process has crashed:  RTSP might be different?)

Comment 24

[Jason Duell]

> It's different from mIPCOpen

Sorry, didn't see that.  Yes, if your criteria is not as simple as "IPC channel still open" then maybe you need a separate variable.

Comment 25

[Ethan Tseng [:ethan]]

(In reply to Jason Duell (:jduell) from comment #24)
Hi Jason,
Thanks for your feedback. We moved the previous works in this bug to another one (the reason is stated in comment 15).
Bug 977518 - [RTSP] Resource leak from RtspMediaResource, RtspController and RTSPSource.

Comment 26

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8384484 [details] [diff] [review]
bug-973840-fix.patch

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

Yup, I agree that the best fix at this stage is to revert the previous change.

(Apologies again for taking so long to review this!)

Comment 27

[Ethan Tseng [:ethan]]

The result of TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=f5b6e6f95b26

Comment 28

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #26) 
> Yup, I agree that the best fix at this stage is to revert the previous
> change.
Thanks, Steve!

Comment 29

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/404edd4f2130

Comment 30

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/404edd4f2130