1275201 - Race condition between MediaStreamTrack ending and MediaTrack being removed when playing a MediaStream in a HTMLMediaElement

Status: RESOLVED DUPLICATE of bug 1274221

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

In MediaStreamTrack::NotifyEnded, no need to dispatch to main thread when it is already in main thread. Or it will bump to a timing issue.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1275201 - In MediaStreamTrack::NotifyEnded, no need to dispatch to main thread when it is already in main thread. r=jesup, persons

Review commit: https://reviewboard.mozilla.org/r/54774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54774/

Comment 2

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8755742 [details]
MozReview Request: Bug 1275201 - In MediaStreamTrack::NotifyEnded, no need to dispatch to main thread when it is already in main thread. r=jesup, persons

https://reviewboard.mozilla.org/r/54774/#review51444

::: dom/media/MediaStreamTrack.cpp:378
(Diff revision 1)
>  void
>  MediaStreamTrack::NotifyEnded()
>  {
> +  if (NS_IsMainThread()) {
> +    NotifyEndedImpl();
> +  } else {
> -  NS_DispatchToMainThread(NewRunnableMethod(this, &MediaStreamTrack::NotifyEndedImpl));
> +    NS_DispatchToMainThread(NewRunnableMethod(this, &MediaStreamTrack::NotifyEndedImpl));
> -}
> +  }
> +}

The spec for MediaStreamTrack says that a track ending should fire a task (this dispatch) that sets the readyState attribute to "ended" and raises the "ended" event synchronously.

We should fix the other race contestant instead.

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

This is a bug of bug 1208373. And bug 1208373 is backed out. I already talked with andreas on IRC. He might fix it before landing.

Comment 4

[Andreas Pehrson [:pehrsons]]

Could you check if the latest set of patches on bug 1274221 fixes it for you?

Comment 5

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #4)
> Could you check if the latest set of patches on bug 1274221 fixes it for you?

Ugh, that was a different issue. My brain is making shortcuts today :-)

Could you check with bug 1208373 and bug 1274221 on top?

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #5)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #4)
> > Could you check if the latest set of patches on bug 1274221 fixes it for you?
> 
> Ugh, that was a different issue. My brain is making shortcuts today :-)
> 
> Could you check with bug 1208373 and bug 1274221 on top?

Andreas,
I did two experiments, one is test the patch of bug 126646 on top of bug 1208373 and bug 1274221 and another is only on top of "Bug 1274221 - Make FindOwnedDOMTrack look for (optionally) unique tracks.".
Both pass the assert in bug 126646.

So if you can land bug 1208373 soon, then bug 126646 can wait until bug 1208373 and bug 1274221 landing. Or you can land "Bug 1274221 - Make FindOwnedDOMTrack look for (optionally) unique tracks." first. Then I can land bug 126646. And you can combine "Ensure MediaStreamListeners are always notified of created and ended tracks." with bug 1208373. The second approach might be better since if bug 1208373 is backed out again, then the bug 126646 no need to be backed out too. So how do you think?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8755742 [details]
MozReview Request: Bug 1275201 - In MediaStreamTrack::NotifyEnded, no need to dispatch to main thread when it is already in main thread. r=jesup, persons

https://reviewboard.mozilla.org/r/54774/#review52690

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

FYI, I'm good with whatever Andreas thinks works here

Comment 9

[Andreas Pehrson [:pehrsons]]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #5)
> > (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #4)
> > > Could you check if the latest set of patches on bug 1274221 fixes it for you?
> > 
> > Ugh, that was a different issue. My brain is making shortcuts today :-)
> > 
> > Could you check with bug 1208373 and bug 1274221 on top?
> 
> Andreas,
> I did two experiments, one is test the patch of bug 126646 on top of bug
> 1208373 and bug 1274221 and another is only on top of "Bug 1274221 - Make
> FindOwnedDOMTrack look for (optionally) unique tracks.".
> Both pass the assert in bug 126646.
> 
> So if you can land bug 1208373 soon, then bug 126646 can wait until bug
> 1208373 and bug 1274221 landing. Or you can land "Bug 1274221 - Make
> FindOwnedDOMTrack look for (optionally) unique tracks." first. Then I can
> land bug 126646. And you can combine "Ensure MediaStreamListeners are always
> notified of created and ended tracks." with bug 1208373. The second approach
> might be better since if bug 1208373 is backed out again, then the bug
> 126646 no need to be backed out too. So how do you think?

You mean bug 1266646?

Well, bug 1274221 is built on bug 1208373 so I intend to land them together. They'll probably go in together with bug 1273136 since that's blocking.

Comment 10

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6)
> > (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #5)
> > > (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #4)
> > > > Could you check if the latest set of patches on bug 1274221 fixes it for you?
> > > 
> > > Ugh, that was a different issue. My brain is making shortcuts today :-)
> > > 
> > > Could you check with bug 1208373 and bug 1274221 on top?
> > 
> > Andreas,
> > I did two experiments, one is test the patch of bug 126646 on top of bug
> > 1208373 and bug 1274221 and another is only on top of "Bug 1274221 - Make
> > FindOwnedDOMTrack look for (optionally) unique tracks.".
> > Both pass the assert in bug 126646.
> > 
> > So if you can land bug 1208373 soon, then bug 126646 can wait until bug
> > 1208373 and bug 1274221 landing. Or you can land "Bug 1274221 - Make
> > FindOwnedDOMTrack look for (optionally) unique tracks." first. Then I can
> > land bug 126646. And you can combine "Ensure MediaStreamListeners are always
> > notified of created and ended tracks." with bug 1208373. The second approach
> > might be better since if bug 1208373 is backed out again, then the bug
> > 126646 no need to be backed out too. So how do you think?
> 
> You mean bug 1266646?
> 
> Well, bug 1274221 is built on bug 1208373 so I intend to land them together.
> They'll probably go in together with bug 1273136 since that's blocking.

Yes, 1266646...

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

This bug is already fixed in bug 1274221.