ipc: Use a global for the process's attachment broker.

ipc: Use a global for the process's attachment broker.

This eliminates the need for a lot of plumbing, at the expense of yet another
global.

BUG=493414
Committed: https://crrev.com/5708aae6ec926bddd69f3e9058f129d10a0d1873
Cr-Commit-Position: refs/heads/master@{#348649}

[erikchen, 2015-08-14 01:08:50]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-14 01:09:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/20001

[commit-bot: I haz the power, 2015-08-14 01:29:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-14 01:29:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2015-08-14 18:23:42]

Patchset #3 (id:40001) has been deleted

[erikchen, 2015-08-25 21:01:53]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-25 21:03:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/20001

[commit-bot: I haz the power, 2015-08-25 21:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-25 21:07:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2015-08-26 00:31:39]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-26 00:32:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/60001

[commit-bot: I haz the power, 2015-08-26 01:24:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 01:24:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2015-08-27 00:08:20]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 00:08:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/100001

[erikchen, 2015-08-27 00:23:24]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-08-27 00:23:38]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-08-27 00:24:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/100001

[commit-bot: I haz the power, 2015-08-27 00:24:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 00:24:19]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[erikchen, 2015-08-27 00:33:13]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 00:33:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/120001

[commit-bot: I haz the power, 2015-08-27 01:56:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 01:56:11]

Dry run: This issue passed the CQ dry run.

[erikchen, 2015-08-28 00:35:34]

erikchen@chromium.org changed reviewers:
+ tsepez@chromium.org

[erikchen, 2015-08-28 00:35:35]

tsepez: Please review.

[Tom Sepez, 2015-08-31 16:59:24]

Doesn't seem like you need to do the thread-safety yourself:

// The LazyInstance<Type, Traits> class manages a single instance of Type,
// which will be lazily created on the first time it's accessed.  This class is
// useful for places you would normally use a function-level static, but you
// need to have guaranteed thread-safety.  The Type constructor will only ever
// be called once, even if two threads are racing to create the object.  Get()
// and Pointer() will always return the same, completely initialized instance.
// When the instance is constructed it is registered with AtExitManager.  The
// destructor will be called on program exit.
//

[erikchen, 2015-08-31 17:36:40]

On 2015/08/31 16:59:24, Tom Sepez wrote:
> Doesn't seem like you need to do the thread-safety yourself:
> 
> // The LazyInstance<Type, Traits> class manages a single instance of Type,
> // which will be lazily created on the first time it's accessed.  This class
is
> // useful for places you would normally use a function-level static, but you
> // need to have guaranteed thread-safety.  The Type constructor will only ever
> // be called once, even if two threads are racing to create the object.  Get()
> // and Pointer() will always return the same, completely initialized instance.
> // When the instance is constructed it is registered with AtExitManager.  The
> // destructor will be called on program exit.
> //

The lock is used to ensure that IPC::AttachmentBroker::SetGlobal() is called
exactly once.

[erikchen, 2015-09-10 01:01:08]

On 2015/08/31 17:36:40, erikchen wrote:
> On 2015/08/31 16:59:24, Tom Sepez wrote:
> > Doesn't seem like you need to do the thread-safety yourself:
> > 
> > // The LazyInstance<Type, Traits> class manages a single instance of Type,
> > // which will be lazily created on the first time it's accessed.  This class
> is
> > // useful for places you would normally use a function-level static, but you
> > // need to have guaranteed thread-safety.  The Type constructor will only
ever
> > // be called once, even if two threads are racing to create the object. 
Get()
> > // and Pointer() will always return the same, completely initialized
instance.
> > // When the instance is constructed it is registered with AtExitManager. 
The
> > // destructor will be called on program exit.
> > //
> 
> The lock is used to ensure that IPC::AttachmentBroker::SetGlobal() is called
> exactly once.

tsepez: ping

[Tom Sepez, 2015-09-11 15:58:30]

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker.h
File ipc/attachment_broker.h (right):

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker....
ipc/attachment_broker.h:54: // for ensuring that |broker| stays alive for the
life time of the process.
nit: lifetime

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker_...
File ipc/attachment_broker_privileged_win_unittest.cc (right):

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker_...
ipc/attachment_broker_privileged_win_unittest.cc:428:
IPC::AttachmentBroker::SetGlobal(&broker);
This goes out of scope while the process is still alive, seemingly contradicting
the previous comments. Now the process is well on its way to exiting when the
code returns ...

[Tom Sepez, 2015-09-11 16:04:07]

>
https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker_...
> ipc/attachment_broker_privileged_win_unittest.cc:428:
> IPC::AttachmentBroker::SetGlobal(&broker);
> This goes out of scope while the process is still alive, seemingly
contradicting
> the previous comments. Now the process is well on its way to exiting when the
> code returns ...

LGTM once you make the comment consistent, though.

[erikchen, 2015-09-11 20:16:02]

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker.h
File ipc/attachment_broker.h (right):

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker....
ipc/attachment_broker.h:54: // for ensuring that |broker| stays alive for the
life time of the process.
On 2015/09/11 15:58:30, Tom Sepez wrote:
> nit: lifetime

I updated the comment to say:
"""
Each process has at most one attachment broker. The process is responsible
for ensuring that |broker| stays alive for as long as the process is
sending/receiving ipc messages.
"""

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker_...
File ipc/attachment_broker_privileged_win_unittest.cc (right):

https://codereview.chromium.org/1292263003/diff/120001/ipc/attachment_broker_...
ipc/attachment_broker_privileged_win_unittest.cc:428:
IPC::AttachmentBroker::SetGlobal(&broker);
On 2015/09/11 15:58:30, Tom Sepez wrote:
> This goes out of scope while the process is still alive, seemingly
contradicting
> the previous comments. Now the process is well on its way to exiting when the
> code returns ...

On 2015/09/11 15:58:30, Tom Sepez wrote:
> nit: lifetime

I updated the comment to say:
"""
Each process has at most one attachment broker. The process is responsible
for ensuring that |broker| stays alive for as long as the process is
sending/receiving ipc messages.
"""

[erikchen, 2015-09-11 20:16:54]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-11 20:17:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/160001

[commit-bot: I haz the power, 2015-09-11 20:34:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 20:34:50]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[erikchen, 2015-09-11 21:08:10]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-11 21:08:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/180001

[commit-bot: I haz the power, 2015-09-11 21:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 21:46:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-09-11 21:54:26]

erikchen@chromium.org changed reviewers:
+ avi@chromium.org

[erikchen, 2015-09-11 21:54:26]

avi: Looking for a content/ review.

[erikchen, 2015-09-11 22:30:52]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-09-11 22:30:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1292263003/#ps180001
(title: "Rebase errors.")

[commit-bot: I haz the power, 2015-09-11 22:31:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/180001

[commit-bot: I haz the power, 2015-09-11 22:43:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 22:43:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Avi (use Gerrit), 2015-09-11 23:24:40]

https://codereview.chromium.org/1292263003/diff/180001/content/common/child_p...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/1292263003/diff/180001/content/common/child_p...
content/common/child_process_host_impl.cc:58: return &g_attachment_broker.Get();
What? This is literally a double-checked lock, and it's an antipattern.

https://en.wikipedia.org/wiki/Double-checked_locking

If you need a singleton (base::Singleton) then use one; don't build one
yourself.

[erikchen, 2015-09-12 01:06:31]

avi: PTAL

https://codereview.chromium.org/1292263003/diff/180001/content/common/child_p...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/1292263003/diff/180001/content/common/child_p...
content/common/child_process_host_impl.cc:58: return &g_attachment_broker.Get();
On 2015/09/11 23:24:40, Avi wrote:
> What? This is literally a double-checked lock, and it's an antipattern.
> 
> https://en.wikipedia.org/wiki/Double-checked_locking
> 
> If you need a singleton (base::Singleton) then use one; don't build one
> yourself.

What I need is a cross-platform version of pthread_once, but I didn't see any in
the Chrome source code. I changed the code to perform the relevant logic in the
constructor of a base::LazyInstance. What do you think?

[Avi (use Gerrit), 2015-09-12 02:39:18]

On 2015/09/12 01:06:31, erikchen wrote:
> avi: PTAL
> 
>
https://codereview.chromium.org/1292263003/diff/180001/content/common/child_p...
> File content/common/child_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1292263003/diff/180001/content/common/child_p...
> content/common/child_process_host_impl.cc:58: return
&g_attachment_broker.Get();
> On 2015/09/11 23:24:40, Avi wrote:
> > What? This is literally a double-checked lock, and it's an antipattern.
> > 
> > https://en.wikipedia.org/wiki/Double-checked_locking
> > 
> > If you need a singleton (base::Singleton) then use one; don't build one
> > yourself.
> 
> What I need is a cross-platform version of pthread_once, but I didn't see any
in
> the Chrome source code. I changed the code to perform the relevant logic in
the
> constructor of a base::LazyInstance. What do you think?

That's a reasonable workaround. LGTM

[erikchen, 2015-09-12 17:17:19]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-09-12 17:17:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1292263003/#ps220001
(title: "Comments from avi.")

[commit-bot: I haz the power, 2015-09-12 17:17:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/220001

[commit-bot: I haz the power, 2015-09-12 20:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-12 20:19:28]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-09-13 17:50:12]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-09-13 17:50:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/220001

[commit-bot: I haz the power, 2015-09-13 23:51:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-13 23:51:48]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2015-09-14 17:11:37]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-09-14 17:13:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263003/220001

[commit-bot: I haz the power, 2015-09-14 17:45:23]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-09-14 17:46:11]

Patchset 11 (id:??) landed as
https://crrev.com/5708aae6ec926bddd69f3e9058f129d10a0d1873
Cr-Commit-Position: refs/heads/master@{#348649}

[commit-bot: I haz the power, 2015-09-23 12:33:56]

Patchset 11 (id:??) landed as
https://crrev.com/5708aae6ec926bddd69f3e9058f129d10a0d1873
Cr-Commit-Position: refs/heads/master@{#348649}