1135304 - Fix HTMLMediaElement destruction

Status: RESOLVED INVALID

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #1128357 +++

This bug is created based on Bug 1128357 comment 46. The following is its copy.

------------------------------------------------------------------

The following test failure was caused by incorrect HTMLMediaElement's destruction. MediaDecoder have a dependency to HTMLMediaElement::mAudioTrackList and HTMLMediaElement::mVideoTrackList. They are unlinked from HTMLMediaElement before HTMLMediaElement::~HTMLMediaElement() call. When there is a MediaDecoder exist during HTMLMediaElement::~HTMLMediaElement(), it create dangling pointer. And it could cause the following crash.

> PROCESS-CRASH | dom/media/test/test_video_to_canvas.html | application crashed [@ mozilla::dom::MediaTrackList::~MediaTrackList()]

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix HTMLMediaElement destruction

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix HTMLMediaElement destruction

Comment 3

[Sotaro Ikeda [:sotaro]]

To fix the dangling pointer problem, the following code needs to be moved from HTMLMediaElement::~HTMLMediaElement() to CYCLE_COLLECTION_UNLINK macro, because in the macro, mAudioTrackList and mVideoTrackList are removed.

> if (mDecoder) {
>    ShutdownDecoder();
>  }

Comment 4

[Sotaro Ikeda [:sotaro]]

The following function call in HTMLMediaElement::~HTMLMediaElement() is also wired. mMediaSource is set to nullptr in CYCLE_COLLECTION_UNLINK macro

>  if (mMediaSource) {
>    mMediaSource->Detach();
>    mMediaSource = nullptr;
>  }

Even when we do not call it in the CYCLE_COLLECTION_UNLINK macro. If we call ShutdownDecoder() in the CYCLE_COLLECTION_UNLINK macro, mMediaSource->Detach() is called via MediaSourceDecoder::Shutdown().

Comment 5

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a420b865ace8

Comment 6

[Sotaro Ikeda [:sotaro]]

A lot of test failures related to MES. Current MES implementation seems not expecting mMediaSource->Detach() is called from HTMLMediaElement.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix HTMLMediaElement destruction

Comment 8

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2410cd07124c

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2410cd07124c

Almost all test failure seems to become fixed. But the following crash exist.

> xul.dll!mozilla::MP4Reader::UpdateIndex() [MP4Reader.cpp:2410cd07124c : 994 + 0x7]

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix HTMLMediaElement destruction

Comment 11

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e1f5b80969

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8567774 [details] [diff] [review]
patch - Fix HTMLMediaElement destruction

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

Karl, did you want to provide feedback here? You've been looking at the media stack shutdown code and cycles recently?

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8567774 [details] [diff] [review]
patch - Fix HTMLMediaElement destruction

How do you know that Unlink() will be called on the HTMLMediaElement?
Is there a cycle that is guaranteed to exist?

The intention is that MediaDecoderReader is used on the decode task queue,
while TrackBuffer->Detach() is called on the main thread.  TrackBuffer is a
multi-thread class, parts of which are main-thread only, but the parts called
from the decode task queue are meant to be safe to call on those threads, even after the main thread objects are destroyed.

Why is it necessary to Detach() TrackBuffers from the reader?
Is a TrackBuffer being separated from its MediaSource before Detach() is
called?

I wonder whether calling MediaDecoder::Shutdown() multiple times might be
causing problems.  Can that be avoided by clearing the reference when
Shutdown() is called?

(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> The following test failure was caused by incorrect HTMLMediaElement's
> destruction. MediaDecoder have a dependency to
> HTMLMediaElement::mAudioTrackList and HTMLMediaElement::mVideoTrackList.
> They are unlinked from HTMLMediaElement before
> HTMLMediaElement::~HTMLMediaElement() call. When there is a MediaDecoder
> exist during HTMLMediaElement::~HTMLMediaElement(), it create dangling
> pointer. And it could cause the following crash.

What member holds the dangling pointer? Can you link to it please? 

> > PROCESS-CRASH | dom/media/test/test_video_to_canvas.html | application crashed [@ mozilla::dom::MediaTrackList::~MediaTrackList()]

I don't understand the cause of this crash, but, if you can point me to the dangling pointer, that might help me understand.

Removing f? for now, but please re-request with explanation.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Karl Tomlinson (:karlt) from comment #13)
> 
> > > PROCESS-CRASH | dom/media/test/test_video_to_canvas.html | application crashed [@ mozilla::dom::MediaTrackList::~MediaTrackList()]
> 
> I don't understand the cause of this crash, but, if you can point me to the
> dangling pointer, that might help me understand.
> 
> Removing f? for now, but please re-request with explanation.

Sorry, I make a mistake by the following, it caused the problem. It made MediaElement::VideoTracks() and MediaElement::AudioTracks() were called from HTMLMediaElement::~HTMLMediaElement(). It make HTMLMediaElement()'s reference count from 0 to > 0, even when HTMLMediaElement was delete.

----------------------------------------------------

 void MediaDecoder::ChangeState(PlayState aState)
 {
   MOZ_ASSERT(NS_IsMainThread());
   ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
 
@@ -1288,17 +1299,17 @@ void MediaDecoder::ChangeState(PlayState aState)
   }
 
   DECODER_LOG("ChangeState %s => %s",
               gPlayStateStr[mPlayState], gPlayStateStr[aState]);
   mPlayState = aState;
 
   if (mPlayState == PLAY_STATE_PLAYING) {
     ConstructMediaTracks();
-  } else if (mPlayState == PLAY_STATE_ENDED) {
+  } else if (IsEndedState()) {
     RemoveMediaTracks();

-----------------------------------------------

Comment 15

[Sotaro Ikeda [:sotaro]]

Though, calling following from HTMLMediaElement::~HTMLMediaElement() seems to have same risk. But this is a different problem.

  if (mSrcStream) {
    EndSrcMediaStreamPlayback();
  }

Comment 16

[Sotaro Ikeda [:sotaro]]

Sorry, asking a review to the incorrect bug from my misunderstanding.