886903 - Send correct UserAgent for content process

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: v1

We have to send more AppInfo to the content process. (This bug is probably not in the right component)

Comment 1

[Josh Matthews [:jdm]]

What effect does this have?

Comment 2

[Tom S. (please needinfo tschuster)]

It sends the correct UserAgent header for http request. Without this pages like addons.mozilla.org are broken, because they don't detect Firefox.

Comment 3

[(not currently active) Ted Mielczarek]

How much are we remoting HTTP? Couldn't we just be building the headers in the parent process instead?

Comment 4

[Benjamin Smedberg]

Comment on attachment 767329 [details] [diff] [review]
v1

Does this bug affect B2G, or is this somehow specific to the new desktop-e10s work?

That said, I don't know how ContentChild::GetSingleton works any more or whether there are cases it can be null which would cause crashes in this code; so I'm going to defer this to somebody who hopefully does know.

Comment 5

[Tom S. (please needinfo tschuster)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 767329 [details] [diff] [review]
> v1
> 
> Does this bug affect B2G, or is this somehow specific to the new
> desktop-e10s work?
> 
I am not sure, but I am pretty sure somebody would have noticed a bad UserAgent by now.
> That said, I don't know how ContentChild::GetSingleton works any more or
> whether there are cases it can be null which would cause crashes in this
> code; so I'm going to defer this to somebody who hopefully does know.
There are other cases of code that look exactly like mine and just return an other property, so I doubt that this is a problem.

Comment 6

[Josh Matthews [:jdm]]

To be clear, what's the UA before and after this patch?

Comment 7

[Tom S. (please needinfo tschuster)]

I am going to try and find out what the difference is. On the first look the UA is the same, but addons.mozilla.org definitely reacts to this change.

Comment 8

[Josh Matthews [:jdm]]

Don't forget to consider HTTP headers and properties of navigator.

Comment 9

[Tom S. (please needinfo tschuster)]

You are right the culprit was in Navigator. So what happens is (on amo):
This code https://addons.cdn.mozilla.net/media/js/preload-min.js?build=ecf37e2-51db415f:15 ends up calling NS_GetNavigatorUserAgent, which calls into GetUserAgent of the HTTP handler. There is a small difference between what we return with and without this patch. With it's: "Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20130710 Firefox/25.0", without however we are missing Firefox :( "Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20130710 /25.0a1".

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 767329 [details] [diff] [review]
v1

This patch seems fine to me.

Comment 11

[Tom S. (please needinfo tschuster)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0ded4854067b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Something in this push was causing Linux debug mochitest-other to be perma-crashing. I couldn't find you on IRC, so I had to back the entire push out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/998c273658b8

https://tbpl.mozilla.org/php/getParsedLog.php?id=25380532&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25380601&tree=Mozilla-Inbound

Comment 13

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86cba0042a68

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86cba0042a68