1312304 - Crash in mozilla::NrIceCtx::Initialize

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-6791f524-f616-46f2-8b74-17d022161023.
=============================================================

Low-volume crash, has only happened 9 times across two installations, all Mac. It's a MOZ_CRASH() on an unexpected path (with ICE_POLICY_NONE).

bwc, any ideas?

Comment 1

[Byron Campen [:bwc]]

I think we should just remove the MOZ_CRASH.

Comment 2

[Byron Campen [:bwc]]

Attachment: Bug 1312304: Don't MOZ_CRASH when we use ICE policy "none".

Review commit: https://reviewboard.mozilla.org/r/88114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88114/

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8803900 [details]
Bug 1312304: Don't MOZ_CRASH when we use ICE policy "none".

https://reviewboard.mozilla.org/r/88114/#review87090

We can go ahead with this as it will remove the crash, although I would prefer a better solution if someone uses "none" as iceTransportPolicy value (see below).

::: media/mtransport/nricectx.cpp:539
(Diff revision 1)
>    UINT4 flags = offerer_ ? NR_ICE_CTX_FLAGS_OFFERER:
>        NR_ICE_CTX_FLAGS_ANSWERER;
>    flags |= NR_ICE_CTX_FLAGS_AGGRESSIVE_NOMINATION;
>    switch (policy_) {
>      case ICE_POLICY_NONE:
> -      MOZ_CRASH();
> +      // This will suppress gathering and ICE checks.

From my own brief testing this case apparently happens when you try to use "none" as your iceTransportPolicy value. Values like "foobar" are rejected with a TypeError on the console log. I'm wondering if it would make more sense if "none" would also result in the same TypeError message, rather then this probably somewhat unexpected behavior.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8803900 [details]
Bug 1312304: Don't MOZ_CRASH when we use ICE policy "none".

https://reviewboard.mozilla.org/r/88114/#review87090

> From my own brief testing this case apparently happens when you try to use "none" as your iceTransportPolicy value. Values like "foobar" are rejected with a TypeError on the console log. I'm wondering if it would make more sense if "none" would also result in the same TypeError message, rather then this probably somewhat unexpected behavior.

Hmm, let me just remove the "none" value from the enum, since it isn't in the spec anymore, and it will fox this bug.

Comment 5

[Byron Campen [:bwc]]

Attachment: Bug 1312304: Remove ICE policy "none".

Review commit: https://reviewboard.mozilla.org/r/88172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88172/

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8803992 [details]
Bug 1312304: Remove ICE policy "none".

https://reviewboard.mozilla.org/r/88172/#review87116

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8803992 [details]
Bug 1312304: Remove ICE policy "none".

https://reviewboard.mozilla.org/r/88172/#review87188

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf39193c8b6e
Remove ICE policy "none". r=drno,smaug

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bf39193c8b6e

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[Tracking Requested - why for this release]: crash regression

Crash introduced in https://hg.mozilla.org/mozilla-central/rev/2f5a249215f6#l1.16

STR: Run http://jsfiddle.net/jib1/j3dyfczd/

Still affects web-platform-tests [1]

We should uplift this to 51. Byron can you request uplift?

[1] https://github.com/w3c/web-platform-tests/pull/4062#issuecomment-255577096

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8803992 [details]
Bug 1312304: Remove ICE policy "none".

Approval Request Comment
[Feature/regressing bug #]:

   Bug 1297416

[User impact if declined]:

   Web content can cause crashes in debug builds at will (MOZ_CRASH).

[Describe test coverage new/current, TreeHerder]:

   No new test coverage.

[Risks and why]:

   Quite low. We've removed functionality on this one.

[String/UUID change made/needed]:

   None.

Comment 12

[Gerry Chang [:gchang]]

Un-track for 51 as low volume of crashes.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8803992 [details]
Bug 1312304: Remove ICE policy "none".

Fix a crash. Take it in 51 aurora.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/245d3d9c8c22