Cache IsNonBrowserProcess values, so we only take a loader-lock the first time.

Cache IsNonBrowserProcess values, so we only take a loader-lock the first time.

BUG=485656, 477137
Committed: https://crrev.com/78d41b5d4e59dc7d52d7e7d6530fdd0a78b36d33
Cr-Commit-Position: refs/heads/master@{#329246}

[Cait (Slow), 2015-05-07 21:58:24]

caitkp@chromium.org changed reviewers:
+ siggi@chromium.org

[grt (UTC plus 2), 2015-05-08 04:44:34]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2015-05-08 04:44:34]

drive-by

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util.cc
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util....
chrome_elf/chrome_elf_util.cc:205: is_sandboxed_process =
if this is true, there's no need to check the command line, is there? how about
only keeping one piece of state rather than two and short-circuiting? for
example:
  static enum {
    UNINITIALIZED,
    NON_BROWSER_PROCESS,
    BROWSER_PROCESS,
  } result = UNINITIALIZED;
  while (result == UNINITIALIZED) {
    ... GetProcAddress
    if (is_sandboxed_process_func && is_sandboxed_process_func()) {
      result = NON_BROWSER_PROCESS;
      break;
    }
    const wchar_t* command_line = GetCommandLine();
    if (command_line && wcsstr(command_line, L"--type") {
      result = NON_BROWSER_PROCESS;
      break;
    }
    result = BROWSER_PROCESS;
    break;
  }
  return result == NON_BROWSER_PROCESS;

[Sigurður Ásgeirsson, 2015-05-08 14:36:33]

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util.cc
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util....
chrome_elf/chrome_elf_util.cc:200: if (!initialized) {
This mode of initialization is not thread safe. It can return bogus results on
concurrent invocations.

[grt (UTC plus 2), 2015-05-08 14:44:22]

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util.cc
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util....
chrome_elf/chrome_elf_util.cc:200: if (!initialized) {
On 2015/05/08 14:36:33, Sigurður Ásgeirsson wrote:
> This mode of initialization is not thread safe. It can return bogus results on
> concurrent invocations.

In practice, I think that the first call will be during a single-threaded state.
Is there a way to assert this without adding a lock here? Otherwise, do you
suggest using a leaky LazyInstance?

[Sigurður Ásgeirsson, 2015-05-08 16:59:43]

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util.cc
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/1/chrome_elf/chrome_elf_util....
chrome_elf/chrome_elf_util.cc:200: if (!initialized) {
On 2015/05/08 14:44:22, grt wrote:
> On 2015/05/08 14:36:33, Sigurður Ásgeirsson wrote:
> > This mode of initialization is not thread safe. It can return bogus results
on
> > concurrent invocations.
> 
> In practice, I think that the first call will be during a single-threaded
state.
> Is there a way to assert this without adding a lock here? Otherwise, do you
> suggest using a leaky LazyInstance?

If you can guarantee that this function is invoked at some particular time,
there's no need for locking, but that seems brittle to maintenance?

Maybe it's better to simply compute the value at some particular time, store it
in a "global" and return it from this function? This function can then simply
assert on this value having been initialized?

[Cait (Slow), 2015-05-11 20:17:43]

caitkp@chromium.org changed reviewers:
+ csharp@chromium.org

[Cait (Slow), 2015-05-11 20:17:44]

Siggi, grt -- PTAL

Chris: Can you take a look at the change to chrome_elf/blacklist/test/?
(IsNonBrowserProcess gets called from within the test dll, so we have to
initialize it, otherwise we hit the assert()).

[csharp, 2015-05-11 20:31:32]

One small naming suggestion, but the change to the blacklist test seems fine to
me

https://codereview.chromium.org/1132473003/diff/40001/chrome_elf/chrome_elf_u...
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/40001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util.cc:13: ProcessType g_is_non_browser_process =
ProcessType::UNINITIALIZED;
As discussed, maybe flip it to become g_is_browser_process

[Sigurður Ásgeirsson, 2015-05-11 20:37:41]

lgtm with but a nit.

https://codereview.chromium.org/1132473003/diff/40001/chrome_elf/chrome_elf_u...
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/40001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util.cc:13: ProcessType g_is_non_browser_process =
ProcessType::UNINITIALIZED;
g_process_type, perhaps?

[Cait (Slow), 2015-05-11 21:16:21]

thanks, all!

https://codereview.chromium.org/1132473003/diff/40001/chrome_elf/chrome_elf_u...
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/40001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util.cc:13: ProcessType g_is_non_browser_process =
ProcessType::UNINITIALIZED;
On 2015/05/11 20:37:41, Sigurður Ásgeirsson wrote:
> g_process_type, perhaps?

Done.

https://codereview.chromium.org/1132473003/diff/40001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util.cc:13: ProcessType g_is_non_browser_process =
ProcessType::UNINITIALIZED;
On 2015/05/11 20:31:32, csharp wrote:
> As discussed, maybe flip it to become g_is_browser_process

Acknowledged.

[Cait (Slow), 2015-05-11 21:17:41]

The CQ bit was checked by caitkp@chromium.org

[Cait (Slow), 2015-05-11 21:17:42]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/1132473003/#ps60001
(title: "Rename variable")

[commit-bot: I haz the power, 2015-05-11 21:20:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132473003/60001

[commit-bot: I haz the power, 2015-05-11 22:00:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-11 22:02:09]

Patchset 4 (id:??) landed as
https://crrev.com/78d41b5d4e59dc7d52d7e7d6530fdd0a78b36d33
Cr-Commit-Position: refs/heads/master@{#329246}

[grt (UTC plus 2), 2015-05-12 15:17:37]

https://codereview.chromium.org/1132473003/diff/60001/chrome_elf/chrome_elf_u...
File chrome_elf/chrome_elf_util.cc (right):

https://codereview.chromium.org/1132473003/diff/60001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util.cc:13: ProcessType g_process_type =
ProcessType::UNINITIALIZED;
i'm surprised this compiles. you're using the syntax for an "enum class" here,
but the type itself is an old-school enum...

https://codereview.chromium.org/1132473003/diff/60001/chrome_elf/chrome_elf_u...
File chrome_elf/chrome_elf_util.h (right):

https://codereview.chromium.org/1132473003/diff/60001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util.h:10: enum ProcessType {
nit: "enum class ProcessType"

https://codereview.chromium.org/1132473003/diff/60001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util.h:41: extern ProcessType g_process_type;
does this need to be public? if so, it should have a doc comment.

[Cait (Slow), 2015-05-12 19:55:33]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1134253002/ by caitkp@chromium.org.

The reason for reverting is: Possibly causing mysterious fails on the Win DBG
bots:
https://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds...

http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28....

[Zhenyao Mo, 2015-05-12 22:18:54]

This CL definitely break the GPU win debug bots (I bisected and is 100% sure).

To reproduce, build chrome in debug, then:

python content/test/gpu/run_gpu_test.py maps --browser=debug --show-stdout

The browser won't launch due to an uncaught exception.