1163852 - Intermittent test_peerConnection_addSecondVideoStream.html | application terminated with exit code -5

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

14:20:55 INFO - ++DOCSHELL 0x96f15800 == 8 [pid = 3573] [id = 49]
14:20:55 INFO - ++DOMWINDOW == 23 (0x971f2c00) [pid = 3573] [serial = 102] [outer = (nil)]
14:20:55 INFO - ++DOMWINDOW == 24 (0x9a0eb400) [pid = 3573] [serial = 103] [outer = 0x971f2c00]
14:20:55 INFO - [Child 3624] WARNING: ENSURE_MAIN_PROCESS failed. Cannot SetCString from content process: media.webrtc.debug.log_file: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/modules/libpref/Preferences.cpp, line 1516
14:20:55 INFO - [Child 3624] WARNING: ENSURE_MAIN_PROCESS failed. Cannot SetCString from content process: media.webrtc.debug.aec_log_dir: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/modules/libpref/Preferences.cpp, line 1516
14:20:56 INFO - [Parent 3573] WARNING: No docshells for remote frames!: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/dom/base/nsFrameLoader.cpp, line 492
14:20:56 INFO - [Parent 3573] WARNING: No docshells for remote frames!: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/dom/base/nsFrameLoader.cpp, line 492
14:20:56 INFO - JavaScript error: chrome://browser/content/webrtcIndicator.js, line 51: TypeError: gStringBundle is undefined
14:20:56 INFO - [Parent 3573] WARNING: Enabling vsync compositor: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/gfx/layers/ipc/CompositorParent.cpp, line 677
14:20:56 INFO - [Parent 3573] ###!!! ASSERTION: /build/buildd/glib2.0-2.32.1/./glib/gmem.c:358: overflow allocating 1515870810*16 bytes: 'glib assertion', file /builds/slave/m-in-lx-d-00000000000000000000/build/src/toolkit/xre/nsSigHandlers.cpp, line 138
14:21:19 INFO - #01: my_glib_log_func [toolkit/xre/nsSigHandlers.cpp:140]
14:21:19 INFO - (firefox:3573): GLib-ERROR **: /build/buildd/glib2.0-2.32.1/./glib/gmem.c:358: overflow allocating 1515870810*16 bytes
14:21:19 INFO - -1323444032[b0f310c0]: [main|sdp_config] sdp_config.c:86: SDP: Initialized config pointer: 0x9e8a4d60
14:21:19 INFO - -1323444032[b0f310c0]: [1431379255029545 (id=2147483742 url=http://mochi.test:8888/tests/dom/media/tests/mochitest/test_peerConnection_addSecondVideoSt]: stable -> have-local-offer
14:21:19 INFO - [Child 3624] WARNING: pipe error (3): Connection reset by peer: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459
14:21:19 INFO - [Child 3624] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1662
14:21:19 INFO - #01: mozilla::ipc::MessageChannel::OnChannelErrorFromLink() [ipc/glue/MessageChannel.cpp:1662]
14:21:19 INFO - #02: mozilla::ipc::ProcessLink::OnChannelError() [ipc/glue/MessageLink.cpp:405]
14:21:19 INFO - #03: IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int) [ipc/chromium/src/chrome/common/ipc_channel_posix.cc:879]
14:21:19 INFO - #04: base::MessagePumpLibevent::OnLibeventNotification(int, short, void*) [ipc/chromium/src/base/message_pump_libevent.cc:231]
14:21:19 INFO - #05: event_base_loop [ipc/chromium/src/third_party/libevent/event.c:1350]
14:21:19 INFO - #06: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) [ipc/chromium/src/base/message_pump_libevent.cc:357]
14:21:19 INFO - #07: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:233]
14:21:19 INFO - #08: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:517]
14:21:19 INFO - #09: base::Thread::ThreadMain() [ipc/chromium/src/base/thread.cc:173]
14:21:19 INFO - #10: ThreadFunc [ipc/chromium/src/base/platform_thread_posix.cc:41]
14:21:19 INFO - #11: libpthread.so.0 + 0x6d4c
14:21:19 INFO - [Child 3624] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1662
14:21:19 INFO - Hit MOZ_CRASH() at /builds/slave/m-in-lx-d-00000000000000000000/build/src/memory/mozalloc/mozalloc_abort.cpp:33
14:21:20 INFO - TEST-INFO | Main app process: killed by out-of-range signal, number 123
14:21:20 INFO - 82 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | A valid string reason is expected
14:21:20 INFO - 83 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | Reason cannot be empty
14:21:20 INFO - 84 INFO Network setup is not required
14:21:20 INFO - 85 INFO Creating PeerConnectionWrapper (pcLocal)
14:21:20 INFO - 86 INFO Creating PeerConnectionWrapper (pcRemote)
14:21:20 INFO - 87 INFO Run step 1: PC_SETUP_SIGNALING_CLIENT
14:21:20 INFO - 88 INFO Run step 2: PC_LOCAL_SETUP_ICE_LOGGER
14:21:20 INFO - 89 INFO Run step 3: PC_REMOTE_SETUP_ICE_LOGGER
14:21:20 INFO - 90 INFO Run step 4: PC_LOCAL_SETUP_SIGNALING_LOGGER
14:21:20 INFO - 91 INFO Run step 5: PC_REMOTE_SETUP_SIGNALING_LOGGER
14:21:20 INFO - 92 INFO Run step 6: PC_LOCAL_SETUP_ADDSTREAM_HANDLER
14:21:20 INFO - 93 INFO Run step 7: PC_REMOTE_SETUP_ADDSTREAM_HANDLER
14:21:20 INFO - 94 INFO Run step 8: PC_LOCAL_CHECK_INITIAL_SIGNALINGSTATE
14:21:20 INFO - 95 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | Initial local signalingState is 'stable'
14:21:20 INFO - 96 INFO Run step 9: PC_REMOTE_CHECK_INITIAL_SIGNALINGSTATE
14:21:20 INFO - 97 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | Initial remote signalingState is 'stable'
14:21:20 INFO - 98 INFO Run step 10: PC_LOCAL_CHECK_INITIAL_ICE_STATE
14:21:20 INFO - 99 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | Initial local ICE connection state is 'new'
14:21:20 INFO - 100 INFO Run step 11: PC_REMOTE_CHECK_INITIAL_ICE_STATE
14:21:20 INFO - 101 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | Initial remote ICE connection state is 'new'
14:21:20 INFO - 102 INFO Run step 12: PC_LOCAL_GUM
14:21:20 INFO - 103 INFO Get 1 local streams
14:21:20 INFO - 104 INFO Call getUserMedia for {"video":true}
14:21:20 INFO - 105 INFO PeerConnectionWrapper (pcLocal) gUM local stream {478b1c81-5d72-41df-96c0-5e9f92b141e7} with video track {46d110b5-d173-48da-a968-af8bdad25f3a}
14:21:20 INFO - 106 INFO Got media stream: video (local)
14:21:20 INFO - 107 INFO PeerConnectionWrapper (pcLocal): "onnegotiationneeded" event fired
14:21:20 INFO - 108 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | addStream adds sender
14:21:20 INFO - 109 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | track has id
14:21:20 INFO - 110 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | track has kind
14:21:20 INFO - 111 INFO Run step 13: PC_REMOTE_GUM
14:21:20 INFO - 112 INFO Get 1 local streams
14:21:20 INFO - 113 INFO Call getUserMedia for {"video":true}
14:21:20 INFO - 114 INFO canplaythrough fired for media element pcLocal_local1_video
14:21:20 INFO - 115 INFO PeerConnectionWrapper (pcRemote) gUM local stream {b8722e28-22b1-4e76-a597-f173e582cffb} with video track {4a224b3d-43b4-4f0b-be1c-2d390b13e39e}
14:21:20 INFO - 116 INFO Got media stream: video (local)
14:21:20 INFO - 117 INFO PeerConnectionWrapper (pcRemote): "onnegotiationneeded" event fired
14:21:20 INFO - 118 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | addStream adds sender
14:21:20 INFO - 119 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | track has id
14:21:20 INFO - 120 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | track has kind
14:21:20 INFO - 121 INFO Run step 14: PC_LOCAL_SETUP_ICE_HANDLER
14:21:20 INFO - 122 INFO Run step 15: PC_REMOTE_SETUP_ICE_HANDLER
14:21:20 INFO - 123 INFO Run step 16: PC_LOCAL_STEEPLECHASE_SIGNAL_EXPECTED_LOCAL_TRACKS
14:21:20 INFO - 124 INFO Run step 17: PC_REMOTE_STEEPLECHASE_SIGNAL_EXPECTED_LOCAL_TRACKS
14:21:20 INFO - 125 INFO Run step 18: PC_LOCAL_GET_EXPECTED_REMOTE_TRACKS 
14:21:20 INFO - 126 INFO Run step 19: PC_REMOTE_GET_EXPECTED_REMOTE_TRACKS
14:21:20 INFO - 127 INFO Run step 20: PC_LOCAL_CREATE_OFFER
14:21:20 INFO - 128 INFO Got offer: {"type":"offer","sdp":"v=0\r\no=mozilla...THIS_IS_SDPARTA-40.0a1 5543845458266036880 0 IN IP4 0.0.0.0\r\ns=-\r\nt=0 0\r\na=fingerprint:sha-256 BA:F1:AD:7C:5C:8B:0C:D8:5E:C9:B4:CF:1F:6A:DA:6A:8B:F4:31:91:D3:58:EB:7C:C1:B3:FE:84:E4:D4:EB:75\r\na=group:BUNDLE sdparta_0\r\na=ice-options:trickle\r\na=msid-semantic:WMS *\r\nm=video 9 RTP/SAVPF 120 126 97\r\nc=IN IP4 0.0.0.0\r\na=sendrecv\r\na=fmtp:120 max-fs=12288;max-fr=60\r\na=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1\r\na=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1\r\na=ice-pwd:d7d27fcd8d71d483c2b5d8946083e374\r\na=ice-ufrag:dbca644f\r\na=mid:sdparta_0\r\na=msid:{478b1c81-5d72-41df-96c0-5e9f92b141e7} {46d110b5-d173-48da-a968-af8bdad25f3a}\r\na=rtcp-fb:120 nack\r\na=rtcp-fb:120 nack pli\r\na=rtcp-fb:120 ccm fir\r\na=rtcp-fb:120 ccm tmmbr\r\na=rtcp-fb:126 nack\r\na=rtcp-fb:126 nack pli\r\na=rtcp-fb:126 ccm fir\r\na=rtcp-fb:126 ccm tmmbr\r\na=rtcp-fb:97 nack\r\na=rtcp-fb:97 nack pli\r\na=rtcp-fb:97 ccm fir\r\na=rtcp-fb:97 ccm tmmbr\r\na=rtcp-mux\r\na=rtpmap:120 VP8/90000\r\na=rtpmap:126 H264/90000\r\na=rtpmap:97 H264/90000\r\na=setup:actpass\r\na=ssrc:3358678933 cname:{36afb69d-826e-4856-a59e-3a7fd6158083}\r\n"}
14:21:20 INFO - 129 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | Local create offer does not change signaling state
14:21:20 INFO - 130 INFO Run step 21: PC_LOCAL_STEEPLECHASE_SIGNAL_OFFER
14:21:20 INFO - 131 INFO Run step 22: PC_LOCAL_SET_LOCAL_DESCRIPTION
14:21:20 INFO - 132 INFO PeerConnectionWrapper (pcLocal): "onsignalingstatechange" event fired
14:21:20 INFO - 133 INFO PeerConnectionWrapper (pcLocal): 'signalingstatechange' event received
14:21:20 INFO - 134 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | PeerConnectionWrapper (pcLocal): legal signaling state transition from stable to have-local-offer
14:21:20 INFO - 135 INFO PeerConnectionWrapper (pcLocal): Successfully set the local description
14:21:20 INFO - 136 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | signalingState after local setLocalDescription is 'have-local-offer'
14:21:20 INFO - 137 INFO Run step 23: PC_REMOTE_GET_OFFER
14:21:20 INFO - 138 INFO Run step 24: PC_REMOTE_SET_REMOTE_DESCRIPTION
14:21:20 WARNING - TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | application terminated with exit code -5
14:21:20 INFO - runtests.py | Application ran for: 0:04:26.759346
14:21:20 INFO - zombiecheck | Reading PID log: /tmp/tmpsHEvnLpidlog
14:21:20 INFO - ==> process 3573 launched child process 3624
14:21:20 INFO - zombiecheck | Checking for orphan process with PID: 3624
14:21:20 INFO - Stopping web server
14:21:20 INFO - Stopping web socket server
14:21:20 INFO - Stopping ssltunnel
14:21:20 INFO - TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes
14:21:20 INFO - TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes
14:21:20 INFO - TEST-INFO | leakcheck | tab process: leak threshold set at 25000 bytes
14:21:20 INFO - TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes
14:21:20 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | tab process: missing output line for total leaks!
14:21:20 INFO - TEST-INFO | leakcheck | missing output line from log file /tmp/tmpM6NVJK.mozrunner/runtests_leaks_tab_pid3624.log
14:21:20 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks!
14:21:20 INFO - TEST-INFO | leakcheck | missing output line from log file /tmp/tmpM6NVJK.mozrunner/runtests_leaks.log
14:21:20 INFO - runtests.py | Running tests: end.
14:21:20 INFO - SUITE-END | took 280s
14:21:20 ERROR - Return code: 251
14:21:20 ERROR - No tests run or test summary not found
14:21:20 INFO - TinderboxPrint: mochitest-plain-chunked<br/><em class="testfail">T-FAIL</em>
14:21:20 WARNING - # TBPL FAILURE #
14:21:20 WARNING - The mochitest suite: plain-chunked ran with return status: FAILURE
14:21:20 INFO - Running post-action listener: _package_coverage_data
14:21:20 INFO - Running post-action listener: _resource_record_post_action
14:21:20 INFO - Running post-run listener: _resource_record_post_run
14:21:21 INFO - Total resource usage - Wall time: 305s; CPU: 100.0%; Read bytes: 2818048; Write bytes: 168591360; Read time: 100; Write time: 253432
14:21:21 INFO - install - Wall time: 24s; CPU: 100.0%; Read bytes: 0; Write bytes: 98234368; Read time: 0; Write time: 205244
14:21:21 INFO - run-tests - Wall time: 282s; CPU: 100.0%; Read bytes: 2785280; Write bytes: 62238720; Read time: 100; Write time: 24496
14:21:21 INFO - Running post-run listener: _upload_blobber_files
14:21:21 INFO - Blob upload gear active.

Comment 1

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=9754771
repository: mozilla-inbound
start_time: 2015-05-11T14:13:59
who: wkocher[at]mozilla[dot]com
machine: tst-linux32-spot-717
buildname: Ubuntu VM 12.04 mozilla-inbound debug test mochitest-e10s-3
revision: c6ea21c091e9

[Child 3624] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1662
[Child 3624] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1662
TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | application terminated with exit code -5
TEST-UNEXPECTED-FAIL | leakcheck | tab process: missing output line for total leaks!
TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks!
Return code: 251
No tests run or test summary not found

Comment 2

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=10176905
repository: mozilla-inbound
start_time: 2015-05-26T17:19:08
who: philringnalda[at]gmail[dot]com
machine: tst-linux64-spot-1560
buildname: Ubuntu ASAN VM 12.04 x64 mozilla-inbound opt test mochitest-3
revision: eba8b4d8703a

SUMMARY: AddressSanitizer: heap-use-after-free /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:924 mozilla::WebrtcVideoConduit::SelectSendResolution(unsigned short, unsigned short)
TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addSecondVideoStream.html | application terminated with exit code 1
Return code: 1
No tests run or test summary not found

Comment 3

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

ASAN hit shows this is a UAF; I know what the issue is.  With renegotiation, we need locking around the codec settings in VideoConduit since it gets changed on MainThread, and gets used on the thread that calls SendVideoFrame (which may be a capture thread or and MSG callback thread).

Comment 4

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

Attachment: add locks around mCurSendCodecConfig

Comment 5

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

Not: not sure the original hit is the same as the second ASAN hit; hard to tell.  I suspect this his quite hard to exploit given the pattern of reads of this memory (shouldn't be any writes).

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8611383 [details] [diff] [review]
add locks around mCurSendCodecConfig

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

There's a little cleanup we could do here.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +362,5 @@
>    {
> +    MutexAutoLock lock(mCodecMutex);
> +
> +    //validate codec param
> +    if((condError = ValidateCodecConfig(codecConfig, true)) != kMediaConduitNoError)

I would expect the locking to go inside ValidateCodecConfig.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +603,5 @@
> +      return condError;
> +    }
> +
> +    //Check if we have same codec already applied
> +    if(CheckCodecsForMatch(mCurSendCodecConfig, codecConfig))

ValidateCodecConfig does this already. We should be able to remove this, and move the lock down into ValidateCodecConfig.

@@ +1150,5 @@
> +    if (!SelectSendResolution(width, height))
> +    {
> +      return kMediaConduitCaptureError;
> +    }
> +    if (!SelectSendFrameRate(mSendingFramerate))

It might be nice to unify SelectSendResolution and SelectSendFramerate and do the locking inside, if it is easy. (It looks like we already have bug 1132318 for that) Not a big deal if it fights back I suppose.

Comment 8

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

Bug 1171255 is likely a duplicate too, but it wasn't an ASAN run and it just timed out.

Comment 9

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

Attachment: interdiffs (including merging SelectSendFrameRate with SelectSendResolution)

Comment 10

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

Attachment: merge SelectSendFrameRate with SelectSendResolution

Merges SelectSendFrameRate() into SelectSendResolution() as well as clean up the locking a bit per review.  I'll land this under bug 1132318 probably, since it also covers that, unless we need to make a more pointed patch for uplift

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8615574 [details] [diff] [review]
interdiffs (including merging SelectSendFrameRate with SelectSendResolution)

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

Just some nits.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1004,5 @@
>  WebrtcVideoConduit::SelectSendResolution(unsigned short width,
>                                           unsigned short height,
>                                           bool force)
>  {
> +  mCodecMutex.AssertCurrentThreadOwns();

Ok, this is a good alternative to putting the lock here. I can live with this.

@@ +1122,1 @@
>  WebrtcVideoConduit::SelectSendFrameRate(unsigned int framerate)

Can this be const? Or is the Assert non-const? (I wouldn't be sad if you made mCodecMutex mutable, if you're so inclined)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +148,5 @@
>                              unsigned short height,
>                              bool force);
>  
>    /**
>     * Function to select and change the encoding frame rate based on incoming frame rate

We're no longer changing this here, right?

Comment 12

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

> @@ +1122,1 @@
> >  WebrtcVideoConduit::SelectSendFrameRate(unsigned int framerate)
> 
> Can this be const? Or is the Assert non-const? (I wouldn't be sad if you
> made mCodecMutex mutable, if you're so inclined)

const works

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +148,5 @@
> >                              unsigned short height,
> >                              bool force);
> >  
> >    /**
> >     * Function to select and change the encoding frame rate based on incoming frame rate
> 
> We're no longer changing this here, right?

Right.  And one thing we missed - the merger of framerate and resolution didn't actually change the rate.

Comment 13

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

Attachment: interdiffs (final)

Comment 14

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

Attachment: merge SelectSendFrameRate with SelectSendResolution

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8615612 [details] [diff] [review]
merge SelectSendFrameRate with SelectSendResolution

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

Patch description doesn't mention the locking anymore, just a heads up.

Comment 16

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

Comment on attachment 8615612 [details] [diff] [review]
merge SelectSendFrameRate with SelectSendResolution

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily - ASAN read not easily exported anywhere.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not really.  Clearly introduces a Mutex, but also does some cleanup of related code.

Which older supported branches are affected by this flaw? 38 and later

If not all supported branches, which bug introduced the flaw? Renegotiation of WebRTC calls + multistream (they landed in 38)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Trivial to backport.  If risk of the cleanup is a factor, uplifts could be a simplified patch that only introduces the mutex (closer to the original patch). If so, it's not worth trying to 'hide' the checkin under the rewrite bug (and since it's being uplifted to beta 39, hiding it may not be very effective anyways).

How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions; some simple manual testing of resolution changes should be all that's needed.

No string changes.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8615612 [details] [diff] [review]
merge SelectSendFrameRate with SelectSendResolution

Approvals given.

Comment 19

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

Landed under Bug 1132318 (which it also covers)
https://hg.mozilla.org/integration/mozilla-inbound/rev/08acee81ae64

Comment 20

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

Attachment: interdiff to fix m3 issues

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b1ea78a7bce

Comment 21

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

Relanded; full-green try

Comment 22

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

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

I took a quick look at applying this to Aurora and hit VideoConduit.{cpp|h} conflicts.

Comment 24

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

Attachment: Aurora/40 patch

Comment 25

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

Attachment: beta/39 patch (virtually identical to Aurora patch)

Comment 26

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

Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e460341920a3

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Thanks for rebasing, it'll be on the radar for the next pass of approvals.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e74f27a5ab0

Comment 30

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

Florin: this landed with bug 1132318.  Other than verifying we got no more hits in automation, there's no manual testing known for this bug.

Comment 31

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Randell Jesup [:jesup] from comment #30)
> Florin: this landed with bug 1132318.  Other than verifying we got no more
> hits in automation, there's no manual testing known for this bug.

Thanks Randell! I'm marking it as qe-verify-.