Closed Bug 1436694 Opened 6 years ago Closed 6 years ago

SourceListener in bad state after initial Start() fails

Categories

(Core :: WebRTC: Audio/Video, enhancement, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Version:
60 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(3 files)

Bug 1436694 - Make a PostTask variant that returns a MozPromise.
59 bytes, text/x-review-board-request
jib
: review+
jya
: review+
Details
Bug 1436694 - MozPromisify device initialization and move it to SourceListener.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1436694 - Clarify that MediaEngineSources can be double-stopped.
59 bytes, text/x-review-board-request
padenot
: review+
Details 
The first MediaEngineSource::Start() happens outside of SourceListener [1], so if it fails it still thinks the source has started.

So later when stopping the MediaEngineSource it is in a bad state (`MOZ_ASSERT(mState == kStarted`), but `mState == kStopped` [2]).

Note that this assert is debug only but for sanity we should get this fixed.

[1] https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/media/MediaManager.cpp#1407
[2] https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#298
Rank: 18
status-firefox58: --- → unaffected
status-firefox59: --- → unaffected
status-firefox-esr52: --- → unaffected
Priority: -- → P2
See Also: → 1435670
No longer blocks: 1299515
Blocks: 1299515
Blocks: 1436074
Comment on attachment 8951622 [details]
Bug 1436694 - Make a PostTask variant that returns a MozPromise.

https://reviewboard.mozilla.org/r/220902/#review227112

Lgtm.
Attachment #8951622 - Flags: review?(jib) → review+
Comment on attachment 8951623 [details]
Bug 1436694 - MozPromisify device initialization and move it to SourceListener.

https://reviewboard.mozilla.org/r/220904/#review227122

lgtm.

::: dom/media/MediaManager.cpp:1420
(Diff revision 1)
> +          auto streamError = MakeRefPtr<MediaStreamError>(window->AsInner(), *error);
> +          onFailure->OnError(streamError);

inline?
Attachment #8951623 - Flags: review?(jib) → review+
Comment on attachment 8951623 [details]
Bug 1436694 - MozPromisify device initialization and move it to SourceListener.

https://reviewboard.mozilla.org/r/220904/#review227122

> inline?

Not sure I follow. Inline what to where?
Comment on attachment 8951622 [details]
Bug 1436694 - Make a PostTask variant that returns a MozPromise.

https://reviewboard.mozilla.org/r/220902/#review228034

jya asked me about MozPromise::Private, and I got caught up doing a quick (superficial) fly-by review. :-)
Just a couple of suggestions:

::: dom/media/MediaManager.cpp:2154
(Diff revision 1)
> +MediaManager::PostTask(const char* aName,
> +                       std::function<void(const RefPtr<typename MozPromiseType::Private>&)>&& aFunction)

I'm not a fan of std::function, when you could instead make this a templated function in the header (in which case change `Move` into `Forward` below).

::: dom/media/MediaManager.cpp:2157
(Diff revision 1)
> +  auto p = MakeRefPtr<typename MozPromiseType::Private>(aName);
> +  MediaManager::PostTask(NS_NewRunnableFunction(
> +        aName, [p, func = Move(aFunction)]() { func(p); }));
> +  return p.get();

You could use a MozPromiseHolder instead of MozPromise::Private, see example in https://searchfox.org/mozilla-central/source/dom/media/MediaRecorder.cpp#1751-1772 .
Comment on attachment 8953060 [details]
Bug 1436694 - Clarify that MediaEngineSources can be double-stopped.

https://reviewboard.mozilla.org/r/222304/#review228602
Attachment #8953060 - Flags: review?(padenot) → review+
Comment on attachment 8951622 [details]
Bug 1436694 - Make a PostTask variant that returns a MozPromise.

https://reviewboard.mozilla.org/r/220902/#review228824

::: dom/media/MediaManager.cpp:2152
(Diff revision 2)
>    NS_ASSERTION(Get(), "MediaManager singleton?");
>    NS_ASSERTION(Get()->mMediaThread, "No thread yet");
>    Get()->mMediaThread->message_loop()->PostTask(Move(task));
>  }
>  
> +template<typename MozPromiseType, typename FuncionType>

'Funcion' -> 'Function'
Comment on attachment 8951622 [details]
Bug 1436694 - Make a PostTask variant that returns a MozPromise.

https://reviewboard.mozilla.org/r/220902/#review229060
Attachment #8951622 - Flags: review?(jyavenard) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae428bfb913f
Make a PostTask variant that returns a MozPromise. r=jib, r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c38cc382d21
MozPromisify device initialization and move it to SourceListener. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5cc71d38e4a
Clarify that MediaEngineSources can be double-stopped. r=padenot
Backed out for frequent assertion failures at MediaEngineWebRTCAudio.cpp:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0b46a7a0a05b1e3f1d46147a46bbe37a7f55f5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1aff350b83b8d504dbaa3d5d8d4fe6cd99512786&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164350480&repo=mozilla-inbound&lineNumber=10939

[task 2018-02-26T15:24:16.209Z] 15:24:16     INFO - GECKO(1045) | [1101:WebRTCPD #2]: E/signaling [WebRTCPD #2|WebrtcAudioSessionConduit] AudioConduit.cpp:797: A/V sync: sync delta: 0ms, audio jitter delay 53ms, playout delay 0ms
[task 2018-02-26T15:24:16.392Z] 15:24:16     INFO - GECKO(1045) | [1101:Main Thread]: W/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:1184: GetTransmitPipelinesMatching: none found for (nil)
[task 2018-02-26T15:24:16.611Z] 15:24:16     INFO - GECKO(1045) | (stun/INFO) STUN-CLIENT(pgCr|IP4:172.17.0.4:36422/UDP|IP4:172.17.0.4:49302/UDP(host(IP4:172.17.0.4:36422/UDP)|candidate:0 1 UDP 2122252543 172.17.0.4 49302 typ host)): Timed out
[task 2018-02-26T15:24:16.613Z] 15:24:16     INFO - GECKO(1045) | (ice/INFO) ICE-PEER(PC:1519658652948303 (id=2147483750 url=http://mochi.test:8888/tests/dom/media/tests/mochitest/test_peerConnection_verifyAudioAfter:default)/CAND-PAIR(pgCr): setting pair to state FAILED: pgCr|IP4:172.17.0.4:36422/UDP|IP4:172.17.0.4:49302/UDP(host(IP4:172.17.0.4:36422/UDP)|candidate:0 1 UDP 2122252543 172.17.0.4 49302 typ host)
[task 2018-02-26T15:24:16.710Z] 15:24:16     INFO - GECKO(1045) | [1101:WebRTCPD #2]: E/signaling [WebRTCPD #2|WebrtcAudioSessionConduit] AudioConduit.cpp:797: A/V sync: sync delta: 0ms, audio jitter delay 48ms, playout delay 0ms
[task 2018-02-26T15:24:17.398Z] 15:24:17     INFO - GECKO(1045) | [1101:Main Thread]: W/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:1184: GetTransmitPipelinesMatching: none found for (nil)
[task 2018-02-26T15:24:17.757Z] 15:24:17     INFO - GECKO(1045) | --DOMWINDOW == 11 (0x7f81840c7000) [pid = 1045] [serial = 52] [outer = (nil)] [url = chrome://browser/content/webrtcIndicator.xul]
[task 2018-02-26T15:24:17.758Z] 15:24:17     INFO - GECKO(1045) | --DOMWINDOW == 10 (0x7f8182f38000) [pid = 1045] [serial = 50] [outer = (nil)] [url = chrome://browser/content/webrtcIndicator.xul]
[task 2018-02-26T15:24:17.969Z] 15:24:17     INFO - GECKO(1045) | Assertion failure: (aGraph->IterationEnd() == 0 && mLastAppendTime == 0) || aGraph->IterationEnd() > mLastAppendTime (Iteration time didn't advance since last append), at /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:115
[task 2018-02-26T15:24:52.127Z] 15:24:52     INFO - GECKO(1045) | #01: mozilla::MediaEngineWebRTCMicrophoneSource::Pull [dom/media/webrtc/MediaEngineWebRTCAudio.cpp:767]
[task 2018-02-26T15:24:52.128Z] 15:24:52     INFO - 
[task 2018-02-26T15:24:52.129Z] 15:24:52     INFO - GECKO(1045) | #02: mozilla::MediaDevice::Pull [dom/media/MediaManager.cpp:1031]
[task 2018-02-26T15:24:52.130Z] 15:24:52     INFO - 
[task 2018-02-26T15:24:52.132Z] 15:24:52     INFO - GECKO(1045) | #03: mozilla::SourceListener::NotifyPull [dom/media/MediaManager.cpp:4251]
Flags: needinfo?(apehrson)
Depends on: 1440040
Flags: needinfo?(apehrson)
Depends on: 1440169
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59f19fc034cf
Make a PostTask variant that returns a MozPromise. r=jib, r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/0451fe123f5b
MozPromisify device initialization and move it to SourceListener. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/04b165b560de
Clarify that MediaEngineSources can be double-stopped. r=padenot
Depends on: 1443774
No longer depends on: 1443774
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: