1100803 - [b2g] The repeat deleting to the port of the OutputStreamData

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alastor Wu [:alwu]]

STR.
1. goto http://people.mozilla.org/~rlin/MediaRecorder.html
2. play VP9/720WebM/352WebM
3. refresh browser
4. continually repeat step 2 & 3, crash

Comment 1

[Alastor Wu [:alwu]]

dump the callstack on flame 2.2 

(gdb) bt
#0  mozilla::MediaInputPort::Destroy (this=0x0) at ../../../../firefox/dom/media/MediaStreamGraph.cpp:2615
#1  0xb55390ea in mozilla::MediaDecoder::DestroyDecodedStream (this=0xb31d9f20) at ../../../../firefox/dom/media/MediaDecoder.cpp:294
#2  0xb553c818 in Shutdown (this=0xb31d9f20) at ../../../../firefox/dom/media/MediaDecoder.cpp:484
#3  mozilla::MediaDecoder::Shutdown (this=0xb31d9f20) at ../../../../firefox/dom/media/MediaDecoder.cpp:473
#4  0xb54f72ea in mozilla::dom::HTMLMediaElement::ShutdownDecoder (this=this@entry=0xb1c3d880) at ../../../../firefox/dom/html/HTMLMediaElement.cpp:609
#5  0xb54f7b42 in mozilla::dom::HTMLMediaElement::~HTMLMediaElement (this=0xb1c3d880, __in_chrg=<optimized out>) at ../../../../firefox/dom/html/HTMLMediaElement.cpp:2053
#6  0xb5508e7a in mozilla::dom::HTMLVideoElement::~HTMLVideoElement (this=0xb1c3d880, __in_chrg=<optimized out>) at ../../../../firefox/dom/html/HTMLVideoElement.cpp:51
#7  0xb5508e8c in mozilla::dom::HTMLVideoElement::~HTMLVideoElement (this=0xb1c3d880, __in_chrg=<optimized out>) at ../../../../firefox/dom/html/HTMLVideoElement.cpp:51
#8  0xb4f9a0d2 in InMemoryDataSource::DeleteCycleCollectable (this=<optimized out>) at ../../../../firefox/rdf/base/nsRDFXMLDataSource.cpp:453
#9  0xb514831c in mozilla::dom::FragmentOrElement::cycleCollection::DeleteCycleCollectable (this=<optimized out>, p=<optimized out>) at ../../dist/include/mozilla/dom/FragmentOrElement.h:235
#10 0xb4cf88f2 in SnowWhiteKiller::~SnowWhiteKiller (this=0xbe9c6f44, __in_chrg=<optimized out>) at ../../../../firefox/xpcom/base/nsCycleCollector.cpp:2646
#11 0xb4cf899c in nsCycleCollector::FreeSnowWhite (this=0xb6aaa000, aUntilNoSWInPurpleBuffer=aUntilNoSWInPurpleBuffer@entry=false) at ../../../../firefox/xpcom/base/nsCycleCollector.cpp:2820
#12 0xb4cf89c6 in nsCycleCollector_doDeferredDeletion () at ../../../../firefox/xpcom/base/nsCycleCollector.cpp:4201
#13 0xb4f6490c in AsyncFreeSnowWhite::Run (this=0xb6a810c0) at ../../../../../firefox/js/xpconnect/src/XPCJSRuntime.cpp:226
#14 0xb4d14e78 in nsThread::ProcessNextEvent (this=0xb6a02710, aMayWait=<optimized out>, aResult=0xbe9c7017) at ../../../../firefox/xpcom/threads/nsThread.cpp:830
#15 0xb4d22aa6 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=false) at /home/alastor/firefox/xpcom/glue/nsThreadUtils.cpp:265
#16 0xb4e66d2c in mozilla::ipc::MessagePump::Run (this=0xb6a01eb0, aDelegate=0xbe9c7118) at ../../../../firefox/ipc/glue/MessagePump.cpp:99
#17 0xb4e5b654 in MessageLoop::RunInternal (this=this@entry=0xbe9c7118) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233
#18 0xb4e5b708 in RunHandler (this=0xbe9c7118) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226
#19 MessageLoop::Run (this=0xbe9c7118) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200
#20 0xb56f9e06 in nsBaseAppShell::Run (this=0xb3930520) at ../../../firefox/widget/nsBaseAppShell.cpp:164
#21 0xb5a150d2 in XRE_RunAppShell () at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:731
#22 0xb4e5b654 in MessageLoop::RunInternal (this=this@entry=0xbe9c7118) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233
#23 0xb4e5b708 in RunHandler (this=0xbe9c7118) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226
#24 MessageLoop::Run (this=this@entry=0xbe9c7118) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200
#25 0xb5a1550a in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>, aGMPLoader=<optimized out>) at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:568
#26 0xb4f3d810 in content_process_main (argc=8, argc@entry=9, argv=argv@entry=0xb6a01ca0) at ../../../../firefox/ipc/contentproc/plugin-container.cpp:190
#27 0xb4e67f8e in mozilla::ipc::ProcLoaderLoadRunner::DoWork (this=<optimized out>) at ../../../../firefox/ipc/glue/ProcessUtils_linux.cpp:415
#28 0xb4e65ea6 in ProcLoaderServiceRun (aReservedFds=..., aArgv=<optimized out>, aArgc=1, aFd=<optimized out>, aPeerPid=32340) at ../../../../firefox/ipc/glue/ProcessUtils_linux.cpp:574
#29 XRE_ProcLoaderServiceRun (aPeerPid=32340, aFd=<optimized out>, aArgc=1, aArgv=<optimized out>, aReservedFds=...) at ../../../../firefox/ipc/glue/ProcessUtils_linux.cpp:608
#30 0x00011314 in RunProcesses (aReservedFds=..., argv=0xbe9c7c84, argc=1) at ../../../../firefox/b2g/app/B2GLoader.cpp:219
#31 main (argc=1, argv=0xbe9c7c84) at ../../../../firefox/b2g/app/B2GLoader.cpp:290

Comment 2

[Alastor Wu [:alwu]]

It seems that there are another crashes related to this bug. They all have similar error backtrace. 

Case 1.
STR.
1. goto http://people.mozilla.org/~rlin/MediaRecorder.html
2. Play 720PWebM, then put the phone to sleep
3. Wake up the phone & unlock the device
4. The video becomes a loading state (It seems abnormal), then put the phone to sleep
5. Repeat step 3, then crash happens

Case 2.
STR.
1. goto http://people.mozilla.org/~rlin/MediaRecorder.html
2. Play 720PWebM, then put the phone to sleep
3. Wake up the phone & unlock the device
4. The video becomes a loading state , then play 720PWebM again
5. Crash

Comment 3

[Alastor Wu [:alwu]]

Here are their callback, and there are some differences after the #27. 

#0  mozilla::MediaInputPort::Destroy (this=0x0) at ../../../../firefox/dom/media/MediaStreamGraph.cpp:2619
#1  0xb597f474 in mozilla::MediaDecoder::DestroyDecodedStream (this=0xb1246040) at ../../../../firefox/dom/media/MediaDecoder.cpp:301
#2  0xb597f4d6 in mozilla::MediaDecoder::SetDormantIfNecessary (this=0xb1246040, aDormant=<optimized out>) at ../../../../firefox/dom/media/MediaDecoder.cpp:142
#3  0xb59405e6 in mozilla::dom::HTMLMediaElement::NotifyOwnerDocumentActivityChanged (this=this@entry=0xb43bdda0) at ../../../../firefox/dom/html/HTMLMediaElement.cpp:3470
#4  0xb59565cc in mozilla::dom::HTMLVideoElement::NotifyOwnerDocumentActivityChanged (this=0xb43bdda0) at ../../../../firefox/dom/html/HTMLVideoElement.cpp:188
#5  0xb55c5f9a in NotifyActivityChanged (aSupports=0xb43bdda0, aUnused=<optimized out>) at ../../../../firefox/dom/base/nsDocument.cpp:4492
#6  0xb55bad1c in EnumerateObservers (aEntry=<optimized out>, aData=<optimized out>) at ../../../../firefox/dom/base/nsDocument.cpp:9881
#7  0xb55baf9e in nsTHashtable<nsPtrHashKey<nsISupports> >::s_EnumStub (aTable=<optimized out>, aEntry=<optimized out>, aNumber=<optimized out>, aArg=<optimized out>)
    at ../../dist/include/nsTHashtable.h:496
#8  0xb516e058 in Enumerate (aArg=0xbebf2f28, aEtor=0xb55baf95 <nsTHashtable<nsPtrHashKey<nsISupports> >::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, this=0xb1a538a0)
    at /home/alastor/firefox/xpcom/glue/pldhash.cpp:723
#9  PL_DHashTableEnumerate (aTable=0xb1a538a0, aEtor=0xb55baf95 <nsTHashtable<nsPtrHashKey<nsISupports> >::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, aArg=aArg@entry=0xbebf2f28)
    at /home/alastor/firefox/xpcom/glue/pldhash.cpp:774
#10 0xb55c0e08 in EnumerateEntries (aUserArg=0xbebf2f20, aEnumFunc=0xb55bad13 <EnumerateObservers(nsPtrHashKey<nsISupports>*, void*)>, this=<optimized out>) at ../../dist/include/nsTHashtable.h:215
#11 nsIDocument::EnumerateActivityObservers (this=this@entry=0xb1a8f800, aEnumerator=<optimized out>, aData=aData@entry=0x0) at ../../../../firefox/dom/base/nsDocument.cpp:9892
#12 0xb55c125a in UpdateVisibilityState (this=0xb1a8f800) at ../../../../firefox/dom/base/nsDocument.cpp:12142
#13 nsDocument::UpdateVisibilityState (this=0xb1a8f800) at ../../../../firefox/dom/base/nsDocument.cpp:12128
#14 0xb513b60e in nsRunnableMethodImpl<tag_nsresult (nsMemoryReporterManager::*)(), void, true>::Run (this=<optimized out>) at ../../dist/include/nsThreadUtils.h:388
#15 0xb51609b0 in nsThread::ProcessNextEvent (this=0xb4302710, aMayWait=<optimized out>, aResult=0xbebf2fc7) at ../../../../firefox/xpcom/threads/nsThread.cpp:830
#16 0xb516e5de in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=false) at /home/alastor/firefox/xpcom/glue/nsThreadUtils.cpp:265
#17 0xb52ada0c in mozilla::ipc::MessagePump::Run (this=0xb4301dc0, aDelegate=0xbebf30c8) at ../../../../firefox/ipc/glue/MessagePump.cpp:99
#18 0xb52a2334 in MessageLoop::RunInternal (this=this@entry=0xbebf30c8) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233
#19 0xb52a23e8 in RunHandler (this=0xbebf30c8) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226
#20 MessageLoop::Run (this=0xbebf30c8) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200
#21 0xb5b40286 in nsBaseAppShell::Run (this=0xb222e5e0) at ../../../firefox/widget/nsBaseAppShell.cpp:164
#22 0xb5e5b7d2 in XRE_RunAppShell () at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:731
#23 0xb52a2334 in MessageLoop::RunInternal (this=this@entry=0xbebf30c8) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:233
#24 0xb52a23e8 in RunHandler (this=0xbebf30c8) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:226
#25 MessageLoop::Run (this=this@entry=0xbebf30c8) at ../../../../firefox/ipc/chromium/src/base/message_loop.cc:200
#26 0xb5e5bc0a in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>, aGMPLoader=<optimized out>) at ../../../../firefox/toolkit/xre/nsEmbedFunctions.cpp:568
#27 0x00009114 in content_process_main (argc=7, argv=0xbebf3a54) at ../../../../firefox/ipc/app/../contentproc/plugin-container.cpp:190

[Case1]
#28 0xb4e6ac6e in mozilla::ipc::ProcLoaderLoadRunner::DoWork (this=<optimized out>) at ../../../../firefox/ipc/glue/ProcessUtils_linux.cpp:415
#29 0xb4e68b86 in ProcLoaderServiceRun (aReservedFds=..., aArgv=<optimized out>, aArgc=1, aFd=<optimized out>, aPeerPid=206) at ../../../../firefox/ipc/glue/ProcessUtils_linux.cpp:574
#30 XRE_ProcLoaderServiceRun (aPeerPid=206, aFd=<optimized out>, aArgc=1, aArgv=<optimized out>, aReservedFds=...) at ../../../../firefox/ipc/glue/ProcessUtils_linux.cpp:608
#31 0x00011314 in RunProcesses (aReservedFds=..., argv=0xbeb1ec84, argc=1) at ../../../../firefox/b2g/app/B2GLoader.cpp:219
#32 main (argc=1, argv=0xbeb1ec84) at ../../../../firefox/b2g/app/B2GLoader.cpp:290

[Case2]
#28 0xb6e704a4 in __libc_init (raw_args=0xbebf3a50, onexit=<optimized out>, slingshot=0x9131 <main(int, char**)>, structors=<optimized out>) at bionic/libc/bionic/libc_init_dynamic.cpp:112
#29 0x00009014 in _start ()

Comment 4

[Alastor Wu [:alwu]]

Let me describe more about this crash.

========================================================================
Reason : Due to the repeat deleting to the port of the OutputStreamData. 

Something you should know : When the media went into the dormant state, the source and ports were released by |DestroyDecodedStream()|. 

[Crash case]
1. Play video & refresh it 
  Step1: Play a video
  Step2: Refreshing made the media into the dormant state
  Step3: Destroy the ports and decoded-source
  Step4: The decoder shutdown, call |DestroyDecodedStream()| to destroy the ports again 
  Step5: Crash! repeat deleting!
 

2. Play video -> sleep the phone -> wake up the phone -> sleep the phone
  Step1: Play a video
  Step2: Sleep the phone, media into dormant state
  Step3: Destroy the ports and decoded-source
  Step4: Wake up, but not recreate the ports (because the decoded-source didn't exist)
  Step5: Sleep the phone, media into dormant state
  Step6: Destroy the ports and decoded-source (the ports were already released)
  Step7: Crash! repeat deleting!

Comment 5

[Alastor Wu [:alwu]]

Attachment: solveForRepeatDeleting.patch

Now I have a simple patch could fix the crash problem, but there still has a problem. The video does not play after waking up the phone, and it stays in the seek state.

Comment 6

[Alastor Wu [:alwu]]

(In reply to Alastor Wu [:alwu] from comment #5)
> Created attachment 8528260 [details] [diff] [review]
> solveForRepeatDeleting.patch
> 
> Now I have a simple patch could fix the crash problem, but there still has a
> problem. The video does not play after waking up the phone, and it stays in
> the seek state.
 
And there has an error message , "Unexpected error when seeking ", which is resultd from OmxDecoder::ReadVideo(). The result of |mVideoSource->read(..)| is "INFO_FORMAT_CHANGED".

Comment 7

[Andreas Pehrson [:pehrsons]]

I am about to introduce a test to bug 879717 that reproduces this crash at a high rate on B2G.

What it does is essentially shutting down the decoder immediately after loadedmetadata.

See my try here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2da1d9869681

And the relevant log on B2G emulator OPT here:
http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-2da1d9869681/try-emulator/try_ubuntu64_vm-b2g-emulator_test-mochitest-6-bm117-tests1-linux64-build315.txt.gz

Comment 8

[Alastor Wu [:alwu]]

Because of blocking another bug, I think that I should fixed this crash first. So I move the problem, video can't playback after waking from the dormant state, to another bug. See Bug 1105913.

Comment 9

[Alastor Wu [:alwu]]

Attachment: Bug1100803_v1.patch

Hi, JW,
Could you give my patch some suggestions?
I describe some details in the comment4, thanks a lot for your help :)

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8530055 [details] [diff] [review]
Bug1100803_v1.patch

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

::: dom/media/MediaDecoder.cpp
@@ +290,5 @@
>      // its Destroy message before this decoder is destroyed. So we have to
>      // be careful not to send any messages after the Destroy().
>      if (os.mStream->IsDestroyed()) {
>        // Probably the DOM MediaStream was GCed. Clean up.
> +      if (os.mPort) {

You should also do so in MediaDecoder::PlaybackEnded().

@@ +307,2 @@
>    }
> +  if (mShuttingDown) {

What is this for?

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Alastor Wu [:alwu] from comment #4)
> 2. Play video -> sleep the phone -> wake up the phone -> sleep the phone
>   Step1: Play a video
>   Step2: Sleep the phone, media into dormant state
>   Step3: Destroy the ports and decoded-source

What is decoded-source? Can you show me the code?

Comment 12

[Alastor Wu [:alwu]]

(In reply to JW Wang [:jwwang] from comment #10)
> @@ +307,2 @@
> >    }
> > +  if (mShuttingDown) {
> 
> What is this for?

If we release |mDecodedStream| in the dormant state, we can't arouse the function|RecreateDecodedStream(...)| in the |MediaDecoderStateMachine::StartSeek()|.

> What is decoded-source? Can you show me the code?

What I mean is |nsAutoPtr<DecodedStreamData> mDecodedStream| which is a member variable of the MediaDecoder.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8530055 [details] [diff] [review]
Bug1100803_v1.patch

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

::: dom/media/MediaDecoder.cpp
@@ +307,2 @@
>    }
> +  if (mShuttingDown) {

It will leak mDecodedStream if MediaDecoder::RecreateDecodedStream is called when |mShuttingDown| is false.

Comment 14

[Andreas Pehrson [:pehrsons]]

Alastor, are you still looking into this? I guess you might be busy in meetings all of this week.

It is the last bit left blocking bug 879717 after bug 1106963 lands (I hope). Would be great to close it soon.

Comment 15

[Alastor Wu [:alwu]]

Hi, Andreas, 
Sorry for that slow process. I would try to finish it ASAP.

Comment 16

[Alastor Wu [:alwu]]

Attachment: Bug1100803_v2.patch

Hi, JW,
Could you give my patch some suggestions?

The previous starting condition of the |RecreateDecodedStream()| leads to the error that we can't recreate the stream and ports after waking from the dormant state. Because we release the |mDecodedStream| in the dormant state, but we need it to arouse the |RecreateDecodedStream()|.

So I change the condition to ensure we can arouse the |RecreateDecodedStream()| successfully.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8533462 [details] [diff] [review]
Bug1100803_v2.patch

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

::: dom/media/MediaDecoder.cpp
@@ +279,5 @@
>  }
> +
> +bool MediaDecoder::IsNeedToRecreateDecodedStream()
> +{
> +  if (mIsExitingDormant || mDecodedStream) {

It returns true when |mIsExitingDormant| is true even when the media element is not captured at all. We should't create a DecodedStreamData when not captured.

Comment 18

[Alastor Wu [:alwu]]

Attachment: Bug1100803_v3.patch

Hi, JW,

Now I arouse the |RecreateDecodedStream()| depend on whether the media tracks have been created. I use the variable |mMediaTracksConstructed| as a condition. This variable is set to true only when the state is |PLAY_STATE_PLAYING|. Therefore, we could seek the streams when it is true, because we extactly have the media element and played it before.

Thanks your help :)

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8534104 [details] [diff] [review]
Bug1100803_v3.patch

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

You will still create a DecodedStreamData when not captured.

Comment 20

[Alastor Wu [:alwu]]

Attachment: Bug1100803_v4.patch

Hi, JW,
I use the variables, |mSrcStream| and |mOutputStream|, in the HTMLMediaElement to ensure the media element does not read data from files.
Very thanks for your help :)

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8534271 [details] [diff] [review]
Bug1100803_v4.patch

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

::: dom/media/MediaDecoder.cpp
@@ +280,5 @@
> +
> +void MediaDecoder::RecreateDecodedStreamIfNecessary(int64_t aStartTimeUSecs)
> +{
> +  HTMLMediaElement* mediaElement = mOwner->GetMediaElement();
> +  if (mediaElement->IsNecessaryToCreateStream()) {

I think you just need to check |mInitialAudioCaptured| to know if it is captured or not without resorting to the media element.

Comment 22

[Alastor Wu [:alwu]]

Attachment: Bug1100803_v5.patch

Hi, JW,
Here is my revised patch,
Thanks a lot :)

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8534296 [details] [diff] [review]
Bug1100803_v5.patch

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

The patch should be separated into 2 parts. One is for fixing duplicated deletion in comment 5, the other for the timing issue in checking mDecoder->GetDecodedStream() in MediaDecoderStateMachine::StartSeek().

::: dom/media/MediaDecoder.h
@@ +866,5 @@
>    bool IsLogicallyPlaying();
>  
> +  // To Ensure the media element has a need to capture the streams. In somecase,
> +  // it just reads data from the file.
> +  void RecreateDecodedStreamIfNecessary(int64_t aStartTimeUSecs);

The comment is confusing. It should be "Re-create a decoded stream if audio being captured".

Comment 24

[Alastor Wu [:alwu]]

Attachment: Part1 : Avoid media ports repeated deletion

Hi, Robert.
Sorry to bother you, could you help me review my patches?
Thanks a lots :)

Comment 25

[Alastor Wu [:alwu]]

Attachment: Part2 : Recreate media streams

Comment 26

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Alastor Wu [:alwu] from comment #25)
> Created attachment 8534766 [details] [diff] [review]
> Part2 : Recreate media streams

Please describe the problem and what this patch tries to fix. It will make the life of the reviewer easier.

Comment 27

[Alastor Wu [:alwu]]

Hi, Robert,
Here are some descriptions for this bug.
Hope it could be helpful :)

==========================================================
[Problem] 
Media ports repeated deletion

[Situation] 
That could happen in two situations, see Comment4.

[Solution]
Check the existence of media ports, before delete them.

==========================================================

[Problem] 
Media stream can't be recreated 

[Situation] 
When the media go to the dormant state, the |mDecodedStream| would be released. It causes that we can't arouse the |RecreateDecodedStream()| in the |MediaDecoderStateMachine::StartSeek()|.

[Solution]
We use |mInitialAudioCaptured| to ensure that we wouldn't recreate a stream without be captured by media element, and ensure that we indeed have a streams which can be played, be seeked ..e.t.c.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8534766 [details] [diff] [review]
Part2 : Recreate media streams

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

Please add tests for both of these bugs. Thanks!

Comment 29

[Andreas Pehrson [:pehrsons]]

With this bug landing soon, I was trying to be proactive on bug 879717 with a try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=61090a7a7e25

> TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_basicH264Video.html | Test timed out. - expected PASS

That's an issue I haven't seen before. Alastor, do you know if it's triggered by your patches here or mine on bug 879717?

Comment 30

[Alastor Wu [:alwu]]

Hi, Andreas,
Sorry for that I didn't get this problem before, and here is my try result.
Hope it would be helpful :)
https://tbpl.mozilla.org/?tree=Try&rev=b2e256f98679

Comment 31

[Alastor Wu [:alwu]]

Attachment: [checkin] Part1 : Avoid media ports repeated deletion

Comment 32

[Alastor Wu [:alwu]]

Attachment: [checkin] Part2 : Re-create media stream

Comment 33

[JW Wang [:jwwang] [:jw_wang]]

Please also file a following bug to add test cases for this bug. Thanks.

Comment 34

[Alastor Wu [:alwu]]

(In reply to JW Wang [:jwwang] from comment #33)
> Please also file a following bug to add test cases for this bug. Thanks.

Should I land this patch before writing test cases?
Because I am solving the Bug 1105913 now, I could write the test cases after fixing this bug.

Comment 35

[JW Wang [:jwwang] [:jw_wang]]

Sure, you can have test cases  later.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/333eda1b0099
https://hg.mozilla.org/integration/mozilla-inbound/rev/19313a129f6d

Comment 37

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/333eda1b0099
https://hg.mozilla.org/mozilla-central/rev/19313a129f6d

Comment 38

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8536267 [details] [diff] [review]
[checkin] Part1 : Avoid media ports repeated deletion

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Blocking uplift of bug 879717, see bug 879717 comment 200 for its approval request comment.
[Describe test coverage new/current, TBPL]:Landed on m-c in 37. Covered by all media tests capturing an element with mozCaptureStream().
[Risks and why]: Low. Only adds guards to protect from referencing null pointers. Only ever seen on B2G but could potentially appear in other slow situations.
[String/UUID change made/needed]: None.

This request applies to all patches on this bug.

Comment 39

[Alastor Wu [:alwu]]

Hi, Andreas, 
Bug1105913 is highly related with this bug, I think it should be uplift together if needed.

Comment 40

[Andreas Pehrson [:pehrsons]]

Ah, didn't know that one. So it would be enough to uplift bug 1105913 part 1?

Comment 41

[Alastor Wu [:alwu]]

Hi, Andreas,
I think what you need is the patch 1 in Bug1105913 and Bug1100803. 
They can ensure the correct releasing order of the media port to avoid the repeated deletion.

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8536267 [details] [diff] [review]
[checkin] Part1 : Avoid media ports repeated deletion

If it affects only b2g, not taking it.