Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2048083003:
Use IsOS(OS_DOMAINMEMBER) to Check for Domain Membership (Closed)
|
Created:
4 years, 6 months ago by robliao Modified:
3 years, 7 months ago CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org, pastarmovj, Roger Tawa OOO till Jul 10th Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse IsOS(OS_DOMAINMEMBER) to Check for Domain Membership
The IsOS check and the NetGetJoinInformation check are equivalent.
BUG=579196
Committed: https://crrev.com/4d1fae2deb4a053c1e572e036b7a9b033136e945
Cr-Commit-Position: refs/heads/master@{#399019}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove Newly Unused netapi32.lib Dependency from base #
Total comments: 2
Patch Set 3 : Remove the '!!' #
Dependent Patchsets: Messages
Total messages: 49 (18 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048083003/1
robliao@chromium.org changed reviewers: + scottmg@chromium.org
scottmg: Please review this CL. Thanks!
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc File base/win/win_util.cc (right): https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc#newcod... base/win/win_util.cc:529: !!IsOS(OS_DOMAINMEMBER) ? is there a benefit to doing this? is there a gain in switching to a function in shlwapi.dll?
scottmg@chromium.org changed reviewers: - grt@chromium.org
Add bug=579196. There's some concern on the original CL about it likely being a sync call to csrss https://codereview.chromium.org/140553005 . Do you have any reason to think IsOS will be any different perfomance-wise?
https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc File base/win/win_util.cc (right): https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc#newcod... base/win/win_util.cc:529: !!IsOS(OS_DOMAINMEMBER) ? On 2016/06/09 16:51:41, grt (slow) wrote: > is there a benefit to doing this? is there a gain in switching to a function in > shlwapi.dll? This way, MS maintains the code necessary to check for domain membership. We don't need to maintain the same code on our side if anything changes.
Description was changed from ========== Use IsOS(OS_DOMAINMEMBER) to Check for Domain Membership The IsOS check and the NetGetJoinInformation check are equivalent. BUG= ========== to ========== Use IsOS(OS_DOMAINMEMBER) to Check for Domain Membership The IsOS check and the NetGetJoinInformation check are equivalent. BUG=579196 ==========
https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc File base/win/win_util.cc (left): https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc#oldcod... base/win/win_util.cc:530: if(::NetGetJoinInformation(NULL, &domain, &join_status) != NERR_Success) From a quick skim, it looks like we could remove netapi32.lib from the libs in base/BUILD.gn if this is removed.
On 2016/06/09 16:55:32, scottmg wrote: > Add bug=579196. > > There's some concern on the original CL about it likely being a sync call to > csrss https://codereview.chromium.org/140553005 . Do you have any reason to > think IsOS will be any different perfomance-wise? Nope, the performance characteristics will be same as IsOS performs the same NetGetJoinInformation check as we do. In fact, it also internally caches the result, but we can't take advantage of that since we need to be able to change the answer for tests. There's most definitely an RPC call involved since we're querying something about the network config.
Sounds like you two have this covered. I'll bow out. Thanks!
On 2016/06/09 17:18:25, grt (slow) wrote: > Sounds like you two have this covered. I'll bow out. Thanks! Not particularly. :) But it looks reasonable to me from reading docs. lgtm but please check if you can remove the lib from the build file.
+pastarmovj just fyi
On 2016/06/09 17:20:58, scottmg wrote: > On 2016/06/09 17:18:25, grt (slow) wrote: > > Sounds like you two have this covered. I'll bow out. Thanks! > > Not particularly. :) But it looks reasonable to me from reading docs. > > lgtm but please check if you can remove the lib from the build file. Yep, scanning to see if we can remove all instances of netapi32.lib.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048083003/40001
robliao@chromium.org changed reviewers: + thakis@chromium.org
thakis@chromium.org: Please review the netapi32.lib removal in base/BUILD.gn Thanks! https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc File base/win/win_util.cc (left): https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc#oldcod... base/win/win_util.cc:530: if(::NetGetJoinInformation(NULL, &domain, &join_status) != NERR_Success) On 2016/06/09 17:12:14, scottmg wrote: > From a quick skim, it looks like we could remove netapi32.lib from the libs in > base/BUILD.gn if this is removed. Yep. Removed 1/4 instances. The remaining 3... chrome/browser/BUILD.gn needs it for password_manager_util_win (NetUserGetInfo) components/policy/core/common/BUILD.gn is doing a domain check of its own and should probably start using the base version. That's a separate CL entirely. build/common.gypi: Consumed by the policy build (same as components/policy/core/common/BUILD.gn)
thanks, lgtm
https://codereview.chromium.org/2048083003/diff/40001/base/win/win_util.cc File base/win/win_util.cc (right): https://codereview.chromium.org/2048083003/diff/40001/base/win/win_util.cc#ne... base/win/win_util.cc:529: !!IsOS(OS_DOMAINMEMBER) ? the interwebs claim IsOS returns a BOOL -- why the !! ?
gn change lgtm though (needed in gyp too?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2048083003/diff/40001/base/win/win_util.cc File base/win/win_util.cc (right): https://codereview.chromium.org/2048083003/diff/40001/base/win/win_util.cc#ne... base/win/win_util.cc:529: !!IsOS(OS_DOMAINMEMBER) ? On 2016/06/09 19:15:10, Nico wrote: > the interwebs claim IsOS returns a BOOL -- why the !! ? Indeed. That was an artifact of me removing the caching work here and then putting it back, which meant it needed to be a bool. Otherwise, you will get C4800: 'BOOL': forcing value to bool 'true' or 'false' (performance warning) !! removed for the ? :. Thanks!
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048083003/80001
lgtm
On 2016/06/09 19:15:49, Nico wrote: > gn change lgtm though (needed in gyp too?) ...and for gyp, I'm separating it out when I unify that in the policy/components change. See https://codereview.chromium.org/2048083003/diff/1/base/win/win_util.cc#oldcod... for full details..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2048083003/#ps80001 (title: "Remove the '!!'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048083003/80001
Message was sent while issue was closed.
Description was changed from ========== Use IsOS(OS_DOMAINMEMBER) to Check for Domain Membership The IsOS check and the NetGetJoinInformation check are equivalent. BUG=579196 ========== to ========== Use IsOS(OS_DOMAINMEMBER) to Check for Domain Membership The IsOS check and the NetGetJoinInformation check are equivalent. BUG=579196 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Use IsOS(OS_DOMAINMEMBER) to Check for Domain Membership The IsOS check and the NetGetJoinInformation check are equivalent. BUG=579196 ========== to ========== Use IsOS(OS_DOMAINMEMBER) to Check for Domain Membership The IsOS check and the NetGetJoinInformation check are equivalent. BUG=579196 Committed: https://crrev.com/4d1fae2deb4a053c1e572e036b7a9b033136e945 Cr-Commit-Position: refs/heads/master@{#399019} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4d1fae2deb4a053c1e572e036b7a9b033136e945 Cr-Commit-Position: refs/heads/master@{#399019}
Message was sent while issue was closed.
On 2016/06/09 17:15:46, robliao wrote: > On 2016/06/09 16:55:32, scottmg wrote: > > Add bug=579196. > > > > There's some concern on the original CL about it likely being a sync call to > > csrss https://codereview.chromium.org/140553005 . Do you have any reason to > > think IsOS will be any different perfomance-wise? > > Nope, the performance characteristics will be same as IsOS performs the same > NetGetJoinInformation check as we do. In fact, it also internally caches the > result, but we can't take advantage of that since we need to be able to change > the answer for tests. > > There's most definitely an RPC call involved since we're querying something > about the network config. I just did a "wt" on the call to IsOS out of curiosity while trying to decide if setup.exe should use base::win::IsEnrolledToDomain or not. I see a lot of overhead delayloading wkscli before it calls NetGetJoinInformation. What is the benefit? Are we not better off loading wkscli at load time (when we're hitting the disk a ton anyway) rather than waiting until we're somewhere in ChromeBrowserMainPartsWin::PreCreateThreads?
Message was sent while issue was closed.
On 2017/05/02 21:02:57, grt (UTC plus 2) wrote: > On 2016/06/09 17:15:46, robliao wrote: > > On 2016/06/09 16:55:32, scottmg wrote: > > > Add bug=579196. > > > > > > There's some concern on the original CL about it likely being a sync call to > > > csrss https://codereview.chromium.org/140553005 . Do you have any reason to > > > think IsOS will be any different perfomance-wise? > > > > Nope, the performance characteristics will be same as IsOS performs the same > > NetGetJoinInformation check as we do. In fact, it also internally caches the > > result, but we can't take advantage of that since we need to be able to change > > the answer for tests. > > > > There's most definitely an RPC call involved since we're querying something > > about the network config. > > I just did a "wt" on the call to IsOS out of curiosity while trying to decide if > setup.exe should use base::win::IsEnrolledToDomain or not. I see a lot of > overhead delayloading wkscli before it calls NetGetJoinInformation. What is the > benefit? Are we not better off loading wkscli at load time (when we're hitting > the disk a ton anyway) rather than waiting until we're somewhere in > ChromeBrowserMainPartsWin::PreCreateThreads? The main benefit here is not having to maintain the domain check code. Arguably we could also remove the caching logic as IsOS already does that. As for the delay load, it's tricky to say if load time vs PreCreateThreads is better. We're either rearranging the work (same time required at load or now), not actually doing a lot of work (explorer needs shlwapi and shcore explorer has likely made a similar call, so wkscli should be available as shared pages), or actually being the first load, meaning we pay the price here.
Message was sent while issue was closed.
On 2017/05/03 14:39:46, robliao wrote: > On 2017/05/02 21:02:57, grt (UTC plus 2) wrote: > > On 2016/06/09 17:15:46, robliao wrote: > > > On 2016/06/09 16:55:32, scottmg wrote: > > > > Add bug=579196. > > > > > > > > There's some concern on the original CL about it likely being a sync call > to > > > > csrss https://codereview.chromium.org/140553005 . Do you have any reason > to > > > > think IsOS will be any different perfomance-wise? > > > > > > Nope, the performance characteristics will be same as IsOS performs the same > > > NetGetJoinInformation check as we do. In fact, it also internally caches the > > > result, but we can't take advantage of that since we need to be able to > change > > > the answer for tests. > > > > > > There's most definitely an RPC call involved since we're querying something > > > about the network config. > > > > I just did a "wt" on the call to IsOS out of curiosity while trying to decide > if > > setup.exe should use base::win::IsEnrolledToDomain or not. I see a lot of > > overhead delayloading wkscli before it calls NetGetJoinInformation. What is > the > > benefit? Are we not better off loading wkscli at load time (when we're hitting > > the disk a ton anyway) rather than waiting until we're somewhere in > > ChromeBrowserMainPartsWin::PreCreateThreads? > > The main benefit here is not having to maintain the domain check code. I'm not sure it's providing that benefit based on https://crrev.com/452484. FWIW, I'm going to use NetGetJoinInformation directly in setup.exe since I don't want the shell anywhere near the installer (suffered the crashes, wearing the blood-stained tee-shirt right now :-) ).
Message was sent while issue was closed.
On 2017/05/05 20:33:11, grt (UTC plus 2) wrote: > On 2017/05/03 14:39:46, robliao wrote: > > On 2017/05/02 21:02:57, grt (UTC plus 2) wrote: > > > On 2016/06/09 17:15:46, robliao wrote: > > > > On 2016/06/09 16:55:32, scottmg wrote: > > > > > Add bug=579196. > > > > > > > > > > There's some concern on the original CL about it likely being a sync > call > > to > > > > > csrss https://codereview.chromium.org/140553005 . Do you have any reason > > to > > > > > think IsOS will be any different perfomance-wise? > > > > > > > > Nope, the performance characteristics will be same as IsOS performs the > same > > > > NetGetJoinInformation check as we do. In fact, it also internally caches > the > > > > result, but we can't take advantage of that since we need to be able to > > change > > > > the answer for tests. > > > > > > > > There's most definitely an RPC call involved since we're querying > something > > > > about the network config. > > > > > > I just did a "wt" on the call to IsOS out of curiosity while trying to > decide > > if > > > setup.exe should use base::win::IsEnrolledToDomain or not. I see a lot of > > > overhead delayloading wkscli before it calls NetGetJoinInformation. What is > > the > > > benefit? Are we not better off loading wkscli at load time (when we're > hitting > > > the disk a ton anyway) rather than waiting until we're somewhere in > > > ChromeBrowserMainPartsWin::PreCreateThreads? > > > > The main benefit here is not having to maintain the domain check code. > > I'm not sure it's providing that benefit based on https://crrev.com/452484. > FWIW, I'm going to use NetGetJoinInformation directly in setup.exe since I don't > want the shell anywhere near the installer (suffered the crashes, wearing the > blood-stained tee-shirt right now :-) ). Fair enough. Mobile Device Management is a very different notion and I believe orthogonal to domain join.
Message was sent while issue was closed.
pastarmovj@chromium.org changed reviewers: + pastarmovj@chromium.org
Message was sent while issue was closed.
+Roger who recently did thorough analysis of the various ways to check AD vs MDM joining. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
