56753 - user agent misc field can't be changed after handler is initialized

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

If you change the pref for the user agent misc field after the HTTP handler has
been initialized the HTTP handler doesn't recognize that and continues to use
the old misc field.  This probably doesn't sound like a serious problem at first
but the problem is that the HTTP handler is initialzed very early during
startup.  In the case of the embedding api, through some very convouted
dependencies, it's actually initialized before the return from
NS_InitEmbedding().  This means that embedders have no chance to set the misc
field of the HTTP handler.

I'll attach a simple patch that fixes this.  I don't think that any of the other
fields of the user agent string should change after initialization.

Comments?

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 2

[Gagan]

r=gagan on blizzards patch.

Comment 3

[tao]

I did a similar fix for the locale info in u-a string in 55156.

Comment 4

[Scott MacGregor]

sr=mscott

Comment 5

[Christopher Blizzard (:blizzard)]

Checked in.  Thanks, guys!

Comment 6

[Ken]

Chris- version 1.135 causes the assertion on line 1288 on nsHTTPHandler.cpp to 
fire. Basically, your call to BuildUserAgent(), line ~1475, is getting executed 
before the User Agent Components are intialized (via InitUserAgentComponents())

Comment 7

[Christopher Blizzard (:blizzard)]

The fix here seems to be that we need to initialize the user agent components
before we call PrefsChanged().  I looked at the code paths in the file and it
looks like this should work fine.  It's also interesting to note that the locale
prefs change causes this problem, too and I didn't write that code. :)  Also,
PrefsChanged() needs an arg - I added a NULL arg since that's what looks like it
needed.

Uploading patch in a sec.

Comment 8

[Christopher Blizzard (:blizzard)]

Attachment: patch 2

Comment 9

[Christopher Blizzard (:blizzard)]

gagan, does this look OK to you?

Comment 10

[tao]

Blizzard's patch also fixes the same assertion mentioned in bug 56505

Comment 11

[Christopher Blizzard (:blizzard)]

Tao, does the patch look OK to you?

Comment 12

[tao]

Looks good. Two minor points:
1. The original code calls PrefsChanged() without arg. Most compilers interpret
   it
   as NULL. You proposal of calling it w/ NULL (how about nsnull?) sounds
   fine to me.
2. Should we register callbacks after PrefsChanged() ?

You'd need Gagan's blessing on this patch since he knows the code best.

Comment 13

[Gagan]

doh... I see the difference now... and this patch addresses the problem of
PrefsChanged being called AFTER the init of prefs. Which seems fine to me. And
blizzard you don't need a NULL (or correctly a nsnull) since PrefsChanged has a
default parameter. r=gagan.

Comment 14

[Gagan]

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

Comment 15

[Christopher Blizzard (:blizzard)]

I'll check it in without the NULL in the arg.  I didn't see the default param
for that function.  Thanks!

Comment 16

[Scott MacGregor]

I put my super review in 56505 which is a dup of this one by mistake a couple
days ago. Sorry about that. repeating the sr=mscott here

Comment 17

[Christopher Blizzard (:blizzard)]

Checked in.  Thanks!