Add Windows utillity function to verify if a computer is part of a domain.

Add Windows utillity function to verify if a computer is part of a domain.

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246600

[pastarmovj, 2014-01-21 14:41:46]

Hi Carlos,

After verifying that this check returns reasonable results we would like to
start using in various places around Chrome so I thought it will be useful to
add it to the win_util file. WDYT?

Thanks,
Julian

[gab, 2014-01-21 15:04:53]

Thanks! Some run-by nits.

https://codereview.chromium.org/140553005/diff/1/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/140553005/diff/1/base/win/win_util.cc#newcode362
base/win/win_util.cc:362: if(NERR_Success != ::NetGetJoinInformation(NULL,
&domain, &join_status)) {
nit: remove {}

https://codereview.chromium.org/140553005/diff/1/base/win/win_util.cc#newcode362
base/win/win_util.cc:362: if(NERR_Success != ::NetGetJoinInformation(NULL,
&domain, &join_status)) {
nit: Put constant on RHS of !=

https://codereview.chromium.org/140553005/diff/1/base/win/win_util.cc#newcode367
base/win/win_util.cc:367: return join_status == NetSetupDomainName;
Would ::NetSetupDomainName compile? It would look less like a magical variable
(i.e., make it more obvious it's part of the API being used, not our code).

[pastarmovj, 2014-01-21 15:30:30]

Thanks for the drive-by! :)

https://codereview.chromium.org/140553005/diff/1/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/140553005/diff/1/base/win/win_util.cc#newcode362
base/win/win_util.cc:362: if(NERR_Success != ::NetGetJoinInformation(NULL,
&domain, &join_status)) {
On 2014/01/21 15:04:53, gab wrote:
> nit: Put constant on RHS of !=

Right. This used to be longer in the UMA version :). Done!

https://codereview.chromium.org/140553005/diff/1/base/win/win_util.cc#newcode367
base/win/win_util.cc:367: return join_status == NetSetupDomainName;
On 2014/01/21 15:04:53, gab wrote:
> Would ::NetSetupDomainName compile? It would look less like a magical variable
> (i.e., make it more obvious it's part of the API being used, not our code).

Done.

[cpu_(ooo_6.6-7.5), 2014-01-22 00:18:47]

is the dll we are calling delay loaded?

Are we going to call this at every startup?

[pastarmovj, 2014-01-22 08:48:03]

It is delay-loaded according to the dependency walker. The function I call is in
netapi32.dll.

At present the function will not be on the start-up path either. Whether it will
end up being called on every run is not certain but most probably yes.

[gab, 2014-01-22 14:04:47]

On 2014/01/22 08:48:03, pastarmovj wrote:
> It is delay-loaded according to the dependency walker. The function I call is
in
> netapi32.dll.
> 
> At present the function will not be on the start-up path either. Whether it
will
> end up being called on every run is not certain but most probably yes.

I will need this on the startup path, happy to discuss details offline if this
is a problem.

[cpu_(ooo_6.6-7.5), 2014-01-22 20:31:02]

code lgtm put please don't regress startup time. My guess is that this call is
going to do a sync ipc to csrss or another service.

[commit-bot: I haz the power, 2014-01-23 13:59:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/140553005/90001

[commit-bot: I haz the power, 2014-01-23 15:40:22]

Change committed as 246600

[gab, 2014-02-07 21:47:18]

On 2014/01/22 20:31:02, cpu wrote:
> code lgtm put please don't regress startup time. My guess is that this call is
> going to do a sync ipc to csrss or another service.

Follow-up here; this is now on the startup path (added by Julian in
crrev.com/248957).

I just timed this via TRACE_EVENT in a Release build on my Z620; the first call
to IsEnrolledToDomain takes 0.38ms and follow-up calls are ~0.16ms (there are 4
calls total on startup in my build -- should the caller re-use the result rather
than call this API multiple times?).

I also need to call this on startup, I would make this call later than the
current calls; I'm on another thread so caching from the original call doesn't
make sense here. My call also takes ~0.16ms.

So only the first call is a bit longer (initializing the DLL I guess?).

@cpu: if it looks sane on my machine any worries with performance on lower-end
machines?

[pastarmovj, 2014-02-07 22:18:12]

On 2014/02/07 21:47:18, gab wrote:
> On 2014/01/22 20:31:02, cpu wrote:
> > code lgtm put please don't regress startup time. My guess is that this call
is
> > going to do a sync ipc to csrss or another service.
> 
> Follow-up here; this is now on the startup path (added by Julian in
> crrev.com/248957).
> 
> I just timed this via TRACE_EVENT in a Release build on my Z620; the first
call
> to IsEnrolledToDomain takes 0.38ms and follow-up calls are ~0.16ms (there are
4
> calls total on startup in my build -- should the caller re-use the result
rather
> than call this API multiple times?).
> 
> I also need to call this on startup, I would make this call later than the
> current calls; I'm on another thread so caching from the original call doesn't
> make sense here. My call also takes ~0.16ms.
> 
> So only the first call is a bit longer (initializing the DLL I guess?).
> 
> @cpu: if it looks sane on my machine any worries with performance on lower-end
> machines?

There is a CL in review to cache the results after the first call. We were
discussing the need for thread safety but with these numbers maybe we don't need
to prematurely optimize?

[gab, 2014-02-07 23:05:15]

On 2014/02/07 22:18:12, pastarmovj wrote:
> On 2014/02/07 21:47:18, gab wrote:
> > On 2014/01/22 20:31:02, cpu wrote:
> > > code lgtm put please don't regress startup time. My guess is that this
call
> is
> > > going to do a sync ipc to csrss or another service.
> > 
> > Follow-up here; this is now on the startup path (added by Julian in
> > crrev.com/248957).
> > 
> > I just timed this via TRACE_EVENT in a Release build on my Z620; the first
> call
> > to IsEnrolledToDomain takes 0.38ms and follow-up calls are ~0.16ms (there
are
> 4
> > calls total on startup in my build -- should the caller re-use the result
> rather
> > than call this API multiple times?).
> > 
> > I also need to call this on startup, I would make this call later than the
> > current calls; I'm on another thread so caching from the original call
doesn't
> > make sense here. My call also takes ~0.16ms.
> > 
> > So only the first call is a bit longer (initializing the DLL I guess?).
> > 
> > @cpu: if it looks sane on my machine any worries with performance on
lower-end
> > machines?
> 
> There is a CL in review to cache the results after the first call. We were
> discussing the need for thread safety but with these numbers maybe we don't
need
> to prematurely optimize?

I still think it's worth caching the result on the caller side; making an API
call is still very slow (3.7 million CPU cycles as analyzed by SyzyProf for
these 4 calls... 1.7 million of which are in the first call -- whereas checking
a bool is 1 cycle!).

I don't think it should be cached as a static in IsEnrolledDomain() or anything,
then there are thread-safety concerns (i.e., I know for sure that I need this on
another thread than the one you're originally calling it on).

After discussing with cpu offline; since you merged to beta, you should keep an
eye on the beta stats for Startup.BrowserMessageLoopStartTime:
https://uma.googleplex.com/timeline/#metric=Startup.BrowserMessageLoopStartTi...

The next beta won't contain a big diff and if this spikes, we'll know it's
related to your change (i.e. it's very fast on my machine, but perhaps in some
configurations it's very slow -- we should look not to affect the 90'th or even
99'th percentile with this (especially since we don't get much stats from
enterprise configurations which are likely to be hit the hardest out of anybody
else by this...).

Cheers!
Gab