New issue
Advanced search Search tips

Issue 848045 link

Starred by 2 users
Status: WontFix
Owner: ----
Closed: Jun 2018
Cc:
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

[remoting] Too many NetworkManager instances

Project Member Reported by lambroslambrou@chromium.org, May 30 2018 
A new NetworkManager instance is created per connection channel (so, for the Pepper/NaCl client, there is one instance for "mux" channel and one for "video" channel.
Since the NetworkManager is responsible for enumerating local network interfaces (and signaling changes to it), there should only be one singleton instance.

Right now, we create a new instance in ChromiumPortAllocatorFactory:
https://cs.chromium.org/chromium/src/remoting/protocol/chromium_port_allocator_factory.cc?l=22&rcl=6be8acd7b09a2378d8c47ca9fa91b9fd9b4839ba

And in PepperPortAllocatorFactory:
https://cs.chromium.org/chromium/src/remoting/client/plugin/pepper_port_allocator_factory.cc?l=25&rcl=a45859e2bafed9e482e61aceb2ab0cb7efee4ab5

We probably want to keep one PortAllocator per channel, but they should share the same NetworkManager instance.

Note that the NetworkManager fires a WebRTC signal when the network list is changed. But that signal is only a change-notification; there is no way for any signal receiver to know which NetworkManager instance holds the new list - which is another reason it's important for it to be a singleton.

Comment 1 by lambroslambrou@chromium.org, Jun 6 2018

Status: WontFix (was: Untriaged)
It looks like this problem is specific to the NaCl plugin.

Here are the places we create a new PortAllocator:
https://cs.chromium.org/chromium/src/remoting/protocol/ice_transport_channel.cc?l=88&rcl=ee61b7eaea0133a4c47a65bd66a263d08ea8df5b
https://cs.chromium.org/chromium/src/remoting/protocol/webrtc_transport.cc?l=298&rcl=ee61b7eaea0133a4c47a65bd66a263d08ea8df5b

So, for non-WebRTC connections, it's one per channel.
And for WebRTC connections, it's one per connection, which is what we want.

Our PortAllocator owns the NetworkManager, which follows the same ownership model as in Chromium here:
https://cs.chromium.org/chromium/src/content/renderer/p2p/port_allocator.h?l=41&rcl=16d6164b1b7a2bc7a6920be8af1b59b947fe3c99

So, this is already working-as-intended for WebRTC connections, and I don't think it's worth cleaning up the NaCl code. Marking as WontFix.
►

Sign in to add a comment