Adapt upstream patches fixing potential deadlock in tcmalloc after fork.

Adapt upstream patches fixing potential deadlock in tcmalloc after fork.

This applies changes originally done in https://github.com/gperftools/gperftools/issues/499,
which fix a bug causing Chromium deadlock at startup on Linux about once per
100 launches. This is due to lock in tcmalloc after fork() which is done
in some libraries which Chromium depends on. For example, dbus contains this code:

https://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-unix.c#n4243

Here opendir() calls malloc() inside while running between fork() and exec(),
leading to rare deadlock. Almost the same code is in glib:

https://github.com/GNOME/glib/blob/de04fd13048dd208162573187e4c0d9e7d3428d3/glib/gspawn.c#L1041

BUG=
R=wfh@chromium.org,jar@chromium.org

[jar (doing other things), 2016-02-19 23:50:28]

I'd suggest adding a reference to bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=570068

As I noted in that bug, this has some potential for inducing more deadlocks than
it resolves... so YMMV.

As suggested in the bug, I'd argue for re-architecting the way that Chrome
fork()s, to ensure that it *only* forks when single threaded.  To do this, we
may need to create a single process (via fork) early on, and *that* process
would probably do all future forks (rather than using the main process).

Your fix may be more expedient... (though carry risks)... so I'll let Will make
a call.

[Tom Jakubowski, 2016-02-20 00:50:16]

On 2016/02/19 23:50:28, jar (doing other things) wrote:
> I'd suggest adding a reference to bug:
> https://bugs.chromium.org/p/chromium/issues/detail?id=570068
> 
> As I noted in that bug, this has some potential for inducing more deadlocks
than
> it resolves... so YMMV.
> 
> As suggested in the bug, I'd argue for re-architecting the way that Chrome
> fork()s, to ensure that it *only* forks when single threaded.  To do this, we
> may need to create a single process (via fork) early on, and *that* process
> would probably do all future forks (rather than using the main process).
> 

Doesn't the zygote process mostly fit this description? There is at least one
exception, though: the fork that was consistently causing us issues as 
embedders is the fork of the main process to adjust the OOM score of the
renderer process on Linux. See:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/zy...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/zy...

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/zy...

[jar (doing other things), 2016-02-20 02:30:35]

I'm not familiar with the Zygote process....

...but if it is running single threaded, then there is no chance (no need?)
of TCMalloc lock being held during the fork.... and the problem vanishes
(with no risk of unexpected locking state in a child).

You have to do some plumbing to let this process create children as
needed... that are controllable from main (which used to fork directly)....
but that all seems (in my mind) to be a bunch of code and considerations.

Jim

On Fri, Feb 19, 2016 at 4:50 PM, <tjakubowski@oblong.com> wrote:

> On 2016/02/19 23:50:28, jar (doing other things) wrote:
> > I'd suggest adding a reference to bug:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=570068
> >
> > As I noted in that bug, this has some potential for inducing more deadlocks
> than
> > it resolves... so YMMV.
> >
> > As suggested in the bug, I'd argue for re-architecting the way that Chrome
> > fork()s, to ensure that it *only* forks when single threaded.  To do this,
we
> > may need to create a single process (via fork) early on, and *that* process
> > would probably do all future forks (rather than using the main process).
> >
>
> Doesn't the zygote process mostly fit this description? There is at least one
> exception, though: the fork that was consistently causing us issues as
> embedders is the fork of the main process to adjust the OOM score of the
> renderer process on Linux. See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/zy...
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/zy...
> https://codereview.chromium.org/1715903002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Will Harris, 2016-02-20 02:36:41]

Description was changed from

==========
Adapt upstream patches fixing potential deadlock in tcmalloc after fork.

This applies changes originally done in
https://github.com/gperftools/gperftools/issues/499,
which fix a bug causing Chromium deadlock at startup on Linux about once per
100 launches. This is due to lock in tcmalloc after fork() which is done
in some libraries which Chromium depends on. For example, dbus contains this
code:

https://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-unix.c#n4243

Here opendir() calls malloc() inside while running between fork() and exec(),
leading to rare deadlock. Almost the same code is in glib:

https://github.com/GNOME/glib/blob/de04fd13048dd208162573187e4c0d9e7d3428d3/g...

BUG=
R=wfh@chromium.org,jar@chromium.org
==========

to

==========
Adapt upstream patches fixing potential deadlock in tcmalloc after fork.

This applies changes originally done in
https://github.com/gperftools/gperftools/issues/499,
which fix a bug causing Chromium deadlock at startup on Linux about once per
100 launches. This is due to lock in tcmalloc after fork() which is done
in some libraries which Chromium depends on. For example, dbus contains this
code:

https://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-unix.c#n4243

Here opendir() calls malloc() inside while running between fork() and exec(),
leading to rare deadlock. Almost the same code is in glib:

https://github.com/GNOME/glib/blob/de04fd13048dd208162573187e4c0d9e7d3428d3/g...

BUG=
R=wfh@chromium.org,jar@chromium.org
==========

[Will Harris, 2016-02-20 02:37:50]

On 2016/02/20 02:30:35, jar (doing other things) wrote:
> I'm not familiar with the Zygote process....
> 
> ...but if it is running single threaded, then there is no chance (no need?)
> of TCMalloc lock being held during the fork.... and the problem vanishes
> (with no risk of unexpected locking state in a child).
> 
> You have to do some plumbing to let this process create children as
> needed... that are controllable from main (which used to fork directly)....
> but that all seems (in my mind) to be a bunch of code and considerations.
> 
> Jim
> 
> On Fri, Feb 19, 2016 at 4:50 PM, <mailto:tjakubowski@oblong.com> wrote:
> 
> > On 2016/02/19 23:50:28, jar (doing other things) wrote:
> > > I'd suggest adding a reference to bug:
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=570068
> > >
> > > As I noted in that bug, this has some potential for inducing more
deadlocks
> > than
> > > it resolves... so YMMV.
> > >
> > > As suggested in the bug, I'd argue for re-architecting the way that Chrome
> > > fork()s, to ensure that it *only* forks when single threaded.  To do this,
> we
> > > may need to create a single process (via fork) early on, and *that*
process
> > > would probably do all future forks (rather than using the main process).
> > >
> >
> > Doesn't the zygote process mostly fit this description? There is at least
one
> > exception, though: the fork that was consistently causing us issues as
> > embedders is the fork of the main process to adjust the OOM score of the
> > renderer process on Linux. See:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/zy...
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/zy...
> > https://codereview.chromium.org/1715903002/
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Rickyz has a similar CL here - 

https://codereview.chromium.org/1523403003/

I think his is a bit less invasive - uzer can you take a look and see if that
cleanly applies and fixes your issue?