1408487 - stun_getifaddrs is still being called in content processes after bug 1345511

Status: RESOLVED FIXED

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

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I'm seeing some crash reports where the WebRTC stack is still trying to read network interface addresses in a sandboxed content process.  Bug 1345511 should have fixed this, but there seems to be some case where it can still happen.  On non-Nightly this won't be a crash, but it might result in WebRTC (intermittently?) not working.

Example crash reports: bp-7cf0ac50-6ff5-4a89-a952-57ce30171011, bp-aa15044d-4764-4442-bdf5-d351c0171012.

Comment 1

[Byron Campen [:bwc]]

Maybe the IPC code is failing to fetch interfaces, and we're falling back?

Comment 2

[Nils Ohlmeier [:drno]]

Looks like we overlooked the path going down from here http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#3217

which still calls into the content process gatherer.

Michael can you please have a look how to fix this?

Comment 3

[Michael Froman [:mjf]]

After looking - the only option for us to be in that state does seem to be the one Byron suggests: our IPC call has returned, but it has returned 0 stun addresses.  That would be the only way we would unblock the ice ctx operation queue in PeerConnectionMedia here:
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#270

Blocking the ice operation queue until we receive the IPC response has been our firewall on additional ice calls.

Assuming that something terrible is not happening with the IPC channel itself and we are getting 0 addresses back over IPC, what is the correct response to that?  Should we retry the IPC request to gather addresses?  Should we just not unblock the queue?

The surgery will be more invasive if we need to teach nICEr that there are certain times it should not gather addresses, but we can work toward that if needed.

Thoughts?

Comment 4

[Nils Ohlmeier [:drno]]

Discussed on IRC but for the record:

I think we should change the code here http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#961 so that we return/exit with an error if we have asked the parent via IPC, but at this point |mStunAddrs| is empty. That should prevent a content process crash for now.

Then the two remaining questions are:
- what can/could we do instead of returning an error? E.g. is worth asking the parent again for interfaces - how do we prevent a loop in that case?
- why/when do we get an empty list of interfaces? For maybe we should simply add a simple Telemetry probe, which reports if the length of the returned list is zero or not (so only a boolean to avoid tracking how many interfaces peoples machines have).

Comment 5

[Byron Campen [:bwc]]

I think that if we get no interfaces, ICE should just fail, right? I mean, there are ways this could happen due to prefs being set in paranoid ways, and no STUN/TURN servers being provided, and we aren't likely to get a different answer trying again.

It might be useful having a telemetry probe for this, _especially_ if the failure to gather isn't due to prefs.

Comment 6

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #5)
> I think that if we get no interfaces, ICE should just fail, right? I mean,
> there are ways this could happen due to prefs being set in paranoid ways,
> and no STUN/TURN servers being provided, and we aren't likely to get a
> different answer trying again.

Yes, in general no IP address should result in ICE failures. But the IP handling prefs are considered way later in the process. So they should not affect the length of the array.
I guess I was thinking along the lines of that 32bit Windwos gathering problem we used to have. But even then trying again would not have helped really.

So yeah lets switch ICE connection state to failed in case of no IP address returned from the parent.
 
> It might be useful having a telemetry probe for this, _especially_ if the
> failure to gather isn't due to prefs.

Yeah I was thinking about a telemetry probe in the parent process to cover only the scenarios where the interface iterator code really fails to find anything. I can think of probably local firewalls and other non-standard software to cause this. But I would be still interested in how wide spread this problem is.

Comment 7

[Michael Froman [:mjf]]

Attachment: Bug 1408487 - handle parent process returning 0 STUN addrs.

1) don't call StartGathering in e10s mode if parent process failed to provide STUN addrs.
2) set ICE connection state to failed if parent process returns 0 STUN addrs

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

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8919887 [details]
Bug 1408487 - handle parent process returning 0 STUN addrs.

https://reviewboard.mozilla.org/r/190830/#review196040

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:974
(Diff revision 1)
> +  // attempt to gather them (as in non-e10s mode), and this will cause a
> +  // sandboxing exception in e10s mode.
> +  if (!mStunAddrs.Length() && XRE_IsContentProcess()) {
> +    CSFLogInfo(LOGTAG,
> +               "%s: No STUN addresses returned from parent process",
> +               __FUNCTION__);

I think you are missing a |return;| statement here.

Comment 9

[Michael Froman [:mjf]]

Comment on attachment 8919887 [details]
Bug 1408487 - handle parent process returning 0 STUN addrs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190830/diff/1-2/

Comment 10

[Michael Froman [:mjf]]

Comment on attachment 8919887 [details]
Bug 1408487 - handle parent process returning 0 STUN addrs.

https://reviewboard.mozilla.org/r/190830/#review196040

> I think you are missing a |return;| statement here.

Yep - I got carried away looking for where to add the connection state change that I forgot to go back and add the return.  Thank you!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/debeb703ceab
handle parent process returning 0 STUN addrs. r=drno

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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