Closed Bug 1135304 Opened 5 years ago Closed 5 years ago

Fix HTMLMediaElement destruction

Categories

(Core :: Audio/Video, defect)

x86_64
Windows Vista
defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ 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()]
No longer blocks: MSE, 1050031, 1123535, 1134523
No longer depends on: 1128357, 1133167
Blocks: 1128357
Assignee: nobody → sotaro.ikeda.g
Blocks: 1050031
Priority: P1 → --
Attachment #8567409 - Attachment is obsolete: true
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();
>  }
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().
A lot of test failures related to MES. Current MES implementation seems not expecting mMediaSource->Detach() is called from HTMLMediaElement.
Attachment #8567520 - Attachment is obsolete: true
(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]
Attachment #8567654 - Attachment is obsolete: true
Attachment #8567774 - Flags: review?(cpearce)
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?
Attachment #8567774 - Flags: feedback?(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.
Attachment #8567774 - Flags: feedback?(karlt)
Flags: needinfo?(sotaro.ikeda.g)
(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();

-----------------------------------------------
Flags: needinfo?(sotaro.ikeda.g)
Though, calling following from HTMLMediaElement::~HTMLMediaElement() seems to have same risk. But this is a different problem.

  if (mSrcStream) {
    EndSrcMediaStreamPlayback();
  }
Sorry, asking a review to the incorrect bug from my misunderstanding.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Attachment #8567774 - Flags: review?(cpearce)
No longer blocks: 1050031
You need to log in before you can comment on or make changes to this bug.