948757 - Preconnect on hover: unlimited connections to the same host

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[jaulmerd]

Go to any non-ssl page, such as http://planet.mozilla.org/

Hover a link, Firefox opens a TCP preconnection.

Now hover the same link again (move the mouse back and forth quickly): more and more connections get open to the same host. This can easily exceed network.http.max-persistent-connections-per-server

If there are many links together, it's easy to do an unintentional SYN flood.

Comment 1

[Patrick McManus [:mcmanus]]

thanks - I can confirm on nightly. I'm guessing that it is seer related...

Comment 2

[u408661]

Attachment: 0001-Bug-948757-Don-t-flood-servers-with-bunches-of-speculative-connections.-r-mcmanus.patch

As mentioned via email - we were blindly ignoring any idle connections, instead of counting them towards the parallel speculative connection limit like we should have been. This patch is a little more liberal than the strictest version (which would sum the number of half opens & the number of idles) in that it checks both separately, meaning we could conceivably end up with 2x the speculative connection limit (either 12 or 2 connections, depending on if it's from the seer or not, respectively). The strict way is an easy modification to this patch.

I also made the seer not override any limits for hovering predictions for extra safety, and in retrospect, it doesn't make nearly as much sense to override limits for hovers as it does for active pageload predictions.

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8346227 [details] [diff] [review]
0001-Bug-948757-Don-t-flood-servers-with-bunches-of-speculative-connections.-r-mcmanus.patch

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

I agree that hover should stay at 1 - so that nullptr change is good.

we can remove all remnants of ignoreIdle from the IDL and such too, right? I'll leave the r? unresolved for that.

I think we ought to pref off seer in 28 as well and just get the whole set of related things fixed up on the trunk rather than backporting all this.

Comment 4

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> we can remove all remnants of ignoreIdle from the IDL and such too, right?
> I'll leave the r? unresolved for that.

*facepalm* of course. New patch incoming.

> I think we ought to pref off seer in 28 as well and just get the whole set
> of related things fixed up on the trunk rather than backporting all this.

Agreed. I'll update bug 948569 to be for both beta & aurora.

Comment 5

[u408661]

Attachment: 0001-Bug-948757-Don-t-flood-servers-with-bunches-of-speculative-connections.-r-mcmanus.patch

Now with even more code removal!

Comment 6

[u408661]

green try run: https://tbpl.mozilla.org/?tree=Try&rev=7371ef11eb6f

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a9654d11cb26

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5e499ab5e49d

for causing browser-chrome bustage like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=31890027&tree=Mozilla-Inbound on at least Windows 7 and 8.

Comment 8

[u408661]

Attachment: 0001-Bug-948757-Don-t-flood-servers-with-bunches-of-speculative-connections.-r-mcmanus.patch

Here's the version that will be checked in (which doesn't time-out mochitest-bc, see https://tbpl.mozilla.org/?tree=Try&rev=89345c52aa28). This fixes the specific predict on link hover issue, but doesn't fix the underlying issue (which also needs fixed). I'll file a follow-up for that work.

Comment 9

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc8eaeac93a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ecc8eaeac93a

Comment 11

[Ioana (away)]

Could you please verify that this is fixed for you in Firefox 29?