Crash in nr_ice_component_consent_timer_cb

RESOLVED FIXED in Firefox 53

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
15
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: drno, Assigned: drno)

Tracking

({crash, regression})

Trunk
mozilla55
crash, regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53+ fixed, firefox54 fixed, firefox55 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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

a year ago
backlog: --- → webrtc/webaudio+
Rank: 15
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
Comment hidden (mozreview-request)

Comment 2

a year 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

11 months 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?
status-firefox55: --- → affected
Flags: needinfo?(drno)
Keywords: crash, regression

Comment 4

11 months ago
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)
Wes, did this land already or is it still in limbo? Thanks!
tracking-firefox53: --- → +
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

11 months 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

11 months ago
Flags: needinfo?(rjesup)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

10 months 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

10 months ago
Created attachment 8846490 [details] [diff] [review]
bug1335966_aurora_beta_fix.patch

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

10 months 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

10 months 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 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/335a2fee24a8
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 15

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f8621db856f
status-firefox54: affected → fixed

Comment 16

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4f664bd4ca39
status-firefox53: affected → fixed
status-firefox-esr52: --- → unaffected
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.