Closed Bug 948757 Opened 6 years ago Closed 6 years ago

Preconnect on hover: unlimited connections to the same host

Categories

(Core :: Networking, defect, minor)

x86_64
Windows 7
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jaulmerd, Assigned: u408661, NeedInfo)

Details

Attachments

(1 file, 2 obsolete files)

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.
thanks - I can confirm on nightly. I'm guessing that it is seer related...
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.
Assignee: nobody → hurley
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8346227 - Flags: review?(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.
Attachment #8346227 - Flags: review?(mcmanus)
(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.
Now with even more code removal!
Attachment #8346227 - Attachment is obsolete: true
Attachment #8346611 - Flags: review?(mcmanus)
Attachment #8346611 - Flags: review?(mcmanus) → review+
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.
Attachment #8346611 - Attachment is obsolete: true
Attachment #8348902 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ecc8eaeac93a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Could you please verify that this is fixed for you in Firefox 29?
Flags: needinfo?(jaulmerd)
You need to log in before you can comment on or make changes to this bug.