Reland of Fix getUserMedia so that failure is reported for invalid audio sources.

Reland of Fix getUserMedia so that failure is reported for invalid audio sources. (patchset #1 id:1 of https://codereview.chromium.org/2626533002/ )

Reason for revert:
Preparing reland

Original issue's description:
> Revert of Fix getUserMedia so that failure is reported for invalid audio sources. (patchset #2 id:20001 of https://codereview.chromium.org/2623443002/ )
> 
> Reason for revert:
> Revert due to WebRTC tests failing:
> 
> https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/56312
> 
> [4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66): error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio: true, video:true});.
> From javascript: (nothing)
> When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true, video:true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90): error: Failed
> 
> and
> 
> [4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66): error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio: true});.
> From javascript: (nothing)
> When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90): error: Failed
> 
> Original issue's description:
> > Fix getUserMedia so that failure is reported for invalid audio sources.
> > This changes getUserMedia to wait for initialization of local
> > audio sources before issuing the completion callback (either success or
> > failure).
> > Previously, if an error occurs between attempting to start a local
> > audio source and the render side OnStreamCreated callback, getUserMedia
> > would report successful completion with an audio track but no audio
> > would actually be delivered for that track.
> > 
> > BUG=679210
> > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > 
> > Review-Url: https://codereview.chromium.org/2623443002
> > Cr-Commit-Position: refs/heads/master@{#442323}
> > Committed: https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbcaabf6d5c3c04
> 
> TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,tommi@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=679210
> 
> Review-Url: https://codereview.chromium.org/2626533002
> Cr-Commit-Position: refs/heads/master@{#442373}
> Committed: https://chromium.googlesource.com/chromium/src/+/f05caaa1349e44abd44a00f88506bf140a0bc887

TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,dewittj@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2622643006
Cr-Commit-Position: refs/heads/master@{#442628}
Committed: https://chromium.googlesource.com/chromium/src/+/5d02cf8e38acafeea410906105d5fc11c3e3395b

[tommi (sloooow) - chröme, 2017-01-10 10:41:10]

Created Reland of Fix getUserMedia so that failure is reported for invalid audio
sources.

[tommi (sloooow) - chröme, 2017-01-10 10:56:45]

On 2017/01/10 10:41:10, tommi (chrömium) wrote:
> Created Reland of Fix getUserMedia so that failure is reported for invalid
audio
> sources.

Please don't start with review - I'm verifying locally that things work the way
they should

[tommi (sloooow) - chröme, 2017-01-10 15:03:51]

Handle case in debug builds where content_browsertests exit without cleaning up
state

[tommi (sloooow) - chröme, 2017-01-10 15:03:52]

Description was changed from

==========
Reland of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #1 id:1 of https://codereview.chromium.org/2626533002/ )

Reason for revert:
Preparing reland

Original issue's description:
> Revert of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #2 id:20001 of https://codereview.chromium.org/2623443002/ )
> 
> Reason for revert:
> Revert due to WebRTC tests failing:
> 
>
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...
> 
> [4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio:
true, video:true});.
> From javascript: (nothing)
> When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true,
video:true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> and
> 
> [4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio:
true});.
> From javascript: (nothing)
> When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> Original issue's description:
> > Fix getUserMedia so that failure is reported for invalid audio sources.
> > This changes getUserMedia to wait for initialization of local
> > audio sources before issuing the completion callback (either success or
> > failure).
> > Previously, if an error occurs between attempting to start a local
> > audio source and the render side OnStreamCreated callback, getUserMedia
> > would report successful completion with an audio track but no audio
> > would actually be delivered for that track.
> > 
> > BUG=679210
> >
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > 
> > Review-Url: https://codereview.chromium.org/2623443002
> > Cr-Commit-Position: refs/heads/master@{#442323}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...
> 
>
TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,tommi@chromiu...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=679210
> 
> Review-Url: https://codereview.chromium.org/2626533002
> Cr-Commit-Position: refs/heads/master@{#442373}
> Committed:
https://chromium.googlesource.com/chromium/src/+/f05caaa1349e44abd44a00f88506...

TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,dewittj@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=679210
==========

to

==========
Reland of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #1 id:1 of https://codereview.chromium.org/2626533002/ )

Reason for revert:
Preparing reland

Original issue's description:
> Revert of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #2 id:20001 of https://codereview.chromium.org/2623443002/ )
> 
> Reason for revert:
> Revert due to WebRTC tests failing:
> 
>
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...
> 
> [4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio:
true, video:true});.
> From javascript: (nothing)
> When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true,
video:true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> and
> 
> [4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio:
true});.
> From javascript: (nothing)
> When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> Original issue's description:
> > Fix getUserMedia so that failure is reported for invalid audio sources.
> > This changes getUserMedia to wait for initialization of local
> > audio sources before issuing the completion callback (either success or
> > failure).
> > Previously, if an error occurs between attempting to start a local
> > audio source and the render side OnStreamCreated callback, getUserMedia
> > would report successful completion with an audio track but no audio
> > would actually be delivered for that track.
> > 
> > BUG=679210
> >
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > 
> > Review-Url: https://codereview.chromium.org/2623443002
> > Cr-Commit-Position: refs/heads/master@{#442323}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...
> 
>
TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,tommi@chromiu...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=679210
> 
> Review-Url: https://codereview.chromium.org/2626533002
> Cr-Commit-Position: refs/heads/master@{#442373}
> Committed:
https://chromium.googlesource.com/chromium/src/+/f05caaa1349e44abd44a00f88506...

TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,dewittj@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[tommi (sloooow) - chröme, 2017-01-10 15:04:16]

Description was changed from

==========
Reland of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #1 id:1 of https://codereview.chromium.org/2626533002/ )

Reason for revert:
Preparing reland

Original issue's description:
> Revert of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #2 id:20001 of https://codereview.chromium.org/2623443002/ )
> 
> Reason for revert:
> Revert due to WebRTC tests failing:
> 
>
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...
> 
> [4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio:
true, video:true});.
> From javascript: (nothing)
> When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true,
video:true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> and
> 
> [4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio:
true});.
> From javascript: (nothing)
> When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> Original issue's description:
> > Fix getUserMedia so that failure is reported for invalid audio sources.
> > This changes getUserMedia to wait for initialization of local
> > audio sources before issuing the completion callback (either success or
> > failure).
> > Previously, if an error occurs between attempting to start a local
> > audio source and the render side OnStreamCreated callback, getUserMedia
> > would report successful completion with an audio track but no audio
> > would actually be delivered for that track.
> > 
> > BUG=679210
> >
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > 
> > Review-Url: https://codereview.chromium.org/2623443002
> > Cr-Commit-Position: refs/heads/master@{#442323}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...
> 
>
TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,tommi@chromiu...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=679210
> 
> Review-Url: https://codereview.chromium.org/2626533002
> Cr-Commit-Position: refs/heads/master@{#442373}
> Committed:
https://chromium.googlesource.com/chromium/src/+/f05caaa1349e44abd44a00f88506...

TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,dewittj@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #1 id:1 of https://codereview.chromium.org/2626533002/ )

Reason for revert:
Preparing reland

Original issue's description:
> Revert of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #2 id:20001 of https://codereview.chromium.org/2623443002/ )
> 
> Reason for revert:
> Revert due to WebRTC tests failing:
> 
>
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...
> 
> [4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio:
true, video:true});.
> From javascript: (nothing)
> When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true,
video:true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> and
> 
> [4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio:
true});.
> From javascript: (nothing)
> When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> Original issue's description:
> > Fix getUserMedia so that failure is reported for invalid audio sources.
> > This changes getUserMedia to wait for initialization of local
> > audio sources before issuing the completion callback (either success or
> > failure).
> > Previously, if an error occurs between attempting to start a local
> > audio source and the render side OnStreamCreated callback, getUserMedia
> > would report successful completion with an audio track but no audio
> > would actually be delivered for that track.
> > 
> > BUG=679210
> >
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > 
> > Review-Url: https://codereview.chromium.org/2623443002
> > Cr-Commit-Position: refs/heads/master@{#442323}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...
> 
>
TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,tommi@chromiu...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=679210
> 
> Review-Url: https://codereview.chromium.org/2626533002
> Cr-Commit-Position: refs/heads/master@{#442373}
> Committed:
https://chromium.googlesource.com/chromium/src/+/f05caaa1349e44abd44a00f88506...

TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,dewittj@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[tommi (sloooow) - chröme, 2017-01-10 15:06:28]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2017-01-10 15:06:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 18:00:07]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1484060788454830,
"parent_rev": "9d32d0bc1e2fbbad5ff6ba372ff386579d006cbc", "commit_rev":
"5d02cf8e38acafeea410906105d5fc11c3e3395b"}

[commit-bot: I haz the power, 2017-01-10 18:00:37]

Description was changed from

==========
Reland of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #1 id:1 of https://codereview.chromium.org/2626533002/ )

Reason for revert:
Preparing reland

Original issue's description:
> Revert of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #2 id:20001 of https://codereview.chromium.org/2623443002/ )
> 
> Reason for revert:
> Revert due to WebRTC tests failing:
> 
>
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...
> 
> [4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio:
true, video:true});.
> From javascript: (nothing)
> When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true,
video:true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> and
> 
> [4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio:
true});.
> From javascript: (nothing)
> When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> Original issue's description:
> > Fix getUserMedia so that failure is reported for invalid audio sources.
> > This changes getUserMedia to wait for initialization of local
> > audio sources before issuing the completion callback (either success or
> > failure).
> > Previously, if an error occurs between attempting to start a local
> > audio source and the render side OnStreamCreated callback, getUserMedia
> > would report successful completion with an audio track but no audio
> > would actually be delivered for that track.
> > 
> > BUG=679210
> >
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > 
> > Review-Url: https://codereview.chromium.org/2623443002
> > Cr-Commit-Position: refs/heads/master@{#442323}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...
> 
>
TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,tommi@chromiu...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=679210
> 
> Review-Url: https://codereview.chromium.org/2626533002
> Cr-Commit-Position: refs/heads/master@{#442373}
> Committed:
https://chromium.googlesource.com/chromium/src/+/f05caaa1349e44abd44a00f88506...

TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,dewittj@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #1 id:1 of https://codereview.chromium.org/2626533002/ )

Reason for revert:
Preparing reland

Original issue's description:
> Revert of Fix getUserMedia so that failure is reported for invalid audio
sources. (patchset #2 id:20001 of https://codereview.chromium.org/2623443002/ )
> 
> Reason for revert:
> Revert due to WebRTC tests failing:
> 
>
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...
> 
> [4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio:
true, video:true});.
> From javascript: (nothing)
> When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true,
video:true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> and
> 
> [4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
>   Actual: false
> Expected: true
> Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio:
true});.
> From javascript: (nothing)
> When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
> c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed
> 
> Original issue's description:
> > Fix getUserMedia so that failure is reported for invalid audio sources.
> > This changes getUserMedia to wait for initialization of local
> > audio sources before issuing the completion callback (either success or
> > failure).
> > Previously, if an error occurs between attempting to start a local
> > audio source and the render side OnStreamCreated callback, getUserMedia
> > would report successful completion with an audio track but no audio
> > would actually be delivered for that track.
> > 
> > BUG=679210
> >
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > 
> > Review-Url: https://codereview.chromium.org/2623443002
> > Cr-Commit-Position: refs/heads/master@{#442323}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...
> 
>
TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,tommi@chromiu...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=679210
> 
> Review-Url: https://codereview.chromium.org/2626533002
> Cr-Commit-Position: refs/heads/master@{#442373}
> Committed:
https://chromium.googlesource.com/chromium/src/+/f05caaa1349e44abd44a00f88506...

TBR=jochen@chromium.org,guidou@chromium.org,xhwang@chromium.org,dewittj@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2622643006
Cr-Commit-Position: refs/heads/master@{#442628}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d02cf8e38acafeea410906105d5...
==========

[commit-bot: I haz the power, 2017-01-10 18:00:38]

Committed patchset #2 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/5d02cf8e38acafeea410906105d5...

[dewittj, 2017-01-10 18:18:52]

not lgtm - this CL should have been reviewed before landing.

[tommi (sloooow) - chröme, 2017-01-10 18:53:51]

On 2017/01/10 18:18:52, dewittj wrote:
> not lgtm - this CL should have been reviewed before landing.

My apologies. I looked at the failure and it's not because of this CL per se but
with the content_browsertests. I figured they need to be fixed but it's simple
enough to work around. Did you take a look at change? It's effectively one 'if'
statement.

[dewittj, 2017-01-10 19:35:03]

I didn't read the new code, but I don't see a reason to skip the code review,
especially since it's subtle enough to pass the CQ but not the waterfall.

https://codereview.chromium.org/2622643006/diff/250001/content/renderer/media...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2622643006/diff/250001/content/renderer/media...
content/renderer/media/user_media_client_impl.cc:502: // The purpose of this
function is just to check the weak pointer.
Bind already checks weak pointers when calling member functions, so I'm not sure
why/whether this actually does anything different.

https://cs.chromium.org/chromium/src/base/bind_helpers.h?rcl=0&l=468

[tommi (sloooow) - chröme, 2017-01-10 19:36:55]

A revert of this CL (patchset #2 id:250001) has been created in
https://codereview.chromium.org/2628603003/ by tommi@chromium.org.

The reason for reverting is: Reverting for later reland (and review).

[tommi (sloooow) - chröme, 2017-01-10 19:40:47]

On 2017/01/10 19:35:03, dewittj wrote:
> I didn't read the new code, but I don't see a reason to skip the code review,
> especially since it's subtle enough to pass the CQ but not the waterfall.
> 
>
https://codereview.chromium.org/2622643006/diff/250001/content/renderer/media...
> File content/renderer/media/user_media_client_impl.cc (right):
> 
>
https://codereview.chromium.org/2622643006/diff/250001/content/renderer/media...
> content/renderer/media/user_media_client_impl.cc:502: // The purpose of this
> function is just to check the weak pointer.
> Bind already checks weak pointers when calling member functions, so I'm not
sure
> why/whether this actually does anything different.
> 
> https://cs.chromium.org/chromium/src/base/bind_helpers.h?rcl=0&l=468

Agreed. I'm reverting and putting my tail between my legs. I ran the same unit
tests on Windows 7, in shards, parallel and sequentially, on repeat.  They never
failed.  So I looked at the call stack and to me it looked like 'this' was null
when the event was fired. It also seemed to be preceded by other errors that in
themselves are unrelated (video initialization failing), but would cause the
cleanup phase of the test to run differently than the normal case.

In any case, I'm reverting as you point out that PostTask should already be
doing this.

[miu, 2017-01-10 21:20:32]

Since this touches a lot of code I spent a long time refactoring, I'd like to
review the next attempt (olka@ is also very familiar with this code).

Looking at this patch, I feel there's a lot of complexity introduced in the
start-up workflow now (extra OnCaptureStarted() thread-hopping, a new "pending
source" list in UMCImpl, etc.). Granted, maybe I don't understand the big
picture, and this is actually necessary complexity. However, consider whether
this simpler scheme would work:

1. How about we just start the tracks optimistically (as we do now)? If the
source later fails to start, MediaStreamAudioTrack::StopSourceOnError() should
be called. This will set the ready state to "ended" for the source and notify
the web platform.

2. What isn't happening now, but perhaps is what really motivated this change,
is to also "end" the tracks when the source is ended. That can be accomplished
by simply adding the following to MediaStreamAudioSource::DoStopSource():

  std::Vector<MediaStreamTrack*> tracks;
  deliverer_->GetConsumerList(tracks);
  for (MediaStreamTrack* track : tracks)
    track->Stop();

3. There doesn't seem to be any error API in the W3C spec right now. I assume
web pages are supposed to use "onEnded" events to react to errors that end a
source/track?

[tommi (sloooow) - chröme, 2017-01-10 22:25:53]

I made a change last week that does exactly that (fires onended as soon as
the source has finished initializing). So as is, we're in a better state
than we were but we're not spec compliant until we fire an error if
actually can't access the devices.

The reason I did that first step is because this is complicated. At the
heart of what makes it complicated is that we have a synchronous
EnsureSourceIsStarted method (on my phone right now, don't recall the exact
name) that returns a bool. Changing that design to support an asynchronous
callback would probably be the right thing but it's a bigger change than
this approach, which is to only change the way local audio sources are
initialized during a getUserMedia call.

I'm open to suggestions and better yet, if you could work with me on this,
that would be great. Olga might be able to help too but I know she's busy
with the mojo work for audio. There are more issues with audio source
initialization that I'm looking at, including issues with how our stats are
reported and I would really like it if we could be in a better shape than
we are in 56 and earlier. I unfortunately have to do a lot of juggling of
various tasks :-)

On Tue, Jan 10, 2017, 22:20 <miu@chromium.org> wrote:

> Since this touches a lot of code I spent a long time refactoring, I'd like
> to
> review the next attempt (olka@ is also very familiar with this code).
>
> Looking at this patch, I feel there's a lot of complexity introduced in the
> start-up workflow now (extra OnCaptureStarted() thread-hopping, a new
> "pending
> source" list in UMCImpl, etc.). Granted, maybe I don't understand the big
> picture, and this is actually necessary complexity. However, consider
> whether
> this simpler scheme would work:
>
> 1. How about we just start the tracks optimistically (as we do now)? If the
> source later fails to start, MediaStreamAudioTrack::StopSourceOnError()
> should
> be called. This will set the ready state to "ended" for the source and
> notify
> the web platform.
>
> 2. What isn't happening now, but perhaps is what really motivated this
> change,
> is to also "end" the tracks when the source is ended. That can be
> accomplished
> by simply adding the following to MediaStreamAudioSource::DoStopSource():
>
> std::Vector<MediaStreamTrack*> tracks;
> deliverer_->GetConsumerList(tracks);
> for (MediaStreamTrack* track : tracks)
> track->Stop();
>
> 3. There doesn't seem to be any error API in the W3C spec right now. I
> assume
> web pages are supposed to use "onEnded" events to react to errors that end
> a
> source/track?
>
>
> https://codereview.chromium.org/2622643006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-01-11 18:43:48]

So, IIUC, the spec requires getUserMedia() run the rejection callback if a
device can't be accessed instead of later shutting down the source
on-error? I'm looking at GUM algorithm steps 6.2.4 and 10
<https://www.w3.org/TR/mediacapture-streams/#dom-mediadevices-getusermedia>.
Is this what you're trying to fix?

Sure, I can help w/ code reviews and general discussion of approaches.


On Tue, Jan 10, 2017 at 2:25 PM, Tommi <tommi@chromium.org> wrote:

> I made a change last week that does exactly that (fires onended as soon as
> the source has finished initializing). So as is, we're in a better state
> than we were but we're not spec compliant until we fire an error if
> actually can't access the devices.
>
> The reason I did that first step is because this is complicated. At the
> heart of what makes it complicated is that we have a synchronous
> EnsureSourceIsStarted method (on my phone right now, don't recall the exact
> name) that returns a bool. Changing that design to support an asynchronous
> callback would probably be the right thing but it's a bigger change than
> this approach, which is to only change the way local audio sources are
> initialized during a getUserMedia call.
>
> I'm open to suggestions and better yet, if you could work with me on this,
> that would be great. Olga might be able to help too but I know she's busy
> with the mojo work for audio. There are more issues with audio source
> initialization that I'm looking at, including issues with how our stats are
> reported and I would really like it if we could be in a better shape than
> we are in 56 and earlier. I unfortunately have to do a lot of juggling of
> various tasks :-)
>
> On Tue, Jan 10, 2017, 22:20 <miu@chromium.org> wrote:
>
>> Since this touches a lot of code I spent a long time refactoring, I'd
>> like to
>> review the next attempt (olka@ is also very familiar with this code).
>>
>> Looking at this patch, I feel there's a lot of complexity introduced in
>> the
>> start-up workflow now (extra OnCaptureStarted() thread-hopping, a new
>> "pending
>> source" list in UMCImpl, etc.). Granted, maybe I don't understand the big
>> picture, and this is actually necessary complexity. However, consider
>> whether
>> this simpler scheme would work:
>>
>> 1. How about we just start the tracks optimistically (as we do now)? If
>> the
>> source later fails to start, MediaStreamAudioTrack::StopSourceOnError()
>> should
>> be called. This will set the ready state to "ended" for the source and
>> notify
>> the web platform.
>>
>> 2. What isn't happening now, but perhaps is what really motivated this
>> change,
>> is to also "end" the tracks when the source is ended. That can be
>> accomplished
>> by simply adding the following to MediaStreamAudioSource::DoStopSource():
>>
>> std::Vector<MediaStreamTrack*> tracks;
>> deliverer_->GetConsumerList(tracks);
>> for (MediaStreamTrack* track : tracks)
>> track->Stop();
>>
>> 3. There doesn't seem to be any error API in the W3C spec right now. I
>> assume
>> web pages are supposed to use "onEnded" events to react to errors that
>> end a
>> source/track?
>>
>>
>> https://codereview.chromium.org/2622643006/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.