Network Identification

RESOLVED FIXED in Firefox 49

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: bagder, Assigned: bagder)

Tracking

Trunk
mozilla49
All
Unspecified
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox49 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8709691 [details]
network-identification-01

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

Comment 2

2 years ago
Created attachment 8711650 [details] [diff] [review]
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.
Whiteboard: [necko-active]
(Assignee)

Comment 3

a year ago
Created attachment 8739935 [details] [diff] [review]
0002-bug-1240932-figure-out-network-id-on-OS-X-r.patch

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

Comment 4

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

Windows version of getting a network id.
(Assignee)

Comment 5

a year ago
Created attachment 8739961 [details] [diff] [review]
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.
Attachment #8711650 - Attachment is obsolete: true
Attachment #8739961 - Flags: review?(mcmanus)
(Assignee)

Comment 6

a year ago
Created attachment 8739973 [details] [diff] [review]
0001-bug-1240932-add-Telemetry-to-record-network-id-succe.patch

Add Telemetry to the Linux code for counting network id success rate
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...
(Assignee)

Comment 8

a year ago
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.
the functions do IO to read from proc - that's potentially blocking right?
(Assignee)

Comment 10

a year ago
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.
(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 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...
Attachment #8739961 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 13

a year ago
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.
Attachment #8739973 - Flags: review?(mcmanus)
(Assignee)

Updated

a year ago
Attachment #8739936 - Flags: review?(mcmanus)
(Assignee)

Comment 14

a year ago
Created attachment 8744216 [details] [diff] [review]
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 :-) )
Attachment #8739935 - Attachment is obsolete: true
Attachment #8744216 - Flags: review?(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
Attachment #8739936 - Flags: review?(mcmanus)
Attachment #8739973 - Flags: review?(mcmanus) → review+
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
Attachment #8744216 - Flags: review?(mcmanus)
(Assignee)

Comment 17

a year ago
Created attachment 8746497 [details] [diff] [review]
v3-0002-bug-1240932-figure-out-network-id-on-OS-X-r-mcman.patch

OS X version updated
Attachment #8744216 - Attachment is obsolete: true
Attachment #8746497 - Flags: review?(mcmanus)
(Assignee)

Comment 18

a year ago
Created attachment 8746498 [details] [diff] [review]
v3-0003-bug-1240932-figure-out-network-id-on-Windows-r-mc.patch

updated Windows version, ripe for review.
Attachment #8739936 - Attachment is obsolete: true
Attachment #8746498 - Flags: review?(mcmanus)
Attachment #8746497 - Flags: review?(mcmanus) → review+
Attachment #8746498 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 19

a year ago
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.
Keywords: checkin-needed

Comment 20

a year ago
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
Keywords: checkin-needed

Comment 21

a year ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1271131
You need to log in before you can comment on or make changes to this bug.