898864 - RTSP content process crash when press the play button at the end of video.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Benjamin Chen [:bechen]]

STR:
1. play a RTSP video
2. seek/play to the end
3. press the play button

RTSP content process crash by receiving SIGTERM.
Maybe at the end of video, the connection to RTSP server is closed(receive bye or tear down). And the RTSP content still wants to ipc to parent which is destroyed.

Comment 1

[Ken Chang[:kenkai]]

Have we fixed this bug? If not, is it okay for you to mark it ask 1.3+?

Comment 2

[Vincent Chang[:vchang][changyihsin]]

The reason here is that the return value of IPC is false and cause the content process crash. I have a WIP patch on handle but the read thread of decoder state machine in content process seems not be able to activated.

Comment 3

[Vincent Chang[:vchang][changyihsin]]

Attachment: rtsp.wip.patch

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 830186 [details] [diff] [review]
rtsp.wip.patch

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

Thanks for Vincent's help.

This patch resolve a deadlock issue when we are playing a rtsp stream to the end, then press play button again.
The function not return without release the mBuffer in RtspMediaSource::read.

::: content/media/RtspMediaResource.cpp
@@ +193,5 @@
>        RTSPMLOG("BUFFER_SLOT_INVALID move forward");
>      } else {
>        // No data, and disconnected.
>        if (!mIsStarted) {
> +        aReadCount = 0;

Remove this line.

::: content/media/omx/RtspOmxReader.cpp
@@ +124,5 @@
>      rv = mRtspResource->ReadFrameFromTrack((uint8_t *)mBuffer->data(),
>                                             mFrameMaxSize, mTrackIdx, readCount,
>                                             time, actualFrameSize);
> +    if (NS_FAILED(rv)) {
> +      mBuffer->set_range(0, readCount);

Remove this line.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

This patch tries to fix, 
1. Release the memory held by mTracks, so that we could get the correct track for the follow up connection.
2. Instead of return false in IPC call, we should always return true to prevent problem be terminated. I also send the ondisconnect event to notify caller the error. 
3. Release the decoder when receiving ondisconnect event. It forces reload the resource and everything starts from scratch.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 831446 [details] [diff] [review]
Patch v1.0

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

Just some clarifications needed. Almost there.

::: content/media/MediaDecoder.cpp
@@ +823,5 @@
> +  if (mOwner) {
> +    mOwner->UpdateNetworkState();
> +  }
> +
> +  Shutdown();

Shutdown() is pretty serious. Please add a comment to say how the decoder will be initialized again.

::: content/media/MediaDecoder.h
@@ +250,5 @@
>  
>    MediaDecoder();
>    virtual ~MediaDecoder();
>  
> +  virtual void UpdateNetworkState();

Please add a comment to say what this function does.

::: content/media/MediaDecoderOwner.h
@@ +138,5 @@
>  
>    // Called by the media decoder and the video frame to get the
>    // ImageContainer containing the video data.
>    virtual VideoFrameContainer* GetVideoFrameContainer() = 0;
> +  virtual void UpdateNetworkState() = 0;

Please add a comment to say what implementations of this function should do.

Also, the name is misleading. "Update" suggests that network state should be determined and state variables updated; without params it seems like it's up to the function to determine the status. Also, "network" suggests the whole network to me (probably because I work in Necko :) ).

The implementation of this function seems to be about resetting the decoder and media element if the connection goes down. So, how about ResetConnectionState instead?

::: content/media/RtspMediaResource.cpp
@@ +529,5 @@
>  
>    if (aReason == (uint32_t)NS_ERROR_CONNECTION_REFUSED) {
>      mDecoder->NetworkError();
> +  } else {
> +    mDecoder->UpdateNetworkState();

Please review this and confirm that you only want NS_ERROR_CONNECTION_REFUSED to result in NetworkError. There are other network-related nsresult values that might warrant the decoder shutting down in error. Please see ErrorList.h.

I'd also like to see this written as

if (aReason == (uint32_t)NS_ERROR_CONNECTION_REFUSED) {
  mDecoder->NetworkError();
  return NS_OK;
}

mDecoder->ResetConnectionState();
return NS_OK;

It is just a little bit clearer that NetworkError is an exception, whereas ResetConnectionState is the normal case. Helpful for future reading.

You might also want to add either a LOG or an NS_WARN_IF_FALSE(NS_FAILED(rv), nsPrintfCString("Error in OnDisconnected %x", rv).get());
I would choose LOG, unless you want to always track failure cases.

::: content/media/omx/RtspOmxReader.cpp
@@ +128,5 @@
> +      // Release the acquired buffer.
> +      stop();
> +      start();
> +      return ERROR_CONNECTION_LOST;
> +    }

When you remove NS_ENSURE_SUCCESS there will be no logs on the console. Please add NS_WARNING("ReadFrameFromTrack failed; releasing buffers and returning."

Why do you need to call start() again here? Please add a comment to explain because it looks like this is a fatal error for the session.

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +76,5 @@
>    NS_ENSURE_TRUE(mController, NS_ERROR_NOT_INITIALIZED);
>  
>    nsresult rv = mController->Play();
> +  if (NS_FAILED(rv) && mIPCOpen) {
> +    for (int i = 0; mTotalTracks && i < mTotalTracks; i++) {

if (NS_FAILED(rv) && mIPCOpen && mTotalTracks > 0) {
  for (int i = 0; i < mTotalTracks; i++) {

You should only need to check mTotalTracks once, and it should be positive, not just non-zero.

Or, if mTotalTracks should be > 0, then please add this as an NS_ENSURE_TRUE after NS_ENSURE_TRUE(mController...).

@@ +207,5 @@
>    nsresult rv = meta->GetTotalTracks(&int32Value);
>    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> +  if (mTotalTracks < 0) {
> +    mTotalTracks = int32Value;
> +  }

Do you need int32value here? Can you just use mTotalTracks?

Also, can OnConnected be called more than once? In this patch, mTotalTracks will only be set one time - you may need to see it every time OnConnected is called.

Should you set mTotalTracks to -1 in OnDisconnected?

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

Hi sworkman, thanks for your help to review this patch. I have addressed the review comments and post the new patch.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8334438 [details] [diff] [review]
Patch v1.1

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

Good work! Thanks for making those changes, especially adding the comments - it makes the code a lot easier to read and understand.

Some minor changes requested, but the patch looks good: r=me with those changes.

::: content/html/content/public/HTMLMediaElement.h
@@ +368,5 @@
>    {
>      return mNetworkState;
>    }
>  
> +  void ResetConnectionState();

Just to be extra clear, please add the comment from MediaDecoderOwner. Also, please add MOZ_OVERRIDE MOZ_FINAL here and other places you have overridden functions.

::: content/media/MediaDecoderOwner.h
@@ +143,5 @@
> +  // when the resource has a network error during loading.
> +
> +  // Called by the media decoder object, on the main thread,
> +  // when the connection between Rtsp server and client gets lost.
> +  virtual void ResetConnectionState() = 0;

Oops - looks like you have two comments for the same function prototype here :)

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +79,5 @@
> +  if (NS_FAILED(rv) && mIPCOpen && mTotalTracks > 0) {
> +    for (int i = 0; i < mTotalTracks; i++) {
> +      SendOnDisconnected(i, rv);
> +    }
> +  }

These five lines are repeated a lot, so you could create either a macro or a separate function for them.

Comment 9

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.2

Update the patch to address the review comments

Comment 10

[Vincent Chang[:vchang][changyihsin]]

https://tbpl.mozilla.org/?tree=Try&rev=83c0df16bf5b

Comment 11

[Vincent Chang[:vchang][changyihsin]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a94878b0a97b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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