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)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 + fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

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).
backlog: --- → webrtc/webaudio+
Rank: 15
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+
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?
Flags: needinfo?(drno)
Keywords: crash, regression
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!
Randell -- let's land this. 
Nils -- Any concerns with uplifting?  Can you write the request?
Ritu -- thanks for the needinfo.
Flags: needinfo?(mreavy)
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.
Flags: needinfo?(rjesup)
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)
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 on attachment 8846490 [details] [diff] [review]
bug1335966_aurora_beta_fix.patch

Yeah, that sounds reasonable.
Flags: needinfo?(docfaraday)
Attachment #8846490 - Flags: review?(docfaraday) → review+
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+
https://hg.mozilla.org/mozilla-central/rev/335a2fee24a8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.

Attachment

General

Created:
Updated:
Size: