1240932 - Network Identification

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Daniel Stenberg [:bagder]]

Get a semi-reliable identification hash for the network (situation) Firefox runs in.

Should become the same number when returning back to the identical or very similar network conditions. Should *not* be the same when changing networks between home/work/school/coffee shop.

Comment 1

[Daniel Stenberg [:bagder]]

Attachment: network-identification-01

Initial draft outlining what the network identification is and how it could work.

Comment 2

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1240932-figure-out-network-id-on-Linux-r.patch

Here's a first shot at calculating a network id on Linux to serve as a basis for further discussions. It takes the MAC address of default gateway, adds a string to it and gets a sha1 hash: the id. Not sure we need the added string.

Comment 3

[Daniel Stenberg [:bagder]]

Attachment: 0002-bug-1240932-figure-out-network-id-on-OS-X-r.patch

Here's the initial OS X version for doing the same magic.

Comment 4

[Daniel Stenberg [:bagder]]

Attachment: 0003-bug-1240932-figure-out-network-id-on-Windows-r.patch

Windows version of getting a network id.

Comment 5

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1240932-figure-out-network-id-on-Linux-r-mcmanus.patch

Ok, let the reviewing begin. Starting casually with the Linux version. What do you think about this approach mr Mcmanus?

The network id itself is not used anywhere on purpose. I will submit a separate patch that adds telemetry that only gathers info about the success rate of figuring out network ids.

Comment 6

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1240932-add-Telemetry-to-record-network-id-succe.patch

Add Telemetry to the Linux code for counting network id success rate

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8739961 [details] [diff] [review]
0001-bug-1240932-figure-out-network-id-on-Linux-r-mcmanus.patch

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

what's the threading situation here? As it can obviously block...

Comment 8

[Daniel Stenberg [:bagder]]

The new code doesn't actually introduce any new blocking (if it does, it is a bug). If it can't find the default gateway or if the default gateway's IP is not found in the ARP cache, the functions will simply fail to extract the netid.

The general idea is to run the calculateNetworkId() function when a network change has been detected, in the "link monitoring" thread that is already setup and running for the sole purpose of checking for network changes.

Comment 9

[Patrick McManus [:mcmanus]]

the functions do IO to read from proc - that's potentially blocking right?

Comment 10

[Daniel Stenberg [:bagder]]

Ah, ok gotcha. Well, just to make sure I took a peek in the kernel code to see how it actually provides this data.

The only thing I can see that can block while read those proc files is the use of locks in the kernel for each entry that is read and I would assume that they should never hold the lock for a very long time as then I guess other things would break down.

Or is there a more global way it can block? As this is done in the link monitoring thread, a short block shouldn't be problematic.

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to Daniel Stenberg [:bagder] from comment #10)

>  As this is done in the link
> monitoring thread, a short block shouldn't be problematic.

that's the answer I was hoping for :)

Touching any FS does make stuff block a surprising amount of the time... but as this is on the link monitoring thread its fine..

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8739961 [details] [diff] [review]
0001-bug-1240932-figure-out-network-id-on-Linux-r-mcmanus.patch

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

what's the threading situation here? As it can obviously block...

Comment 13

[Daniel Stenberg [:bagder]]

Comment on attachment 8739973 [details] [diff] [review]
0001-bug-1240932-add-Telemetry-to-record-network-id-succe.patch

How about counting netid matches like this?

The idea being to see how common the "not found" case turns out to be and possibly also to make sure that the new netid matches the old one a decent amount of times.

Comment 14

[Daniel Stenberg [:bagder]]

Attachment: v2-0002-bug-1240932-figure-out-network-id-on-OS-X-r-mcman.patch

Corrected and rebased OS X version of the functionality. Patrick, I'm mostly looking for a review for style and general approach, as I realize you're not an OS X guy. I don't think a lot of OSX knowledge is required though, if you just assume the API calls and usage is correct.

I've worked out the API logic and tested it separately in my github repo for figuring out MAC address of default gateway (https://github.com/bagder/gw-mac) so it should be working okay.

Once this is okayed, I will submit a Telemetry counter patch for OSX too, much in the same style as already is provided in the patch for the Windows specific netid code. (Assuming we first agree that patch is fine :-) )

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8739936 [details] [diff] [review]
0003-bug-1240932-figure-out-network-id-on-Windows-r.patch

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

a few small things...

::: netwerk/system/win32/nsNotifyAddrListener.cpp
@@ +153,5 @@
>    *aLinkType = nsINetworkLinkService::LINK_TYPE_UNKNOWN;
>    return NS_OK;
>  }
>  
> +static bool macaddr(BYTE addr[], DWORD len, char *str, size_t strlen)

let's not call it strlen to avoid overlapping symbols

@@ +156,5 @@
>  
> +static bool macaddr(BYTE addr[], DWORD len, char *str, size_t strlen)
> +{
> +    str[0] = '\0';
> +    if (addr == NULL || !len || (len*3 > strlen)) {

!addr rather than == NULL
len space * space 3

@@ +160,5 @@
> +    if (addr == NULL || !len || (len*3 > strlen)) {
> +        return false;
> +    }
> +
> +    for (DWORD i = 0; i < len; i++) {

coding guidelines has this as ++i.. nit

@@ +161,5 @@
> +        return false;
> +    }
> +
> +    for (DWORD i = 0; i < len; i++) {
> +        sprintf_s(str+(i*3), sizeof(str+(i*3)),

spacing in all of this

@@ +168,5 @@
> +    }
> +    return true;
> +}
> +
> +void nsNotifyAddrListener::findmac(char *gateway)

case on method names

@@ +191,5 @@
> +                if (!macaddr(pIpNetTable->table[i].bPhysAddr,
> +                             pIpNetTable->table[i].dwPhysAddrLen,
> +                             hw, sizeof(hw)))
> +                    // failed to get the MAC
> +                    continue;

braces on if

@@ +225,5 @@
> +// returns 0 when the gw is found and stored
> +static bool defaultgw(char *gateway) // at least 128 bytes buffer
> +{
> +    PMIB_IPFORWARDTABLE pIpForwardTable =
> +        (MIB_IPFORWARDTABLE *)malloc(sizeof (MIB_IPFORWARDTABLE));

that's infallible malloc but you checked the return value. This is small so infallible is ok.

more to the point though, use a smart pointer and you don't have to worry about cleanup and early return.. probably uniqueptr and makeunique

@@ +249,5 @@
> +
> +    bool found = false;
> +    DWORD retVal = GetIpForwardTable(pIpForwardTable, &dwSize, 0);
> +    if (retVal == NO_ERROR) {
> +        for (int i = 0; i < (int) pIpForwardTable->dwNumEntries; i++) {

nit ++i

should this be casted or should i be unsigned? use c++ cast if cast is necessary

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8744216 [details] [diff] [review]
v2-0002-bug-1240932-figure-out-network-id-on-OS-X-r-mcman.patch

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

::: netwerk/system/mac/nsNetworkLinkService.mm
@@ +168,5 @@
> +    mib[2] = 0;
> +    mib[3] = AF_INET;
> +    mib[4] = NET_RT_FLAGS;
> +    mib[5] = RTF_LLINFO;
> +    if (sysctl(mib, 6, NULL, &needed, NULL, 0) < 0)

braces on every conditional (many of these in the patch.. I won't mention them all)

@@ +174,5 @@
> +    if (needed == 0)        /* empty table */
> +        return false;
> +    buf = NULL;
> +    for (;;) {
> +        newbuf = static_cast<char *>(realloc(buf, needed));

smartptr/uniqueptr and no need to check infallible allocations

@@ +219,5 @@
> +    if (sysctl(mib, 6, NULL, &needed, NULL, 0) < 0) {
> +        return 1;
> +    }
> +
> +    if ((buf = (char *)malloc(needed)) == NULL) {

smartptr .. no need to null check infallible allocations

Comment 17

[Daniel Stenberg [:bagder]]

Attachment: v3-0002-bug-1240932-figure-out-network-id-on-OS-X-r-mcman.patch

OS X version updated

Comment 18

[Daniel Stenberg [:bagder]]

Attachment: v3-0003-bug-1240932-figure-out-network-id-on-Windows-r-mc.patch

updated Windows version, ripe for review.

Comment 19

[Daniel Stenberg [:bagder]]

Try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c20a26bc69aaa4db4252945492f2088eb92a3486&selectedJob=19958644

Let's see if we can land this with the first telemetry logic only applied in the linux parts to see that it starts ticking in as presumed.

I'll create a follow-up bug for additional netid telemetry for the additional platforms and also additional metrics.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83e7a178b826
https://hg.mozilla.org/integration/mozilla-inbound/rev/c015afbb2b7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d5b6d6d3f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa730410c52c

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83e7a178b826
https://hg.mozilla.org/mozilla-central/rev/c015afbb2b7c
https://hg.mozilla.org/mozilla-central/rev/e6d5b6d6d3f5
https://hg.mozilla.org/mozilla-central/rev/aa730410c52c