New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 751785 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-08-17
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression

Blocking:
issue 755166



Sign in to add a comment

RTCRtpReceiver.track can be null

Project Member Reported by philipp....@googlemail.com, Aug 2 2017

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/59.0.3071.109 Chrome/59.0.3071.109 Safari/537.36

Steps to reproduce the problem:
see https://github.com/webrtc/adapter/issues/640

What is the expected behavior?
receiver.track is not null.

What went wrong?
just about everything

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 61.0.3163.13  Channel: n/a
OS Version: 
Flash Version: Shockwave Flash 16.0 r0
 

Comment 1 by phistuck@gmail.com, Aug 2 2017

https://w3c.github.io/webrtc-pc/#rtcrtpreceiver-interface indeed seems not nullable.
Cc: pbomm...@chromium.org guidou@chromium.org
Components: -Blink>WebRTC Blink>WebRTC>Network
Labels: -Type-Bug M-61 ReleaseBlock-Stable OS-Android OS-Chrome OS-Mac OS-Windows Type-Bug-Regression
Owner: hbos@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue and this is regression in M61, Please find bisect range below :

You are probably looking for a change made after 480501 (known good), but no lat
er than 480502 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/fcbeaac2e4058876f6e76a8dc0215b460f1c36b8..1d05904bcc684ee1267bcef56496a9c3ebce2b61
@hbos: Friendly ping! Could you please provide any update on this issue as it has been marked as a stable blocker.

Thank You!

Comment 4 by hbos@chromium.org, Aug 7 2017

@guidou can you take a look while I'm on vacation? Back on monday. -From a tent in the middle of nowhere.
wrt to impact, I think there are very few people who use receivers yet, in particular together with renegotiation. adapter did which caused an exception in the ontrack shim but that has now been fixed and a new version has been published.
Cc: hbos@chromium.org
Owner: guidou@chromium.org
Labels: -Pri-2 Pri-1
[Bulk Edit]
URGENT - PTAL.
Your bug is labelled as M61 Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP.

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label.

Thank you.

Comment 9 by guidou@chromium.org, Aug 10 2017

Owner: hbos@chromium.org
govind@: I prepared a CL (https://chromium-review.googlesource.com/c/610233) that fixes part of the bug, but not all of it. I think once this patch lands (and is merged to 61), we can remove the ReleaseBlock label.

hbos@ will prepare a complete fix for 62.

Comment 10 by hbos@chromium.org, Aug 14 2017

With guidou's fix getReceivers() there is no initialization race and tracks will be created for the receivers.

There is still a bug if a track is created by adding it a stream that already exists, in which case the upper layer will not be aware of the track. With guidou's fix getReceivers() will ignore such receivers instead of exposing a track-less receiver (or crashing in debug build). This will be tracked by  https://crbug.com/755166  instead.
Shall we bump this to M62?
Blockedon: 755166
Labels: Merge-Request-61
Project Member

Comment 15 by sheriffbot@chromium.org, Aug 15 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 16 by hbos@chromium.org, Aug 15 2017

Blockedon: -755166
Blocking: 755166
Let's swap the blocked on/ing around. With guidou's fix receivers with null tracks won't be exposed, fixing this bug.

@anatolid: No this is to be merged into M61. The remaining bug can be M62.
Before we approve merge to M61, please answer followings:
* Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61?
* Any other important details to justify the merge.

Please note M61 is already in Beta, so merge bar is VERY high. Thank you.

govind@: I have verified the fix in Canary. The fix has enough coverage and it should be a clean merge to M61.
If approved, I would merge after a couple more days in Canary just as a precaution, unless you recommend otherwise.

Since this fixes a race that might potentially be exploited, I think we should really merge this to M61.

NextAction: 2017-08-17
Approving merge to M61 branch 3163 based on comment #18. Please merge after few more days of Canary coverage. Thank you.
Labels: -Merge-Review-61 Merge-Approved-61

Comment 21 Deleted

Please merge you change to M61 branch 3163 by 4:00 PM PT tomorrow, Thursday (08/17) so we can take it in for next week Beta release. Thank you.
Project Member

Comment 23 by bugdroid1@chromium.org, Aug 17 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7acabd7d26a2d9229393f3596bde08a3eec7334e

commit 7acabd7d26a2d9229393f3596bde08a3eec7334e
Author: Guido Urdaneta <guidou@chromium.org>
Date: Thu Aug 17 08:58:58 2017

Fix race during WebRtcMediaStreamTrackAdapter initialization.

This does not completely fix  bug 751785 , but it is a prerequisite.

TBR=guidou@chromium.org

(cherry picked from commit bed043e4d38f7e38c98bec358a5ef870358ba195)

Bug:  751785 
Change-Id: I3ece316c04f80cbafdb5959d9a19851086a1baf1
Reviewed-on: https://chromium-review.googlesource.com/610233
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#494115}
Reviewed-on: https://chromium-review.googlesource.com/618332
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#627}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/7acabd7d26a2d9229393f3596bde08a3eec7334e/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc
[modify] https://crrev.com/7acabd7d26a2d9229393f3596bde08a3eec7334e/chrome/test/data/webrtc/peerconnection_rtp.js
[modify] https://crrev.com/7acabd7d26a2d9229393f3596bde08a3eec7334e/content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc
[modify] https://crrev.com/7acabd7d26a2d9229393f3596bde08a3eec7334e/content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h
[modify] https://crrev.com/7acabd7d26a2d9229393f3596bde08a3eec7334e/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp

Comment 24 by hbos@chromium.org, Aug 17 2017

Status: Fixed (was: Assigned)
The NextAction date has arrived: 2017-08-17
Project Member

Comment 26 by bugdroid1@chromium.org, Aug 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bf60fd73e9e4f069d49b44a166988d45cf33aae

commit 4bf60fd73e9e4f069d49b44a166988d45cf33aae
Author: Henrik Boström <hbos@chromium.org>
Date: Thu Aug 17 18:08:32 2017

Added DCHECK and TODO in WebRtcMediaStreamTrackAdapter.

Documentation/TODO related to changes
https://chromium-review.googlesource.com/c/610233 which added the
EnsureTrackIsInitialized code.

Bug:  751785 
Change-Id: I69611e461f371c9f926b638c1dc8169716c8771a
Reviewed-on: https://chromium-review.googlesource.com/618715
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495223}
[modify] https://crrev.com/4bf60fd73e9e4f069d49b44a166988d45cf33aae/content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc
[modify] https://crrev.com/4bf60fd73e9e4f069d49b44a166988d45cf33aae/content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h

Sign in to add a comment