1235509 - Refreshing IPv6 addresses kills all TCP connections

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[u559402]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151210085006

Steps to reproduce:

When an IPv6 router advertisement is received, Firefox closes open sockets, causing data loss.

This is the case even if the router advertisement doesn't actually change the IPv6 addresses, but only refreshes the lifetimes of existing addresses. On an IPv6-capable network, this happens fairly frequently (once every few minutes or once every few seconds).

See similar problem on Chromium: https://code.google.com/p/chromium/issues/detail?id=100690


Actual results:

Firefox closes TCP sockets.


Expected results:

Firefox sockets are closed only if the addresses really changed.

Comment 1

[Dragana Damjanovic [:dragana]]

This probably has something to do with network link change events.
Asking Daniel.


i could take a look too

Comment 2

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38a73b6f5d7d

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_1235509.patch

Comment 4

[Daniel Stenberg [:bagder]]

doesn't this risk of growing a bit too much if we never evict old entries from that hash? Like for a long-running browser on a system that changes IPv6 address very frequently?

Comment 5

[Dragana Damjanovic [:dragana]]

(In reply to Daniel Stenberg [:bagder] from comment #4)
> doesn't this risk of growing a bit too much if we never evict old entries
> from that hash? Like for a long-running browser on a system that changes
> IPv6 address very frequently?

We also listen to RTM_DELADDR so addresses will be deleted too. If it is only a change to an existing ip address we are not making a new entry.

Comment 6

[Daniel Stenberg [:bagder]]

Oh right. So the hash contains added addresses and removed addresses get removed from the hash again. So it basically stops sending network change requests if RTM_NEWADDR arrives and it actually wasn't a new address but one we already have. Right? Seems fine!

Is this really what happens? We subscribe to IP address and route changes on the netlink socket, I don't understand why it signals us without an actual change. But it looks like this was exactly what Chrome did to their code to avoid the similar problem. (https://src.chromium.org/viewvc/chrome/trunk/src/net/base/address_tracker_linux.cc)

It smells like this could also be related to which Linux kernel that's used too, potentially even how it has been configured/built.

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Daniel Stenberg [:bagder] from comment #6)
> Oh right. So the hash contains added addresses and removed addresses get
> removed from the hash again. So it basically stops sending network change
> requests if RTM_NEWADDR arrives and it actually wasn't a new address but one
> we already have. Right? Seems fine!
> 

yes.

> Is this really what happens? We subscribe to IP address and route changes on
> the netlink socket, I don't understand why it signals us without an actual
> change. But it looks like this was exactly what Chrome did to their code to
> avoid the similar problem.
> (https://src.chromium.org/viewvc/chrome/trunk/src/net/base/
> address_tracker_linux.cc)
> 

It changes the lifetime of ipv6v address but we should not care about these changes
valid_lft or preferred_lft changes

> It smells like this could also be related to which Linux kernel that's used
> too, potentially even how it has been configured/built.

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_1235509_v2.patch

Comment 9

[Daniel Stenberg [:bagder]]

As I'm not a peer in the networking code (yet), I'd like to get a formal OK from someone like :mcmanus  so that I can review and r+ it properly !

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Daniel Stenberg [:bagder] from comment #9)
> As I'm not a peer in the networking code (yet), I'd like to get a formal OK
> from someone like :mcmanus  so that I can review and r+ it properly !

go ahead

Comment 11

[Daniel Stenberg [:bagder]]

Comment on attachment 8705124 [details] [diff] [review]
bug_1235509_v2.patch

It looks good!

What could possibly be added, at least when working for this particular case, is a more dedicated log output for when an identical address is detected (and ignored) as it would more positively prove that we have addressed the problem we suspect is happening.

Comment 13

[Patrick McManus [:mcmanus]]

as discussed in the necko meeting - we're going to pref off the linux link monitor code for 45 and 44 under only Linux (if allowed, but I tihnk its a good idea) and pursue the fix on the trunk only. That will make 44 work like 42 - essentially reintroducing the long standing problems resuming from sleep that 43 fixed, but fixing this problem that hits you when you aren't doing anything unusual like sleeping or moving. :bagder and ::dd.mozilla are taking responsibility for making that happen.

Comment 15

[u559402]

Setting network.notify.changed pref to false really fixes this problem on Firefox v43 installed from Ubuntu repos. Thank you!

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug_1235509_v3.patch

only logging added

Comment 19

[Dragana Damjanovic [:dragana]]

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a498eb69fa4

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a498eb69fa4