46488 - layout SetUserAgent initializes http

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Warren Harris]

Whenever layout gets initialized, the first thing it does is call SetUserAgent. 
This loads up http and sets the product name to "gecko" and the version to a 
date string. This has the unfortunate consequence of initializing http which 
calls GetLocalizedUnicharPref (see bug 46487) which calls string bundle-> chrome 
-> script security manager -> necko -> http (recursively re-entering it). 

Jud and I discussed this tonight, and we think it would be better if gecko just 
stored its name string ("gecko") and version/date in the prefs file. Then when 
http starts up (presumably when the first http page is loaded), it just reads it 
out of prefs. (I verified that removing the call to SetUserAgent does allow http 
to be deferred until needed by an http web page.)

The tricky part here is getting the date string into the prefs file. This will 
require some makefile creativity. I suggested making the make script compute the 
date and append it to prefs.js after it copies it to dist. The one gotcha here 
is that on unix this is a symlink. Maybe we can just copy this file instead of 
symlink it.

Comment 1

[Alec Flett]

there are other places to store such a string, I'm wondering if prefs is the 
right place. prefs are kind of a per-profile type of thing, seems like something 
like this might be better off in a string bundle...

Comment 2

[Judson Valeski]

I used to have all the UA components pulled from a string bundle, then moved it 
to prefs for some reason :-/.

Comment 3

[tao]

The getLocalizedUNicharPref() is called to retrieve the locale info only.
Since this string is not supposed to be modified by end users, it makes
sense to pull it via stringBundle.

Comment 4

[Alec Flett]

wait, I should clarify tao's remark - the localized unichar prefs are meant to 
be overridden if necessary. One example is the default homepage - for the 
english distribution of mozilla, we distribute http://home.netscape.com/, but 
for other languages we want a different, locale-specific homepage. In either 
case, the user can override it.

Comment 5

[tao]

Thanks to Alec for clarifying this up.

My prevous comment refers to the usage of getLocalizedXXX() in the HTTP code 
only.

Comment 6

[Judson Valeski]

I'm going to fix this to read from prefs. I'm not going to whack the build
scripts to modify the prefs.js file. who should own that?

Comment 7

[Alec Flett]

whacking the prefs file really seems like a bad idea.
Why is this in prefs? that's what I still don't understand.

Comment 8

[Judson Valeski]

I'm pretty sure we ditched the string bundle idea because it was bringing in a
nasty dependency.

Comment 9

[tao]

What's the implication of this change? 

  1. the locale part of the U-A string is not localizable or
  2. it is localizable but not switchable

Comment 10

[Mitchell Stoltz (not reading bugmail)]

It can be both localizable and switchable, it just shouldn't live in layout.

Comment 11

[Warren Harris]

The reason we were going to make it a pref was because all the other parts of 
the user agent string come from prefs. However, a string bundle might be a 
better way to go. How about putting it in 
netwerk/resources/locale/en-US/necko.properties? We'd still have to whack the 
build system to insert the version number though. That shouldn't be so hard.

Comment 12

[tao]

Yes, getting it from property file directory makes it both 
localizable and switchable. 

Comment 13

[Phil Peterson]

PDT would like to see some explanation in this bug report about why it's a P1.
What bad experience does the user have? Or is this just code cleanup?

Comment 14

[Michael La Guardia]

marking nsbeta3- as we will not hold PR3 for this.

Comment 15

[Judson Valeski]

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

Comment 16

[Patrick C. Beard]

This also causes a leak in the service manager, due to the recursive request for 
the HTTP handler.

Comment 17

[Alec Flett]

taking from valeski - I have a related bug, I have a pretty good idea of what to
do.

Comment 18

[Alec Flett]

I've got a potential fix... I'm basically making an object in layout that is an
http startup listener (thanks shaver) - when it gets called, it sets these two
strings.

Comment 19

[Alec Flett]

removing nsbeta3 keywords

Comment 20

[Judson Valeski]

do you have a patch ready for review; hit me?

Comment 21

[Alec Flett]

Attachment: use the observer API when creating HTTP startup listeners

Comment 22

[Alec Flett]

Attachment: register an http startup listener, set up user agent

Comment 23

[Alec Flett]

Ok, here's what's going on:
- in order for the startup listener to access the http protocol handler, I had
to make the startup listener routine call Observe() and pass in "this"
- in the observer, I set up the user agent.

looking for a reviewer/super reviewer from valeski/waterson.

Comment 24

[Judson Valeski]

at first blush "this" as aSubject seemed wierd. Should aSubject really be 
"aContext" to imply XPCOM's equivelent of void*?

r=valeski

Comment 25

[Chris Waterson]

Looks good. Fix these, and sr=waterson:

- In nsHTTPHandler.h, follow parenthesizing conventions. (You add spaces.)

- In nsHTTPHandler.cpp, use NS_STATIC_CAST to get where you want. Using 
  those C-style casts (especially from void*) is going to screw you some
  day.

- In the code you moved from nsLayoutModule to nsLayoutHTTPStartup.cpp,
  review the string-fu in `Observe()'. You have some unused variables, and
  you should be able to use NS_LITERAL_STRING() instead of
  NS_ConvertUTF8toUCS2().

- #ifdef DEBUG_alecf your noisy printfs().

- Get someone to buddy you on the Mac.

Comment 26

[Alec Flett]

- spacing conventions were just from moving existing bad spacing - but fixed.
- static cast stuff - I get ambiguous-casting errors if I don't use void*. I
switched to nested NS_STATIC_CAST anyway
- you can't use NS_LITERAL_STRING on #define'd strings.. so I'm fixing up the
#define's themselves (including the perl scripts that define the strings in the
first place)
- oops, didn't mean to leave in those printf's!

new patch forthcoming

Comment 27

[Alec Flett]

Attachment: updated patch

Comment 28

[Chris Waterson]

Don't use `void*'. Cast to one of the base classes first; e.g.

  NS_STATIC_CAST(nsISupports*, NS_STATIC_CAST(nsIHTTPHandler*, this))

Comment 29

[Alec Flett]

ooohh.. thanks :)
done in my tree.

Comment 30

[tao]

Hi, Alec:


In the proposad patch, I didn't see any refernence to the retrieval part of
locale info. Am I missing something?

thx

Comment 31

[Alec Flett]

this was never designed to set the locale - this is the bit where gecko set's
the product name and version. I can only assume someone else sets the locale.

Comment 32

[tao]

I see. Therefore, necko still has dependency on strres -> chrome ->...
I was confused with another bug..

Comment 33

[Alec Flett]

moving "fix in hand" bugs to moz 0.9

Comment 34

[Paul Chen]

Marking nsbeta1+ to put it on the radar

Comment 35

[Paul Chen]

Mass moving most of mozilla0.9 bugs to mozilla0.8.1

Comment 36

[Alec Flett]

Attachment: further updated patch to fix mac issues (thanks pchen)

Comment 37

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Alec, I think you should move the HTTP startup listener into content/build
because that is where we create the gbdate.h file. I am going to disable
exporting gbdate.h rsn.

Comment 38

[Alec Flett]

actually, it's created in BOTH locations - are you disabling the one in
layout/build?

Comment 39

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Heh, right... I have a patch ready to land that will remove gbdate stuff from
layout completely, and will stop exporting gbdate.h in content.

Comment 40

[Alec Flett]

hekki - do you know when you'll be removing the extra gbdate.h? Because I would
really like to wait for that before I move everything over once and for all...

Comment 41

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Landed yesterday, gbdate.pl|h now only in content/build.

Comment 42

[Alec Flett]

Attachment: new fix - for content/build

Comment 43

[Alec Flett]

ok and now for something completely different.
I've moved the patch over to content\build - I'll need a mac person to add
nsContentHTTPStartup to the .mcp file. 

As for that gbdate.pl / gbdate.h dependancy... heikki? why is that only on if
the file doesn't already exist? because once I check this in, all depend builds
are going to break until we manually delete that file. 

Comment 44

[Heikki Toivonen (remove -bugzilla when emailing directly)]

On the Mac, every time you build, gbdate.h is generated again. On Windows and
Linux it is only generated if it does not exist. The situation has not changed
after my checkin.

As to why it is so, I believe it is a matter of opinion. In my opinion the Mac
way is correct, we should generate it every time you compile (it is build date,
after all). jst wanted to keep it as is. I would not object if you changed it to
be generated every compile on Windows and Unix as well. On Unix it would require
some serious makefile hackery I guess...

I am thinking some people see gbdate as the *pull date*. Some even do depend
builds normally, and clobber only after they have pulled, so by looking at
gbdate they see when they pulled. But this is wrong, and obviously fragile.

Comment 45

[Paul Chen]

Just applied latest patch on my powerbook, looks good. r=pchen if you need it

Comment 46

[Paul Chen]

Tree is closed for 0.8.1, since I don't consider this fix critical for 0.8.1,
moving to 0.9

Comment 47

[Alec Flett]

yay, fix finally in

Comment 48

[Chris Petersen]

Marking verified per last comments.