252118 - necko ignores settings for max-persistent-connections-per-server

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Stephan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7) Gecko/20040707 Firefox/0.9.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7) Gecko/20040707 Firefox/0.9.2

A few days ago, I updated to Fx 0.9.2 after uninstalling 0.8.

While everything was fine with 0.8, Fx 0.9 seems to ignore the individual
settings for the # of max. concurrent downloads. No matter what number I put in
my user.js for 'network.http.max-persistent-connections-per-server', Fx does not
allow more than two concurrent downloads from the same server.

When I type 'about:config', the settings are shown correctly (e.g. an integer
value of 8 for the above mentioned), but Fx simply ignores them.



Reproducible: Always
Steps to Reproduce:
1. Install Fx 0.9.2
2. Go to www.mozilla.org/products/mozilla1.x/ for example.
3. Try to download all three different versions of Mozilla at the same time.

Actual Results:  
The third download does not start at once, but only after the first one is
finished or one of the first two is aborted. It is impossible to download more
than two files from the same server concurrently.

Expected Results:  
Start the third download immediately due to the settings in user.js & prefs.js:

user_pref("network.http.max-persistent-connections-per-server", 8);

The bug occurs independently of the theme in use and the installed extensions.

Comment 1

[Stephan]

Important note:

The problem is NOT caused by a server-side download restriction! A lot of people
(including myself) have noticed that (see MozillaZine Forum for example), even
on websites where Fx 0.8 or other browsers can download more than two files
simultaneously.

Comment 2

[Michael Lefevre]

This WFM with the Firefox 0.9.1 release build (win2000).

Comment 3

[Darin Fisher]

please provide a mozilla HTTP log per instructions here:

http://www.mozilla.org/projects/netlib/http/http-debugging.html

thanks!

Comment 4

[Michael Lefevre]

Attachment: http log

http log

set http://uk.altn.com/AntiVirus/Release/ to be home page
started Firefox
clicked /AntiVirus/Release/av224_en.exe, download started
clicked /AntiVirus/Release/av224_fr.exe, download started
clicked /AntiVirus/Release/av224_ge.exe, nothing happened
canceled one of the downloads in progress, dialog for third download appeared
and download commenced when OK'd

Comment 5

[Michael Lefevre]

log was with a current aviary branch installer build on win2k.

I've just gone back a bit - as I said, it worked for me using 0.9.1 release
build, but using the aviary branch build from 20040703, it doesn't seem to work.

Comment 6

[Remy]

Just FYI...   It seems that max-persistent-connections-per-server is locked at 2
and max-connections-per-server is locked at 8.   You can toggle between the 2
behaviors by toggling keep-alive.  I can provide supporting http traces as well
as an Ethereal trace if it'll help further.

Comment 7

[Stephan]

Here's some more information, as some users in the German firefox forum found out:

By changing the values for max. connections and/or pipelining manually in the
file all.js in the "Mozilla Firefox/greprefs" directory, Fx accepts those
settings after the next restart and behaves like it should. Thus it seems that
this is the only file which values are accepted.

Changes made to the user.js, prefs.js or directly under about:config are shown
correctly, but they do not have any effect.

Comment 8

[Darin Fisher]

Line 1552 of nsHttpHandler.cpp has this comment:

  // XXX should probably shutdown and init the conn mgr.

So, it would seem that observing pref changes, which apply to connection
management, is not implemented.  If you hand edit the same pref in the all.js
that ships with Firefox (inside the greprefs directory), then the pref would be
honored.

The solution to this bug is to shutdown and re-init the http connection manager.
 That should be a trivial patch.

Comment 9

[Darin Fisher]

Attachment: v1 patch

This should work.  I haven't rigorously tested it yet, but it should do the
trick.	If you have the ability to compile Mozilla for yourself, then please
give this patch a try.	Thanks!

Comment 10

[Darin Fisher]

OK, I suspect this bug may have been triggered by subtle changes in the timing
between profile selection and HTTP handler initialization.  If it ever happened
in the past that the profile was selected and prefs.js was parsed prior to the
HTTP handler being initialized, then this bug would not be seen.  (The exception
is when modifying the pref via about:config ... it would not take affect until
after a restart.)  It seems that today in firefox, the HTTP handler is
initialized before prefs.js has been parsed from the profile directory.  It's
unclear what is causing this.  I'm not sure it is important to know why this was
caused, since this bug exists when using about:config to change the pref during
a browser session.

Note: even if we have a lot of HTTP prefs in prefs.js, restarting the connection
manager should be relatively inexpensive.  Initialization is very cheap,
amounting to little more than a call to do_GetService, and shutdown is likewise
cheap when there are no connections or pending transactions to abort.

Comment 11

[Remy]

The ordering problem must have been fixed since FF 0.9.2 was released because
the AVIARY nightlies will obey the settings if the brower is restarted.   

I just applied to the patch to my local AVIARY snapshot and it seems to have
fixed the restart issue.

thanks.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154664 [details] [diff] [review]
v1 patch

So let me see if I understand this right.  With this patch, if I change my
accept-language pref that will cancel all http transactions that are in
progress at the time?

Comment 13

[Darin Fisher]

> So let me see if I understand this right.  With this patch, if I change my
> accept-language pref that will cancel all http transactions that are in
> progress at the time?

yes.  do you think that is unacceptable?

a more involved version would call a new method on nsHttpConnectionMgr with the
updated parameter (or maybe all parameters in one batch).  the conn mgr would
post an event to the socket thread containing the new parameter(s).  that event
would then set the new parameters on the conn mgr.  my fear is that the conn mgr
might get into a bad state if the parameters are reduced in value.  at first
glance, i don't think that will be the case ;-)

Comment 14

[Stephan]

Sorry, I am not a developer and thus my question may sound a bit dumb to you...
but what was actually the reason to change the behaviour of the conn mgr from Fx
0.8 to 0.9? Wouldn't it be possible to simply implement the connection
management as it was in 0.8?

Comment 15

[Michael Lefevre]

Leaving it so that some of the connection prefs only take effect with a restart
seems acceptable to me, if the solution is otherwise complicated.

But that would bring us back to the problem you originally reported where the
prefs don't get picked up on startup - the connection management hasn't actually
changed to cause that bug.  I guess that problem could be fixed elsewhere (in
the profile loading stuff) instead?  Unless it's got fixed already (I haven't
tested, but that's what Remy said). 

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

something like this patch is still needed, so that changes of the prefs take
effect immediately, not only after a restart.

> might get into a bad state if the parameters are reduced in value.

hm.. ideally it would not care, leave those connections open and don't accept
new ones while there are more connections than configured :) or it could kill
"idle" connections (keep-alive with no current request).

Why would this need to happen via an event on the socket transport thread?
Couldn't it just acquire a lock and set the member vars?

Comment 17

[Darin Fisher]

> something like this patch is still needed, so that changes of the prefs take
> effect immediately, not only after a restart.

agreed

> > might get into a bad state if the parameters are reduced in value.
> 
> hm.. ideally it would not care, leave those connections open and don't accept
> new ones while there are more connections than configured :) or it could kill
> "idle" connections (keep-alive with no current request).

"ideally" sure.. but reality bites sometimes, and we have to make sure we do not
introduce a regression ;-)


> Why would this need to happen via an event on the socket transport thread?
> Couldn't it just acquire a lock and set the member vars?

what lock is that?  multithreading is not so trivial... most of the conn mgr
runs on the socket thread free of having to synchronize with locks (which is a
huge benefit in that it greatly reduces the complexity of the conn mgr).  the
main thread communicates with the conn mgr via events posted to the socket
thread for most everything... we have a monitor for the purpose of implementing
synchronous shutdown, but that's about the only reason for a monitor in the conn
mgr code.

Comment 18

[Darin Fisher]

Attachment: v2 patch

OK, here's the "better" patch I spoke of.  The only downside to this patch is
that it adds a bit more code, but I think the fact that it won't cause
connections to close when prefs change makes it the right choice ;-)

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 154922 [details] [diff] [review]
v2 patch

+    nsresult UpdateParam(PRUint16 name, PRUint16 value);

why not use a named enum for the name param?

thanks for making this patch, I like this much more.

Comment 20

[Darin Fisher]

Attachment: v2.1 patch

OK, with nsParamName...

Comment 21

[Boris Zbarsky [:bzbarsky]]

[on cancelling connections on pref changes]
> yes.  do you think that is unacceptable?

Yes.  Consider downloads....

Looking at the patch now.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154957 [details] [diff] [review]
v2.1 patch

sr=bzbarsky.  I like this a lot more.

Comment 23

[Darin Fisher]

fixed-on-trunk for 1.8 alpha 3

Comment 24

[Remy]

There's yet another question in the Mozilla Firefox Support forum about this
situation so I'd like to nominate this bug for blocking aviary1.0.  

Comment 25

[Michael Lefevre]

Remy: link? was that with a current branch build?  last time I tried with a
branch build, this worked for me.

Comment 26

[Remy]

(In reply to comment #25)
> Remy: link? was that with a current branch build?  last time I tried with a
> branch build, this worked for me.

Oops, sorry...
http://forums.mozillazine.org/viewtopic.php?t=114760

The problem with settings not being saved correctly was fixed in the FF branch
nightlies some time ago.  Darin's fix for the settings being internally locked
at their defaults currently isn't applied to the aviary branch though.  I've
been applying it to my own source tree for testing.

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 154957 [details] [diff] [review]
v2.1 patch

a=asa for branch checkin.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 154957 [details] [diff] [review]
v2.1 patch

a=asa for 1.7 checkin too

Comment 29

[Darin Fisher]

fixed-aviary1.0

Comment 30

[Darin Fisher]

fixed1.7.x

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 252903 has been marked as a duplicate of this bug. ***

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 264563 has been marked as a duplicate of this bug. ***