1361601 - Remove system-info.getProperty("host")

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Patrick McManus [:mcmanus]]

PR_GetSystemInfo(PR_SI_HOSTNAME, ..) turns out to be quite expensive on windows because it silently initializes winsock.

nsSystemInfo::Init() does this to add the hostname to the propertyBag under the "host" key.

There doesn't seem to be anything in the codebase that accesses that property - so we should remove it. It also seems like a per user tracker waiting to accidentally happen somewhere.

This is a try run that indicates nothing accesses getProperty("host").. nothing came up in an addon search either though its a hard thing to search for.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=394e8b31c3490fd118dd8f3ef12033975c0809a4

https://bugzilla.mozilla.org/show_bug.cgi?id=1361087 shows the original performance problem.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: Bug 1361601 - Remove nsSystemInfo.getProperty("host")

See also bug 1361495 - PR_SI_HOSTNAME is implemented in NSPR on
Windows as initializing winsock which can be janky. There don't seem
to be any users of this property, and it has tracker concerns anyhow -
so remove it.

Review commit: https://reviewboard.mozilla.org/r/135690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135690/

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8863961 [details]
Bug 1361601 - Remove nsSystemInfo.getProperty("host")

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135690/diff/1-2/

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8863961 [details]
Bug 1361601 - Remove nsSystemInfo.getProperty("host")

https://reviewboard.mozilla.org/r/135690/#review138820

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/040f581bbbd9
Remove nsSystemInfo.getProperty("host") r=froydnj

Comment 5

[Dragana Damjanovic [:dragana]]

I remember there was a bug some time a go, where we were stack on main thread doing dns. 
As I remember we needed that for something. I am not sure if this was the place that was making problems and removing this will cause other problems.

I think bsmedberg was commenting in that bug. I will need info him: Do I remember correctly, can you check if it is fine to remove this (if  you know)? Thanks.

Comment 6

[Patrick McManus [:mcmanus]]

there was (is?) a sync dns  resolution interface that we had some plugin users of that caused fits.. maybe that's what you're thinking of? It wouldn't be relevant here if that's it..

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> there was (is?) a sync dns  resolution interface that we had some plugin
> users of that caused fits.. maybe that's what you're thinking of? It
> wouldn't be relevant here if that's it..

I think it was something else, it had to do with a profile, but maybe I am wrong.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/040f581bbbd9

Comment 9

[Benjamin Smedberg]

ISTR a bug, but I'm pretty certain it wasn't this API.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to Patrick McManus [:mcmanus] from comment #0)
> There doesn't seem to be anything in the codebase that accesses that
> property

Sync does (bug 1369285) and http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/devtools/shared/discovery/discovery.js#179 also does. It looks like there should be a bug on file for that?

Comment 11

[Patrick McManus [:mcmanus]]

probably a question for devtools