1643440 - Crash in [@ nr_stun_server_client_create]

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P3)

Description

[Byron Campen [:bwc]]

This bug is for crash report bp-03f46b9a-1b24-43c8-afc9-6df000200604.

Top 10 frames of crashing thread:

0 xul.dll nr_stun_server_client_create media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c:108
1 xul.dll nr_stun_server_add_client media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c:126
2 xul.dll nr_ice_component_pair_candidates media/mtransport/third_party/nICEr/src/ice/ice_component.c:1185
3 xul.dll nr_ice_media_stream_pair_candidates media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:284
4 xul.dll nr_ice_peer_ctx_pair_candidates media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c:412
5 xul.dll mozilla::NrIceCtx::StartChecks media/mtransport/nricectx.cpp:961
6 xul.dll mozilla::MediaTransportHandlerSTS::StartIceChecks::<unnamed-tag>::operator const media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp:667
7 xul.dll mozilla::MozPromise<bool, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, 0>::ThenValue<`lambda at /builds/worker/checkouts/gecko/media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp:651:7', `lambda at /builds/worker/checkouts/gecko/media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp:673:7'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:730
8 xul.dll mozilla::MozPromise<CopyableTArray<bool>, nsresult, 0>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:405
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1200

Looks like a nullptr crash.

Comment 1

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 5 socket and utility process crashes on release

:bwc, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Comment 2

[Byron Campen [:bwc]]

It looks like this only happens when ICE logging is turned up to DEBUG. Looking at the stack, this probably happens because |ctx| is null. Nothing else uses |ctx| in that function, so it seems like we probably don't want to be using |ctx| there?

Comment 3

[Byron Campen [:bwc]]

Hmm, maybe |ctx| is not the problem here. If logging is not set to DEBUG, the caller ought to crash right after the call to nr_stun_server_client_create, but I do not see any crashes there in crash-stats. Maybe it is |ctx->label|?

Comment 4

[Byron Campen [:bwc]]

Ok, this looks really sketchy, but I don't think this is the cause of this particular crash. Probably needs a fix.

https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_component.c#406-407,409,411,424

If |nr_ice_component_create_stun_server_ctx| succeeds, it takes a reference to |isock_tmp->stun_server|. If |nr_ice_candidate_create| then fails, we destroy |isock_tmp|, and by extension |isock_tmp->stun_server|, leaving a dangling pointer. However, the pointer we're getting in the crash-stack is not coming from the reference taken by |nr_ice_component_create_stun_server_ctx|, it is coming from |nr_ice_component::sockets|, which it only ends up in if both of the functions I mentioned succeed.

Comment 5

[Byron Campen [:bwc]]

Honestly, the only way I can see us crashing here is if |ctx| is null. I see a handful of crashes on 106, but they all seem to be coming from a single user. Further, their extension loadout indicates that they are a web-developer of some sort (has @react-devtools, for example). Additionally, they have [e-mail] installed, which hints that this might be someone closely affiliated with mozilla, perhaps even an employee.

Comment 6

[Byron Campen [:bwc]]

When widening my crash-stats search, I see some crashes like this:

https://crash-stats.mozilla.org/signature/?signature=nr_stun_server_remove_client&date=%3E%3D2022-07-26T15%3A23%3A00.000Z&date=%3C2022-10-26T15%3A23%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports

This is also a case where we have an nr_ice_socket with a null stun_server. I think there must just be some rare cases where we fail to populate that. I can see a number of possibilities here and here.

I think we may want to call |nr_ice_component_create_stun_server_ctx| immediately after calls to |nr_ice_socket_create|, or maybe even build that work into |nr_ice_socket_create| itself.

Comment 7

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=2fe86765006907bd852c9f9b27cdd98e072f4401
https://treeherder.mozilla.org/jobs?repo=try&revision=57200fc04b3deafa15c943bfc20a9f904d0ed771
https://treeherder.mozilla.org/jobs?repo=try&revision=0da26adaea9c5f24425f5433815d480b852b180c

Comment 8

[Byron Campen [:bwc]]

Try looks about like normal.

Comment 9

[Byron Campen [:bwc]]

Attachment: Bug 1643440: Fix some potential nullptr crash issues. r?mjf

Comment 10

[BugBot [:suhaib / :marco]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit auto_nag documentation.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7de0c3949881
Fix some potential nullptr crash issues. r=mjf

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7de0c3949881