Closed
Bug 1335966
Opened 7 years ago
Closed 7 years ago
Crash in nr_ice_component_consent_timer_cb
Categories
(Core :: WebRTC: Networking, defect, P1)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | + | fixed |
firefox54 | --- | fixed |
firefox55 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: drno, Assigned: drno)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
bwc
:
review+
|
Details |
1.69 KB,
patch
|
bwc
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Looks like when we get the new event that the network got re-attached we iterate through all streams to refresh consent right away. But probably some of these streams did not have a consent setup (yet).
Assignee | ||
Updated•7 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 15
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8832702 [details] Bug 1335966: skip refreshing ICE cosent for streams with no consent context. https://reviewboard.mozilla.org/r/108892/#review110288 ::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1354 (Diff revision 1) > + if (!comp->consent_ctx) { > + return; > + } I guess this was happening due to a call from |nr_ice_media_stream_refresh_consent_all|? Maybe this check means we no longer need the NR_ICE_COMPONENT_DISABLED checks in that function?
Attachment #8832702 -
Flags: review?(docfaraday) → review+
Comment 3•7 years ago
|
||
hi, this signature seems to be a regression in 53 which is now on beta. what are the next steps in getting the patch landed?
Hello Maire, Jesup, please see comment 3. Any help in getting this uplifted (if deemed low risk) to Beta53 will be appreciated. Thanks!
Flags: needinfo?(rjesup)
Flags: needinfo?(mreavy)
Comment 5•7 years ago
|
||
Wes, did this land already or is it still in limbo? Thanks!
Updated•7 years ago
|
tracking-firefox53:
--- → +
Comment 6•7 years ago
|
||
Randell -- let's land this. Nils -- Any concerns with uplifting? Can you write the request? Ritu -- thanks for the needinfo.
Flags: needinfo?(mreavy)
Assignee | ||
Comment 7•7 years ago
|
||
I apologize that this fell off my radar. I'll get back to it in a few hours (wanted to address Byron's review comments). And then I'll file the uplift request.
Updated•7 years ago
|
Flags: needinfo?(rjesup)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Byron I think you are right with the NR_ICE_COMPONENT_DISABLED checks. So I took them out for the patch for mozilla-central. But I'm thinking that for landing a patch in Aurora and Beta I would leave the checks in there, because that way we change less code (and avoid creating new problems). Does that sound reasonable to you?
Flags: needinfo?(docfaraday)
Assignee | ||
Comment 10•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Bug 1318180 introduced this problem. [User impact if declined]: Content process crashes in certain WebRTC call scenarios if network connectivity is lost and regained. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: No, we don't know exactly how to reproduce. [Needs manual test from QE? If yes, steps to reproduce]: See above. [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: No [Why is the change risky/not risky?]: It only adds a safety check at the beginning of a function to exit early. [String changes made/needed]: N/A
Flags: needinfo?(drno)
Attachment #8846490 -
Flags: review?(docfaraday)
Attachment #8846490 -
Flags: approval-mozilla-beta?
Attachment #8846490 -
Flags: approval-mozilla-aurora?
Comment 11•7 years ago
|
||
Comment on attachment 8846490 [details] [diff] [review] bug1335966_aurora_beta_fix.patch Yeah, that sounds reasonable.
Flags: needinfo?(docfaraday)
Attachment #8846490 -
Flags: review?(docfaraday) → review+
Comment 12•7 years ago
|
||
Pushed by drno@ohlmeier.org: https://hg.mozilla.org/integration/autoland/rev/335a2fee24a8 skip refreshing ICE cosent for streams with no consent context. r=bwc
Comment 13•7 years ago
|
||
Comment on attachment 8846490 [details] [diff] [review] bug1335966_aurora_beta_fix.patch fix a webrtc crash in 53 and 54 Should be in 53.0b3
Attachment #8846490 -
Flags: approval-mozilla-beta?
Attachment #8846490 -
Flags: approval-mozilla-beta+
Attachment #8846490 -
Flags: approval-mozilla-aurora?
Attachment #8846490 -
Flags: approval-mozilla-aurora+
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/335a2fee24a8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f8621db856f
Comment 16•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4f664bd4ca39
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 17•7 years ago
|
||
Setting qe-verify- based on Nils' assessment on manual testing needs (see Comment 10).
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•