1107702 - Failure in GetAdaptersInfo causing ICE failure

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

We have a confirmed case of GetAdaptersInfo reliably failing for one of our users, which is something we've seen telltale signs of in telemetry and statistics from TokBox. We need to determine why, and whether there is something we can improve here.

Comment 1

[Byron Campen [:bwc]]

Try push that adds better logging of this failure case:

https://tbpl.mozilla.org/?tree=Try&rev=7be262a8739e

Binaries will be here once the build is done:

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7be262a8739e

Comment 2

[Tor-Einar Jarnbjo]

I seem to have the same problem with Firefox 36.0.1 running under Windows 7 64bit. No matter which WebRTC provider I try to connect with, I've tried both opentokrtc, the demos from PeerJS and a local Kurento installation, I get the following error and the session fails to setup:

(generic/ERR) Got error from GetAdaptersInfo

Is there anything I can do to help debugging the issue? Your link to the try-build seem to have expired. Would it be possible for you to run the build again and provide me with binaries with additional debug information?

Comment 3

[Tor-Einar Jarnbjo]

The issue (at least for me) seem to be fixed in 36.0.3 and 36.0.4. Looking at the release notes for 36.0.3 (https://www.mozilla.org/en-US/firefox/36.0.3/releasenotes/) there does not however seem to be any relevant changes.

Comment 4

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

Can you confirm it was a firefox change (and not a site change) by going back to 36.0.1?

It should be available from ftp.mozilla.org
http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/36.0.1/

Comment 5

[Tor-Einar Jarnbjo]

Unfortunately, it was not the Firefox update, which solved the problem. After downgrading to 36.0.1, I am now for some reason still able to use WebRTC.

During the last weeks, I have been evaluating different WebRTC frameworks and the problem (or its solution) is definitively not related to any site changes. During the last weeks, both the demos from http://opentokrtc.com and http://peerjs.com, as well as a local Kurento installation did not work with Firefox 36.0.1 on my computer, after getting the upgrade to 36.0.3 (and later 36.0.4), both the public demos as well as the local Kurento installation suddenly worked. After downgrading to 36.0.1, the public demos and the local Kurento deployment are still working.

During the problems over the past weeks, Firefox 36.0.1 used to work when testing from other computers in the same network, I could also use Chrome on my own computer, so I wouldn't assume a problem with either the sites, nor with my own network setup.

Comment 6

[Tor-Einar Jarnbjo]

I have now been able to reproduce the problem with 36.0.4 as well. The problem does not occur with a fresh instance of Firefox. When Firefox has been running for a while or I have many tabs open, GetAdaptersInfo fails quite reliably.

It does not seem to be a bug in Firefox, but a bug in the Windows kernel. At least this bug report seem to fit very well and using the attached example, I can reproduce the same problem with the bug test:

https://connect.microsoft.com/VisualStudio/feedback/details/665383/getadaptersaddresses-api-incorrectly-returns-no-adapters-for-a-process-with-high-memory-consumption

Basically, several of the network API functions fail if a 32 bit program has been compiled with /LARGEADDRESSAWARE and is currently using more than 2GB of memory. If I understand it correctly, the kernel routines for converting 32-bit addresses to 64-bit address space (may) fail if the MSB of the 32-bit address is set.

Microsoft offers a hotfix for the issue, but the hotfix must be requested by e-mail and is not automatically installed by Windows Update:

https://support.microsoft.com/de-de/kb/2588507/en

Comment 7

[Byron Campen [:bwc]]

Ugh. That's just awful. Sounds pretty plausible though, but not sure what we can do about it aside from distributing 64-bit binaries for windows.

Comment 8

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

This is *extremely* unfortunate....  And 64-bit won't be shipping for a while I believe

Benjamin: any ideas how one could work around it?  Or who might know?  (there may be no way to do so...)

Comment 9

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

The only solution I can think of right now (short of shipping 64-bit Firefox always) is to drop the /LARGEADDRESSAWARE linker flag, and reduce our max memory available :-(

Comment 10

[Tor-Einar Jarnbjo]

I am not sure if Windows offer other functions to get the configuration of the network interfaces. GetAdaptersInfo has been deprecated since Windows XP and has other drawbacks as well. It does e.g. not support IPv6 addresses and will fail, if the computer is connected only with IPv6. I am not sure if the rest of the WebRTC implementation supports IPv6, but IPv6 support should IMHO by now be taken for granted.

Unfortunately, the API documentation of GetAdaptersInfo points to GetAdaptersAddresses as a suitable replacement for the deprecated function and GetAdaptersAddresses is affected by the mentioned bug as well.

Comment 11

[Benjamin Smedberg]

I'm going to redirect you to dmajor and jmathies, our resident Windows experts. If we can construct a minimal testcase, this is something that we can file with Microsoft and ask for paid support.

Comment 12

[Jim Mathies [:jimm]]

Hmm, not finding much on the web. Have we confirmed these failures fail with the right failure code so we know the kb article issue is to blame?

Chrome is using GetAdaptersAddresses heavily, and it looks like they have LARGEADDRESSAWARE set. I wonder if they ran into this? I don't see much of a trail if they did from searching bugs.

We might want to start by converting over to the GetAdaptersAddresses api.

https://code.google.com/p/chromium/codesearch#search/&q=GetAdaptersAddresses&sq=package:chromium&type=cs
https://code.google.com/p/chromium/codesearch#search/&q=LARGEADDRESSAWARE&sq=package:chromium&type=cs
https://code.google.com/p/chromium/issues/list?can=1&q=GetAdaptersAddresses&colspec=ID+Pri+M+Week+ReleaseBlock+Cr+Status+Owner+Summary+OS+Modified&x=m&y=releaseblock&cells=tiles

Comment 13

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

My understanding was that Chrome ships 64-bit binaries (only) for Windows, so this wouldn't affect them.  Also, even if they're shipping 32-bit-large-address binaries, it would only hit them if a single process in Chrome calling these functions that's using >2GB; their multi-process approach makes this much less likely to bite them.

So Chrome not dealing with it doesn't really help us figure out a solution.

And per the article, GetAdaptersAddresses is apparently hit as well.

Comment 14

[Daniel Stenberg [:bagder]]

We use GetAdaptersAddresses in nsNotifyAddrListener which does the link service, checking network interfaces for network availability. Although we don't have any logs for if that function call suddenly starts to fail...

Comment 15

[Tor-Einar Jarnbjo]

Attachment: Minimal test case: GetAdaptersInfoTest.cpp

I've attached a minimal test case to show the behaviour. I allocate a lot of smaller memory blocks (about 2.4 GB) and then try to invoke GetAdaptersInfo, which always fail with error code 998 on my system.

For some reason, it is important to allocate multiple smaller memory blocks and not one large chunk to provoke the bug.

Comment 16

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

> For some reason, it is important to allocate multiple smaller memory blocks
> and not one large chunk to provoke the bug.

Perhaps that avoids holes in the memory use below the 2GB line

Comment 17

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

Attachment: add error values on GetAdaptersInfo failures

Comment 18

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

Tor-Einar: care to try this?  (note, includes some sctp/datachannels patches)
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=478820f27613
(when green, click on the Build: entry in lower-left, and download a build to install in another directory)
Use the ICE logging setup from
https://wiki.mozilla.org/Media/WebRTC/Logging

You should see GetAdaptersInfo() fail, and get the result and address it was using.

Comment 19

[Daniel Stenberg [:bagder]]

(In reply to Tor-Einar Jarnbjo from comment #15)

> I've attached a minimal test case to show the behaviour. I allocate a lot of
> smaller memory blocks (about 2.4 GB) and then try to invoke GetAdaptersInfo,
> which always fail with error code 998 on my system.

Does it make a difference which of these memory blocks that is used to GetAdaptersInfo? I mean what if you _first_ allocate a buffer to use with GetAdaptersInfo and then you spend the rest of the 2GB memory. Does that work? If so, we could just make sure to pick a buffer much earlier and reserve for that purpose...

Comment 20

[Tor-Einar Jarnbjo]

@Randell: The build process for the patch with additional debug output seem to have failed.

@Daniel: No, unfortunately it does not help to allocate the buffer for GetAdaptersInfo before allocating more memory. Even if the MSB of the passed buffer address is not set, GetAdaptersInfo will then still fail if too much memory is used:

Got error from GetAdaptersInfo 998 at 0058E0B0

(0058E0B0 is here the pointer to pAdapterInfo)

Perhaps GetAdaptersInfo allocates more memory internally and is prone to the same problem independent of the address of the result buffer. 

It would anyway be difficult to know how much memory to be allocated in advance. The number of adapters returned by GetAdaptersInfo may very well change during the lifetime of the Firefox process, since it only returns info about active network adapters with assigned IPv4 addresses. Activating wi-fi adapters, enabling VPNs or starting virtual machines with virtual network adapters while Firefox is running, will increase the number of adapters, which would later be returned by GetAdaptersInfo .

Comment 21

[Jim Mathies [:jimm]]

Can we use this test case to see if GetAdaptersAddresses is more resilient than GetAdaptersInfo?

Comment 22

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tor-Einar Jarnbjo from comment #20)
> Perhaps GetAdaptersInfo allocates more memory internally and is prone to the
> same problem independent of the address of the result buffer. 

It should be possible to check this by stepping through the code and seeing what the function calls

Comment 23

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

(In reply to Jim Mathies [:jimm] from comment #21)
> Can we use this test case to see if GetAdaptersAddresses is more resilient
> than GetAdaptersInfo?

I imagine we can try it, but as the KB article mentions GetAdaptersAddresses, I doubt it will better.

Comment 24

[Tor-Einar Jarnbjo]

The KB article describes the problem to apply to "various network-related APIs" and mentions explicitely GetAdaptersAddresses as one of the affected functions. Even if the same test case wouldn't show any obvious problems with GetAdaptersAddresses, one should assume that the same problem apply to GetAdaptersAddresses as well and perhaps only occur under different circumstances.

Comment 25

[(Away)]

Thanks for the needinfo. I want to dig into this with a debugger and see if there's any possible workaround. I'm currently on the road but I will try to take a look within a few days.

Comment 26

[(Away)]

GetAdaptersInfo makes some heap allocations of its own, so I guess that explains why pre-allocating a pAdaptersInfo doesn't help. 

But maybe we could pre-allocate some heap ballast to drop before the call. I tried that, and it didn't work, but: the test case might not be a fair representation of Firefox memory usage. We use jemalloc, which avoids the Windows heap APIs. 

To better simulate Firefox address space usage, I changed the leaked malloc calls to VirtualAlloc, and then the ballast trick started working. I don't know whether it will hold up in practice, but it might be worth a try.

Comment 27

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

(In reply to Tor-Einar Jarnbjo from comment #20)
> @Randell: The build process for the patch with additional debug output seem
> to have failed.

I think the logs for that are available even in open builds, however you only need the "Windows XP" builds anyways, and they succeeded (and win64 now has too).  I have no doubt this is the problem you're hitting with ICE, though.

Comment 28

[Tor-Einar Jarnbjo]

(In reply to Randell Jesup [:jesup] from comment #27)

> I think the logs for that are available even in open builds, however you
> only need the "Windows XP" builds anyways, and they succeeded (and win64 now
> has too).  I have no doubt this is the problem you're hitting with ICE,
> though.

It is unfortunately not quite trivial to reproduce the problem with Firefox. I have to open a lot of tabs, make sure that much memory has been used and in most cases even then, keep using Firefox a while before the problem occurs. Since the invokation of GetAdaptersInfo in my test code is more or less just a copy of the Firefox code, just with the possibility to allocate memory in advance in a controlled manner, I am not sure either if it is very important for you that I am able to reproduce the behaviour from Firefox itself?

Comment 29

[(Away)]

While playing around I found that calling GetAdaptersInfo before leaking memory will make it continue to work after leaking memory. Perhaps there is some one-time initialization happening under the hood. (This is with my VirtualAlloc leaks; it doesn't work if I leak malloc, but I still think that's not a good comparison to real FF)

Jesup would you be willing to do a try build that primes GetAdaptersInfo early in the process? I don't know where to put the call myself.

Comment 30

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

Attachment: hackish patch to touch GetAdaptersInfo at startup on Vista/Win7

Give this a try...

Comment 31

[Tor-Einar Jarnbjo]

I unfortunately don't have an environment, in which I can build Firefox so providing me a patch won't help. Are you able to trigger a build somewhere on your build servers and make an installer available?

Comment 32

[(Away)]

Do you have a try build for Tor-Einar?

Comment 33

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0f8535b8225
Should be done in an hour or two

Comment 34

[Tor-Einar Jarnbjo]

Did the builds on https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0f8535b8225 fail, or am I just not able to find any download links?

Comment 35

[(Away)]

The builds succeeded. Once a build is green, you can click the "B" and look in the bottom-left panel for a line like "Build: x86_64 windows8-64 win".

Comment 36

[Masatoshi Kimura [:emk]]

(In reply to David Major [:dmajor] from comment #35)
> The builds succeeded. Once a build is green, you can click the "B" and look
> in the bottom-left panel for a line like "Build: x86_64 windows8-64 win".

x86_64? Isn't this bug about 32 bit binaries?

Comment 37

[(Away)]

> x86_64? Isn't this bug about 32 bit binaries?
You're right, I'm getting my WebRTC bugs confused!

Comment 38

[Tor-Einar Jarnbjo]

Clicking on the green 'B' first shows a failure summary, which I assumed ment that the build had failed. I've downloaded and installed the build now, but I am unfortunately not able to get the memory usage of the Firefox process very high.

Plugins are in the new Firefox release obviously hosted by a separate process (plugin-container.exe) and also the Flash player is started as a separate process (Flash-Player-Plugin_XYZ.exe).

Running multiple processes will of course by itself at least mitigate the original problem with the GetAdaptersInfo function, since the Firefox process is now unlikely to reach a size > 2GB.

Comment 39

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

(In reply to Tor-Einar Jarnbjo from comment #38)
> Clicking on the green 'B' first shows a failure summary, which I assumed
> ment that the build had failed.

Click on a green B.  Then click on the "Build: ..." line at lower-left.  Red B means it's failed; orange means it completed but some post-build tests failed.

> I've downloaded and installed the build now,
> but I am unfortunately not able to get the memory usage of the Firefox
> process very high.
> 
> Plugins are in the new Firefox release obviously hosted by a separate
> process (plugin-container.exe) and also the Flash player is started as a
> separate process (Flash-Player-Plugin_XYZ.exe).
> 
> Running multiple processes will of course by itself at least mitigate the
> original problem with the GetAdaptersInfo function, since the Firefox
> process is now unlikely to reach a size > 2GB.

It still will do so, even in e10s mode (content + master processes) if a site leaks (I've seen since tabs with >1GB of memory allocated).

For more ease in hitting this, you can disable e10s mode (go to preferences).  I regularly get to 3GB used on windows (I'm a tab-hoarder, and some sites leak a fair bit over time (I'm looking at you, huffington post!)

Comment 40

[Tor-Einar Jarnbjo]

I am not sure if it is related to the current problem, but trying to use WebRTC in the provided build after disabling E10S and under high memory usage, Firefox several times crash and quit completely. The first error dialog mentions an APPCRASH in plugin_container.exe, module mozglue.dll. I have attached the complete error report, but I don't see anything in the data indicating what the problem might be:

AdapterDeviceID: 0x0416
AdapterDriverVersion: 10.18.10.3960
AdapterSubsysID: 65021558
AdapterVendorID: 0x8086
Add-ons: %7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D:40.0a1,firebug%40software.joehewitt.com:2.0.9
AsyncPluginInit: 0
AvailablePageFile: 22121254912
AvailablePhysicalMemory: 6747631616
AvailableVirtualMemory: 1159532544
BIOS_Manufacturer: American Megatrends Inc.
BlockedDllList: 
BreakpadReserveAddress: 47251456
BreakpadReserveSize: 67108864
BuildID: 20150413143612
CrashTime: 1429524812
EMCheckCompatibility: true
EventLoopNestingLevel: 1
FramePoisonBase: 00000000f0de0000
FramePoisonSize: 65536
InstallTime: 1429192997
Notes: AdapterVendorID: 0x8086, AdapterDeviceID: 0x0416, AdapterSubsysID: 65021558, AdapterDriverVersion: 10.18.10.3960
Has dual GPUs. GPU #2: AdapterVendorID2: 0x10de, AdapterDeviceID2: 0x1341, AdapterSubsysID2: 65021558, AdapterDriverVersion2: 9.18.13.4725D2D? D2D1.1? D2D1.1+ D2D+ DWrite? DWrite+ D3D11 Layers? D3D11 Layers+ 
ProductID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
ProductName: Firefox
ReleaseChannel: default
StartupTime: 1429523902
SystemMemoryUsePercentage: 60
Theme: classic/1.0
Throttleable: 1
TotalPageFile: 34115006464
TotalPhysicalMemory: 17058451456
TotalVirtualMemory: 4294836224
URL: http://localhost:8080/
Vendor: Mozilla
Version: 40.0a1
Winsock_LSP: MSAFD-Tcpip [TCP/IP] : 2 : 1 : %SystemRoot%\system32\mswsock.dll 
 MSAFD-Tcpip [UDP/IP] : 2 : 2 :  
 MSAFD-Tcpip [RAW/IP] : 2 : 3 : %SystemRoot%\system32\mswsock.dll 
 MSAFD-Tcpip [TCP/IPv6] : 2 : 1 :  
 MSAFD-Tcpip [UDP/IPv6] : 2 : 2 : %SystemRoot%\system32\mswsock.dll 
 MSAFD-Tcpip [RAW/IPv6] : 2 : 3 :  
 RSVP-TCPv6-Dienstanbieter : 2 : 1 : %SystemRoot%\system32\mswsock.dll 
 RSVP-TCP-Dienstanbieter : 2 : 1 :  
 RSVP-UDPv6-Dienstanbieter : 2 : 2 : %SystemRoot%\system32\mswsock.dll 
 RSVP-UDP-Dienstanbieter : 2 : 2 :  
 MSAFD RfComm [Bluetooth] : 2 : 1 : %SystemRoot%\system32\mswsock.dll
useragent_locale: en-US

This report also contains technical information about the state of the application when it crashed.

Comment 41

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

We need to figure out some way to unblock this bug.  dmajor, ideas?  

Tor-einar, have you been able to verify if that build worked or not?  I can do a new Try if it's no longer available.  What's your steps-to-reproduce, so we can try to create it locally and catch it in a debugger?

Thanks!

Comment 42

[Tor-Einar Jarnbjo]

@Randell As I wrote in my most recent comment, I am not able to reproduce the problem with the given build, since the build crashes, obviously due to other reasons, before I can get far enough to test.

There are no definitive and reliable steps to reproduce the issue. The bug occurs often (but not reliably reproduceable) when the Firefox process is using more than 2GB of memory and a WebRTC session is initiated.

Comment 43

[(Away)]

I'm going to assume that the crashes were unrelated and if we try again, the crash may be fixed. Randell could you spin another build? Preferably with a baseline before bug 77999 so that we can test high-memory more easily. Maybe even simulate Aurora or Beta to avoid interference from unstable Nightly features? Let's add MOZ_CRASHREPORTER_UPLOAD_FULL_SYMBOLS in case we do still need to debug crashes.

Perhaps we can make the repro more reliable (or at least less time-consuming) by getting us over the 2GB line very quickly. Nick, what are your favourite memory-devouring pages/addons?

Comment 44

[Nicholas Nethercote [inactive]]

> Nick, what are your favourite memory-devouring pages/addons?

My favourite was always TechCrunch, but they recently changed things so it's much slimmer than it used to be, alas.

dev.w3.org/html5/spec/single-page.html is enormous.

If you want more control, probably the best thing is to write a small JS function that allocates lots of typed arrays and stuffs them into a global structure. That would allow you to know exactly how much memory has been allocated.

Comment 45

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

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eef399117fe

That's from the patch before 77999 landed, with my patch and full symbols

Comment 46

[Tor-Einar Jarnbjo]

The build provided by Randell on June 30th does not work either. Firefox refuses to start with an "Could not find the Mozilla runtime" error dialog.

Comment 47

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

Tor: How did you install?  I just downloaded and installed it (to a new directory).  I opened a shell, cd'd to the new directory, then did "firefox.exe -profilemanager -no-remote" (since I have other Firefoxes running).  I downloaded the installer version by clicking on the WinXP opt 'B' (debug should work too, though slower), clicking on the Build: link in the lower left, and download the windows installer (~40MB).

Thanks!

Comment 48

[Tor-Einar Jarnbjo]

I downloaded the Win XP debug installer, ran the installer (also with installing to a fresh directory) and tried to start firefox.exe from the explorer. I am now on holidays for the next weeks and have during that time no access to a computer on which I can do further tests.

Comment 49

[Maire Reavy [:mreavy]]

Randell - can you push a new Try?  I marked the patch for review by jimm, since dmajor is no longer around.  Please NI Tor when it's available to try.

Jimm - I think we should see if this can resolve the problem, since Microsoft apparently won't push this fix out.

Comment 50

[Jim Mathies [:jimm]]

Attachment: cleaned up patch

pushed to try, but I didn't have a chance to build this locally due to a clobber.

lets skip all the debug stuff and just get an opt build going for testing purposes.

Comment 51

[Jim Mathies [:jimm]]

Attachment: cleaned up patch

This time with the actual patch.

Comment 52

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cdccbae7823

Comment 53

[Jim Mathies [:jimm]]

Tor-Einar, is there any chance you might have the time to test this build for issues?

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-2cdccbae7823777a18e5269737003239640caed9/try-win32/

One way to do this would be to install this build in a custom location using the exe installer. An even simpler method: download the 'firefox-45.0a1.en-US.win32.zip' zipped build, once downloaded open the zip file and drag the 'firefox' folder onto your desktop. Shutdown firefox and run the firefox.exe in the newly created firefox folder and test.

Comment 54

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

Comment on attachment 8681925 [details] [diff] [review]
cleaned up patch

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

::: dom/media/MediaManager.cpp
@@ +1637,5 @@
> +    // first(?) call occurs after the process size is over 2GB (kb/2588507).
> +    // Attempt to 'prime' the pump by making a call at startup.
> +    DWORD result;
> +    unsigned long out_buf_len = sizeof(IP_ADAPTER_INFO);
> +    PIP_ADAPTER_INFO pAdapterInfo = (IP_ADAPTER_INFO *)moz_xmalloc(out_buf_len);

Trivial nit: space before moz_xmalloc() in keeping with convention (at least in this file/dir)

@@ +1639,5 @@
> +    DWORD result;
> +    unsigned long out_buf_len = sizeof(IP_ADAPTER_INFO);
> +    PIP_ADAPTER_INFO pAdapterInfo = (IP_ADAPTER_INFO *)moz_xmalloc(out_buf_len);
> +    if ((result = GetAdaptersInfo(pAdapterInfo, &out_buf_len)) == ERROR_BUFFER_OVERFLOW) {
> +      pAdapterInfo = (IP_ADAPTER_INFO *)moz_xmalloc(out_buf_len);

ditto

::: layout/build/nsLayoutStatics.cpp
@@ +264,5 @@
>      return rv;
>    }
>  
>    AsyncLatencyLogger::InitializeStatics();
> +  MediaManager::StartupInit();

Is it safe to remove mozilla/MediaManager.h from the include list?  I prefer not to set up future random bustage when the build-system chunking changes.

Comment 55

[Jim Mathies [:jimm]]

It's been 4 months so we might have lost Tor. If you all want to land this feel free, there is evidence that it helped.

Comment 56

[Tor-Einar Jarnbjo]

I am not lost, but I am unfortunately busy with a completely different project now and don't have a reasonable setup accessible where I can easily test the fix. Sorry about that.

Comment 57

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

Nils, this is the bug I was talking about re: possible lack of remote candidates.  Would this show up in failed ice due to no local candidates?

Comment 58

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

We should consider landing this and see if it helps anything.

Comment 59

[Nils Ohlmeier [:drno]]

Yes this was the bug I was thinking of, but the error signature does not match all with what I'm seeing in the Loop logs. I think these are two distinct problems. I'll file a new bug for the problem I see in the logs.

Comment 60

[Nils Ohlmeier [:drno]]

For cross-reference purposes: bug 1229633 is the Windows gather problem we see in the Loop client error logs.

Comment 61

[Jim Mathies [:jimm]]

Comment on attachment 8681925 [details] [diff] [review]
cleaned up patch

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

::: dom/media/MediaManager.cpp
@@ +1643,5 @@
> +      pAdapterInfo = (IP_ADAPTER_INFO *)moz_xmalloc(out_buf_len);
> +      result = GetAdaptersInfo(pAdapterInfo, &out_buf_len);
> +    }
> +    if (result == ERROR_SUCCESS) {
> +      free(pAdapterInfo);

I think this code has a leak in it. We allocate pAdapterInfo, then if the buffer isn't large enough, we allocate again rather than resize. The first allocation leaks if we make the second allocation, the second allocation leaks if the second call to GetAdaptersInfo fails for some reason. I'll touch this up and push to try.

Comment 62

[Jim Mathies [:jimm]]

Attachment: patch for landing

built and tested locally on win7 so I don't see a need for a try run. jesup, just mark the bug with checkin-needed if you consider the patch r+.

Comment 63

[Jim Mathies [:jimm]]

Attachment: patch for landing

- removed MediaManager.h include

Comment 64

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

Comment on attachment 8704665 [details] [diff] [review]
patch for landing

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

r+ with one nit

::: dom/media/MediaManager.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 "MediaManager.h"
> -

Why is this removed?  Generally we want to include the appropriate file; you may be being saved by Unified Build pulling it in from another file.

Restore it.

Comment 65

[Jim Mathies [:jimm]]

(In reply to Randell Jesup [:jesup] from comment #64)
> Comment on attachment 8704665 [details] [diff] [review]
> patch for landing
> 
> Review of attachment 8704665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with one nit
> 
> ::: dom/media/MediaManager.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 "MediaManager.h"
> > -
> 
> Why is this removed?  Generally we want to include the appropriate file; you
> may be being saved by Unified Build pulling it in from another file.
> 
> Restore it.

oh, I misinterpreted your comment in 54. I removed a header from the wrong file.

Comment 66

[Jim Mathies [:jimm]]

Attachment: patch for landing

Comment 67

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0324a62e156c

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0324a62e156c

Comment 69

[Nils Ohlmeier [:drno]]

FYI according to the Firefox Hello ICE failure reports this really seem to have fixed the problem in 46, as the subsequent error "Error getting buf len from GetAdaptersAddresses()" (which replaced the original error after IPv6 support had been added to WebRTC - this changed the WebRTC call from GetAdaptersInfo() to GetAdaptersAddresses()) appears to have disappeared in >=46 according to my crunched stats: http://people.mozilla.org/~nohlmeier/report.html?minversion=45#2

Note: a little preliminary as 46 hasn't made it into release yet, but as the error was present a lot in Aurora and Beta and appears to gone in 46 I'm optimistic.