867933 - [WebRTC] Enumerate network interfaces for e10s

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[StevenLee[:slee]]

For e10s, we need mechanism to list all network interfaces from parent process.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 1 - get interface list in content process.

Using cpmm.sendSyncMesssage to get network interfaces from NetworkManager in content process. I am still verifying if this works in necko's thread (and when main thread is blocked).

Comment 2

[Shian-Yow Wu [:swu]]

As my understanding, the reason to list all network interfaces is because it's required by the ICE module during initialization.
https://mxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#130

I have a question, why do we need to list all network interfaces, instead of going through default route?

On B2G, the external network interface could Wifi or 3G.  And there could be more than one 3G connections in multiple APN condition.  For example, one APN for internet access, one for MMS, and one for A-GPS.  There is only one active interface(Wifi by default) and default route will be set on that interface.

For B2G, maybe we only need to pass the active network interface to WebRTC?

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Shian-Yow Wu from comment #2)
> On B2G, the external network interface could Wifi or 3G.  And there could be
> more than one 3G connections in multiple APN condition.  For example, one
> APN for internet access, one for MMS, and one for A-GPS.  There is only one
> active interface(Wifi by default) and default route will be set on that
> interface.
> 
> For B2G, maybe we only need to pass the active network interface to WebRTC?

Agree, we should remove AGPS and MMS interfaces from the interface list we pass to WebRTC.

AFAIK, interface of data connection will be turned down when wifi up. In such case, we will only pass wifi interface (because data connection down). But if there are two or more interfaces are up at the same time, I think we should pass all of them to WebRTC (except AGPS/MMS interfaces).

Comment 4

[Eric Rescorla (:ekr)]

I don't know the details about how B2G networking is implemented, but
let's talk first about the desired behavior.

Say I have a device with both a 4G interface and WiFi and I'm in an
area where I both have live WiFi service and 4G connectivity. If
there is a NAT/Firewall on the WiFi that blocks UDP but not on the 4G,
I would like to be able to set up an ICE connection over the 4G.
Similarly, if the 4G blocks UDP, I would like to be able to set up
an ICE connection over the wireless. This is true even if HTTPS
works fine over the other interface.

Now, when we translate this into terms of interfaces, I suspect this
means that you need to return any interface that is potentially
able to transmit data, even if it's not your current default.

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 2 - addrs.c for gonk

Implement nr_stun_get_addrs() for gonk. The interface list we get from NetworkManager doesn't contain loopback interface, so aDropLoopback is not implemented.

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - addrs.c for gonk

Add a build time flag to addrs.c to remove nr_stun_get_addrs() when we build with Gonk. Keeping addrs.c beacuse we will still need its nr_stun_remove_duplicate_addrs() function.

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - get interface list in content process.

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - addrs.c for gonk

Comment 9

[Shian-Yow Wu [:swu]]

On B2G, there could be the case that interface not exist at the time been queried.

Take Unagi device for example.
Wifi: No wlan0 interface before Wifi enabled.  Because wifi driver module not been loaded if Wifi not enabled.
Data connection: Many interfaces (rmnet0 ~ rmnet7).  We don't know which one will be used for Internet access before data connection established.  Normally rmnet0, but could be different if rmnet0 been occupied by non-Internet APNs(MMS, A-GPS...).

root@android:/ # netcfg
lo       UP                                   127.0.0.1/8   0x00000049 00:00:00:00:00:00
rmnet0   DOWN                                   0.0.0.0/0   0x00000000 00:00:00:00:00:00
rmnet1   DOWN                                   0.0.0.0/0   0x00000000 00:00:00:00:00:00
rmnet2   DOWN                                   0.0.0.0/0   0x00000000 00:00:00:00:00:00
rmnet3   DOWN                                   0.0.0.0/0   0x00001002 ee:43:5d:48:8e:00
rmnet4   DOWN                                   0.0.0.0/0   0x00001002 0a:f4:da:b6:66:ee
rmnet5   DOWN                                   0.0.0.0/0   0x00001002 7e:5a:10:27:f6:8c
rmnet6   DOWN                                   0.0.0.0/0   0x00001002 9e:96:25:d6:92:eb
rmnet7   DOWN                                   0.0.0.0/0   0x00001002 de:3c:ac:7b:a4:0b
tunl0    DOWN                                   0.0.0.0/0   0x00000080 00:00:00:00:00:00
sit0     DOWN                                   0.0.0.0/0   0x00000080 00:00:00:00:00:00
ip6tnl0  DOWN                                   0.0.0.0/0   0x00000080 00:00:00:00:00:00

If a user enables data connection to run WebRTC, the rmnet0 will be active interface for Internet access.  Later on if the user enabled Wifi, the rmnet0 will be turned down, and wlan0 interface will be added.

For this reason, we need another mechanism to notify ICE module when there is interface change, and ICE initialization should be run again.  How do you think?

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - addrs.c for gonk

Enumerate network interfaces with B2G's network manager.

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 751567 [details] [diff] [review]
Patch: Part 2 - addrs.c for gonk

Review of attachment 751567 [details] [diff] [review]:
-----------------------------------------------------------------

pwang: a bunch of suggestions for improvement here.

::: media/mtransport/gonk_addrs.cpp
@@ +2,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifdef MOZ_B2G_RIL

Would it make more sense to conditionally compile tis file on gonk rather than always
compile it but have it sometimes be a no-op.

@@ +20,5 @@
> +#include <sys/socket.h>
> +#include "r_types.h"
> +#include "csi_platform.h"
> +#include "stun.h"
> +#include "addrs.h"

Can this list be trimmed down? I wonder if we need netdb.h, for instance?

@@ +31,5 @@
> +namespace {
> +
> +typedef struct {
> +  struct sockaddr_in addr;
> +  char name[64];

Suggest using std::string here.

@@ +34,5 @@
> +  struct sockaddr_in addr;
> +  char name[64];
> +} NetworkInterface;
> +
> +class GetNetworkInterfaceList: public nsRunnable {

Suggest using WrapRunnableNMRet() rather than creating a new class.

I.e.,

nsresult GetInterfaces(std::vector<NetworkInterface>* interfaces);

And have it return NS_OK for success and something else for failure.

@@ +38,5 @@
> +class GetNetworkInterfaceList: public nsRunnable {
> +public:
> +  nsresult Run()
> +  {
> +    // Obtain network intefaces from network manager.

intefaces->interfaces.

@@ +43,5 @@
> +    nsresult rv;
> +    nsCOMPtr<nsINetworkInterfaceList> networkList;
> +    nsCOMPtr<nsINetworkInterfaceListService> listService =
> +      do_GetService("@mozilla.org/network/interface-list-service;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

This doesn't bubble up to the caller. Suggest instead setting an error flag and returning NS_OK. This won't be necessary if you take my advice about about WrapRunnable.

@@ +44,5 @@
> +    nsCOMPtr<nsINetworkInterfaceList> networkList;
> +    nsCOMPtr<nsINetworkInterfaceListService> listService =
> +      do_GetService("@mozilla.org/network/interface-list-service;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    listService->GetDataInterfaceList(getter_AddRefs(networkList));

Can this fail?

@@ +57,5 @@
> +    for (int32_t i = 0; i < listLength; i++) {
> +      NetworkInterface &interface = mNetworkArray.ElementAt(i);
> +      nsCOMPtr<nsINetworkInterface> iface;
> +      rv = networkList->GetInterface(i, getter_AddRefs(iface));
> +      NS_ENSURE_SUCCESS(rv, rv);

You shouldn't be returning here b/c you are leaving the list in an inconsistent state.

@@ +62,5 @@
> +
> +      memset(&(interface.addr), 0, sizeof(interface.addr));
> +      interface.addr.sin_family = AF_INET;
> +      iface->GetIp(ip);
> +      inet_pton(AF_INET, NS_ConvertUTF16toUTF8(ip).get(),

Check for error here.

@@ +68,5 @@
> +
> +      iface->GetName(ifaceName);
> +      strncpy(interface.name,
> +              NS_ConvertUTF16toUTF8(ifaceName).get(),
> +              sizeof(interface.name));

Check for the string being too long.

@@ +95,5 @@
> +                                      sizeof(interface.addr),
> +                                      IPPROTO_UDP, 0, &(aAddrs[n]))) {
> +      NS_WARNING("Problem transforming address");
> +    } else {
> +      strlcpy(aAddrs[n].ifname, interface.name,

Why strncpy() above and strlcpy() here?

@@ +102,5 @@
> +    }
> +  }
> +
> +  *aCount = n;
> +  nr_stun_remove_duplicate_addrs(aAddrs, aDropLoopback, aCount);

Check for error.

::: media/mtransport/third_party/nICEr/nicer.gyp
@@ +215,5 @@
>                    'WEBRTC_GONK',
>                    'NO_REG_RPC',
>                  ],
>               }],
> +             # Gonk has its own addrs.c implementation.

s/addrs.c/nr_get_stun_addrs/, right?

Comment 12

[Shian-Yow Wu [:swu]]

Comment on attachment 749771 [details] [diff] [review]
Patch: Part 1 - get interface list in content process.

Review of attachment 749771 [details] [diff] [review]:
-----------------------------------------------------------------

Improvements listed below per discussion.

::: dom/system/gonk/NetworkInterfaceListService.js
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const NETWORKLISTSERVICE_CONTRACTID = "@mozilla.org/network/interface-list-service;1";
> +const NETWORKLISTSERVICE_CID = Components.ID("{3780be6e-7012-4e53-ade6-15212fb88a0d}");

More than 80 characters in a line.

@@ +16,5 @@
> +                                   "nsISyncMessageSender");
> +
> +function NetworkInterfaceListService () {
> +  cpmm.addMessageListener('NetworkInterfaceList:ListInterface:OK', this);
> +  cpmm.addMessageListener('NetworkInterfaceList:ListInterface:KO', this);

The two listeners not been used.

@@ +28,5 @@
> +  _id: 0,
> +
> +  _requests: {},
> +
> +  getDataInterfaceList: function() {

Can we avoid hard-coding exclueSupl/excludeMms inside, and use a function name better representing the purpose?

@@ +29,5 @@
> +
> +  _requests: {},
> +
> +  getDataInterfaceList: function() {
> +    let currentId = this._id++;

currentId, _id, _requests not been used.

@@ +51,5 @@
> +    return this._interfaces.length;
> +  },
> +
> +  getInterface: function(index) {
> +    if (!this._interfaces) return null;

Move return to next line with braces.

::: dom/system/gonk/NetworkInterfaceListService.manifest
@@ +1,3 @@
> +# NetworkInterfaceListService.js
> +component {3780be6e-7012-4e53-ade6-15212fb88a0d} NetworkInterfaceListService.js
> +contract @mozilla.org/network/interface-list-service;1 {3780be6e-7012-4e53-ade6-15212fb88a0d}

Please add license information in the beginning of this file.

::: dom/system/gonk/NetworkManager.js
@@ +293,5 @@
>    },
>  
> +  receiveMessage: function receiveMessage(aMsg) {
> +    switch (aMsg.name) {
> +    case "NetworkInterfaceList:ListInterface": {

Two spaces indentation.

@@ +319,5 @@
> +          httpProxyPort: i.httpProxyPort
> +        });
> +      }
> +      return interfaces;
> +      break;

This break after return is not needed.

::: dom/system/gonk/nsINetworkInterfaceListService.idl
@@ +18,5 @@
> +[scriptable, uuid(eaad3425-e2dc-418d-a91e-a93e5bd27977)]
> +interface nsINetworkInterfaceListCallback : nsISupports
> +{
> +  void onInterfaceListGot(in nsINetworkInterfaceList list);
> +};

nsINetworkInterfaceListCallback is not been used.

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - nr_stun_get_addrs for gonk

Comment 14

[Eric Rescorla (:ekr)]

Comment on attachment 752044 [details] [diff] [review]
Patch: Part 2 - nr_stun_get_addrs for gonk

Review of attachment 752044 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good. What's the state of the patch overall? I.e., do you want to try
to land it?

::: media/mtransport/gonk_addrs.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "nsINetworkManager.h"
> +#include "nsINetworkInterfaceListService.h"
> +#include "nsTArray.h"

No longer needed.

@@ +14,5 @@
> +#include "addrs.h"
> +}
> +
> +#include <vector>
> +#include <string>

Please push C++ system headers above mozilla aystem headers.

@@ +29,5 @@
> +
> +nsresult
> +GetInterfaces(std::vector<NetworkInterface>* aInterfaces)
> +{
> +  if (!aInterfaces) {

Suggest MOZ_ASSERT()

@@ +60,5 @@
> +
> +    memset(&(interface.addr), 0, sizeof(interface.addr));
> +    interface.addr.sin_family = AF_INET;
> +    iface->GetIp(ip);
> +    if (inet_pton(AF_INET, NS_ConvertUTF16toUTF8(ip).get(),

We don't need v6 support now, but what's your eventual plan?

@@ +76,5 @@
> +} // anonymous namespace
> +
> +int
> +nr_stun_get_addrs(nr_transport_addr aAddrs[], int aMaxAddrs,
> +                  int aDropLoopback, int *aCount)

Please be consistent about */& placement. Above it is on the left.

@@ +84,5 @@
> +
> +  // Get network interface list.
> +  std::vector<NetworkInterface> interfaces;
> +  NS_DispatchToMainThread(mozilla::WrapRunnableNMRet(&GetInterfaces, &interfaces, &rv),
> +                          NS_DISPATCH_SYNC);

Check for failure to dispatch.

@@ +86,5 @@
> +  std::vector<NetworkInterface> interfaces;
> +  NS_DispatchToMainThread(mozilla::WrapRunnableNMRet(&GetInterfaces, &interfaces, &rv),
> +                          NS_DISPATCH_SYNC);
> +  if (NS_FAILED(rv)) {
> +    return 1;

This should return some R_* error. E.g., R_FAILED.

@@ +99,5 @@
> +    }
> +    if (nr_sockaddr_to_transport_addr(reinterpret_cast<sockaddr*>(&(i->addr)),
> +                                      sizeof(i->addr),
> +                                      IPPROTO_UDP, 0, &(aAddrs[n]))) {
> +      NS_WARNING("Problem transforming address");

I would suggest that this should cause a failure rather than skipping.
Also, you should log an error.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #14)
> This looks pretty good. What's the state of the patch overall? I.e., do you
> want to try to land it?

Yeah. I will need to fix some review comments, but the patch basically works. I think it's almost ready.

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #14)
> We don't need v6 support now, but what's your eventual plan?
Network manager doesn't support either. So I don't have plan to implement v6 at this moment.

Comment 17

[Eric Rescorla (:ekr)]

OK.

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - get interface list in content process.

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - nr_stun_get_addrs for gonk

Comment 20

[Shian-Yow Wu [:swu]]

Comment on attachment 752543 [details] [diff] [review]
Patch: Part 1 - get interface list in content process.

Review of attachment 752543 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good to me.

::: dom/system/gonk/nsINetworkInterfaceListService.idl
@@ +28,5 @@
> +
> +  /**
> +   * Obtain a list of network interfaces that satisfy the specified condition.
> +   * @param condition flags that specify the interfaces to be returned. This
> +   *        can be or combation of LIST_* flags, or zero to make all available

s/or combation/OR combination/

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 752545 [details] [diff] [review]
Patch: Part 2 - nr_stun_get_addrs for gonk

Review of attachment 752545 [details] [diff] [review]:
-----------------------------------------------------------------

This is getting close, but I'd like to see one more round.

::: media/mtransport/gonk_addrs.cpp
@@ +19,5 @@
> +#include "nsThreadUtils.h"
> +#include "nsServiceManagerUtils.h"
> +
> +namespace {
> +typedef struct {

C++ does not require typedef here.

@@ +31,5 @@
> +  MOZ_ASSERT(aInterfaces);
> +
> +  // Obtain network interfaces from network manager.
> +  nsresult rv;
> +  nsCOMPtr<nsINetworkInterfaceList> networkList;

Please move this down to location of first use.

@@ +67,5 @@
> +      continue;
> +    }
> +
> +    iface->GetName(ifaceName);
> +    interface.name = NS_ConvertUTF16toUTF8(ifaceName).get();

Can this fail?

@@ +86,5 @@
> +  // Get network interface list.
> +  std::vector<NetworkInterface> interfaces;
> +  if (NS_FAILED(NS_DispatchToMainThread(
> +                    mozilla::WrapRunnableNMRet(&GetInterfaces, &interfaces, &rv),
> +                    NS_DISPATCH_SYNC)) || NS_FAILED(rv)) {

I would prefer you break this out into two tests.

@@ +95,5 @@
> +  int32_t n = 0;
> +  for (std::vector<NetworkInterface>::iterator i = interfaces.begin();
> +       i != interfaces.end(); ++i) {
> +    if (aMaxAddrs <= n) {
> +      break;

Let's just compare the length against aMaxAddrs at the top.

I.e.

size_t num_interfaces = min(aMaxAddrs.interfaces.size());
for (size_t = 0; i<num_interfaces; ++i) {
...
}

@@ +97,5 @@
> +       i != interfaces.end(); ++i) {
> +    if (aMaxAddrs <= n) {
> +      break;
> +    }
> +    if (nr_sockaddr_to_transport_addr(reinterpret_cast<sockaddr*>(&(i->addr)),

Isn't this struct sockaddr?

@@ +98,5 @@
> +    if (aMaxAddrs <= n) {
> +      break;
> +    }
> +    if (nr_sockaddr_to_transport_addr(reinterpret_cast<sockaddr*>(&(i->addr)),
> +                                      sizeof(i->addr),

Should this be sizeof(struct sockaddr)?

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #21)
> > +    if (nr_sockaddr_to_transport_addr(reinterpret_cast<sockaddr*>(&(i->addr)),
> Isn't this struct sockaddr?
it's a sockaddr_in actually.

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - nr_stun_get_addrs for gonk

Thanks for the review.

Update patch according to comment 21.

Comment 24

[Eric Rescorla (:ekr)]

Comment on attachment 757286 [details] [diff] [review]
Patch: Part 2 - nr_stun_get_addrs for gonk

Review of attachment 757286 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with nits below

::: media/mtransport/gonk_addrs.cpp
@@ +9,5 @@
> +#include "stun.h"
> +#include "addrs.h"
> +}
> +
> +#include <algorithm>

Why is this needed? Please remove if not.

@@ +72,5 @@
> +
> +    if (NS_FAILED(iface->GetName(ifaceName))) {
> +      continue;
> +    }
> +    interface.name = NS_ConvertUTF16toUTF8(ifaceName).get();

I still don't see an error check here. Is one needed?

@@ +104,5 @@
> +  int32_t n = 0;
> +  size_t num_interface = std::min(interfaces.size(), (size_t)aMaxAddrs);
> +  for (size_t i = 0; i < num_interface; ++i) {
> +    NetworkInterface &interface = interfaces[i];
> +    if (nr_sockaddr_to_transport_addr(reinterpret_cast<sockaddr*>(&(interface.addr)),

Since we are interacting with C-style structures, don't we want
"reinterpret_cast<struct sockaddr *>"

Comment 25

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #24)
> > +    interface.name = NS_ConvertUTF16toUTF8(ifaceName).get();
> 
> I still don't see an error check here. Is one needed?

Per asking on IRC, NS_ConvertUTF16toUTF8 won't produce invalid UTF8 string. (It asserts if input string is in wrong format and crashes when fail to allocate memory)

Comment 26

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1 - get interface list in content process. check-in

rebased

Comment 27

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2 - nr_stun_get_addrs for gonk check-in

Address comment 24 and rebase.

Comment 28

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/projects/birch/rev/911c71b4f301
https://hg.mozilla.org/projects/birch/rev/2fe36f68b55d

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Patrick Wang [:kk1fff] from comment #25)
> (In reply to Eric Rescorla (:ekr) from comment #24)
> > > +    interface.name = NS_ConvertUTF16toUTF8(ifaceName).get();
> > 
> > I still don't see an error check here. Is one needed?
> 
> Per asking on IRC, NS_ConvertUTF16toUTF8 won't produce invalid UTF8 string.
> (It asserts if input string is in wrong format and crashes when fail to
> allocate memory)

A more detailed look:

Per nsUTF8Utils.h:421, it calls NS_ERROR(), which calls NS_DebugBreak().  NS_ERROR does nothing in non-debug builds.  Not truly an assertion (NS_ASSERTION(), though in practice the act the same).

In an opt build, it will stop calculating the length there, and then copy.  Copy should also hit an error (line 304) and return (and ignore any further data).  However, the calling code will not know it failed, and in fact has no easy way to know it failed.

If the source is known not to be potentially "bad", it's probably not a problem, and a bad input string will just truncate at the bad character.  So I doubt this use needs to be changed.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/911c71b4f301
https://hg.mozilla.org/mozilla-central/rev/2fe36f68b55d

Comment 31

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Randell Jesup [:jesup] from comment #29)
> If the source is known not to be potentially "bad", it's probably not a
> problem, and a bad input string will just truncate at the bad character.  So
> I doubt this use needs to be changed.

If we don't know failure of convert, is there a way to check the input utf-16 character? (Sorry, I shouldn't push this before getting this response, I will add a patch to fix this.)

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

In nsReadableUtils.cpp:150, NS_ASSERTION is called when length of result utf8 string != expected length.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Both should fail at the same point, and thus the assertion probably won't fire.

In any case, the assertion does not cause a crash unless you set a variable, and it does nothing in non-debug builds (per my previous comment).

Even if the source may have bad characters in it, the only affect would be truncation, which is likely not a problem, and would just cause a failure to find the interface.  And the sources for the interface names are probably not user-code-modifiable anyways, but even if they are I don't think it's an issue.

I commented because the statement made seemed incorrect, but I think in practice (as with *most* other calls to NS_ConvertUTF16...()), it doesn't matter.