Making LazyInstance Leaky by default

[Kentaro Hara]

Hi

TL;DR: I'm planning to make base::LazyInstance Leaky by default. Please let me know if there's any concern about it.

[Details]

I'm now trying to remove a shutdown sequence (=RenderThreadImpl::Shutdown) from the renderer process. RenderThreadImpl::Shutdown has caused a lot of use-after-free bugs and many engineers have spent lots of time fixing complicated ordering issues around the shutdown. As discussed in blink-dev and platform-architecture-dev, there is no reason we have to shut down the renderer gracefully. It's just causing use-after-free bugs and wasting performance.

At first, I tried to simply remove RenderThreadImpl::Shutdown but it broke a lot of things. If we simply remove RenderThreadImpl::Shutdown, we end up with destructing RenderThreadImpl and MessageLoop without stopping worker threads. Then the worker threads crash when they post a task to an already-destructed MessageLoop.

Next, I tried to forcibly call exit(0) at the beginning of RenderThreadImpl::Shutdown(). However, this didn't work well in a single-process mode and browser tests because it forcibly exits the process before the browser side is ready to exit.

So I'm now trying yet another approach: Don't call RenderThreadImpl::Shutdown() and leak RenderThreadImpl. If we go this way, we need to leak all LazyInstances as well (otherwise we hit an error like this). With that change, I confirmed that the approach passes all tests.

As far as I understand, there would be no benefit in destructing LazyInstances when OnExit is called. So I think it would be okay to leak LazyInstances, but if I'm missing something, please let me know.

Thanks!

--

Kentaro Hara, Tokyo, Japan

[Alexei Svitkine]

Is your plan to change both browser process and child process semantics?

How do you know we don't have LazyInstances in browser process that rely on doing something useful in their destructor? Have you done any analysis/audit of this?

(FWIW, I like this direction but it's not obvious to me how you'd make the change and ensure you don't break something subtle.)

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Kentaro Hara]

On Wed, Oct 26, 2016 at 4:43 PM, Alexei Svitkine <asvi...@chromium.org> wrote:

Is your plan to change both browser process and child process semantics?

Yes. Specifically, I'm planning to remove the OnExit handler from LazyInstance.

At first I tried to add "::Leaky" to only LazyInstances that hit the DCHECK when removing RenderThreadImpl::Shutdown, but I've learned that it's not realistic because there're many LazyInstances that hit the DCHECK :/ So I'm proposing making LazyInstances leaky by default.

 

How do you know we don't have LazyInstances in browser process that rely on doing something useful in their destructor? Have you done any analysis/audit of this?

Yeah, this is a good point. If there is any LazyInstance that is doing some meaningful thing in its destructor, the proposed change will cause a problem. It's hard to check >1200 LazyInstances in Chromium though.

If you're concerned about the side effect, here is a more complicated but safer option:

- In a multi-process mode, just call exit(0) at the beginning of RenderThreadImpl::Shutdown().

- In a single-process mode (including browser tests), leak RenderThreadImpl and all LazyInstances. This would not be problematic because it just happens in a single-process mode.

[Alexei Svitkine]

(I was actually going to suggest that approach too - it does sound safer to me, but happy to hear others' opinions.)

[Nico Weber]

I think this is great.

On Wed, Oct 26, 2016 at 11:47 AM, 'Kentaro Hara' via Chromium-dev <chromi...@chromium.org> wrote:

On Wed, Oct 26, 2016 at 4:43 PM, Alexei Svitkine <asvi...@chromium.org> wrote:

Is your plan to change both browser process and child process semantics?

Yes. Specifically, I'm planning to remove the OnExit handler from LazyInstance.

At first I tried to add "::Leaky" to only LazyInstances that hit the DCHECK when removing RenderThreadImpl::Shutdown, but I've learned that it's not realistic because there're many LazyInstances that hit the DCHECK :/ So I'm proposing making LazyInstances leaky by default.

 

How do you know we don't have LazyInstances in browser process that rely on doing something useful in their destructor? Have you done any analysis/audit of this?

Yeah, this is a good point. If there is any LazyInstance that is doing some meaningful thing in its destructor, the proposed change will cause a problem. It's hard to check >1200 LazyInstances in Chromium though.

How did you come up with 1200? I can find under 276 non-leaky LazyInstances: https://cs.chromium.org/search/?q=LazyInstance%3C%5B%5E%3E%5D*%3E%5Cs&sq=package:chromium&type=cs That seems auditable.

[John Abd-El-Malek]

On Wed, Oct 26, 2016 at 8:47 AM, Kentaro Hara <har...@google.com> wrote:

On Wed, Oct 26, 2016 at 4:43 PM, Alexei Svitkine <asvi...@chromium.org> wrote:

Is your plan to change both browser process and child process semantics?

Yes. Specifically, I'm planning to remove the OnExit handler from LazyInstance.

At first I tried to add "::Leaky" to only LazyInstances that hit the DCHECK when removing RenderThreadImpl::Shutdown, but I've learned that it's not realistic because there're many LazyInstances that hit the DCHECK :/ So I'm proposing making LazyInstances leaky by default.

 

How do you know we don't have LazyInstances in browser process that rely on doing something useful in their destructor? Have you done any analysis/audit of this?

Yeah, this is a good point. If there is any LazyInstance that is doing some meaningful thing in its destructor, the proposed change will cause a problem. It's hard to check >1200 LazyInstances in Chromium though.

This is what worries me, i.e. unintended consequences.

The renderer side change is great; if we can do below that seems like it would have less side effects.

 

If you're concerned about the side effect, here is a more complicated but safer option:

- In a multi-process mode, just call exit(0) at the beginning of RenderThreadImpl::Shutdown().

- In a single-process mode (including browser tests), leak RenderThreadImpl and all LazyInstances. This would not be problematic because it just happens in a single-process mode.

THere are only a very small number of browser tests that run in single process mode. We can try writing them in other ways. 

[Kentaro Hara]

Nico: 

How did you come up with 1200? I can find under 276 non-leaky LazyInstances: https://cs.chromium.org/search/?q=LazyInstance%3C%5B%5E%3E%5D*%3E%5Cs&sq=package:chromium&type=cs That seems auditable.

Ah, your query looks correct. Yeah, that seems auditable.

On Wed, Oct 26, 2016 at 8:02 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, Oct 26, 2016 at 8:47 AM, Kentaro Hara <har...@google.com> wrote:

On Wed, Oct 26, 2016 at 4:43 PM, Alexei Svitkine <asvi...@chromium.org> wrote:

Is your plan to change both browser process and child process semantics?

Yes. Specifically, I'm planning to remove the OnExit handler from LazyInstance.

At first I tried to add "::Leaky" to only LazyInstances that hit the DCHECK when removing RenderThreadImpl::Shutdown, but I've learned that it's not realistic because there're many LazyInstances that hit the DCHECK :/ So I'm proposing making LazyInstances leaky by default.

 

How do you know we don't have LazyInstances in browser process that rely on doing something useful in their destructor? Have you done any analysis/audit of this?

Yeah, this is a good point. If there is any LazyInstance that is doing some meaningful thing in its destructor, the proposed change will cause a problem. It's hard to check >1200 LazyInstances in Chromium though.

This is what worries me, i.e. unintended consequences.

The renderer side change is great; if we can do below that seems like it would have less side effects.

 

If you're concerned about the side effect, here is a more complicated but safer option:

- In a multi-process mode, just call exit(0) at the beginning of RenderThreadImpl::Shutdown().

- In a single-process mode (including browser tests), leak RenderThreadImpl and all LazyInstances. This would not be problematic because it just happens in a single-process mode.

THere are only a very small number of browser tests that run in single process mode. We can try writing them in other ways. 

However, we still need to support a single-process mode so we anyway need to implement the leak logic, right?

Then do you think we should pass in an IsSingleProcessMode flag to LazyInstance's constructor? I'm a bit worried that the process mode is not yet available when some LazyInstances are instantiated.

 

 

(FWIW, I like this direction but it's not obvious to me how you'd make the change and ensure you don't break something subtle.)

On Wed, Oct 26, 2016 at 6:34 AM, Kentaro Hara <har...@chromium.org> wrote:

Hi

TL;DR: I'm planning to make base::LazyInstance Leaky by default. Please let me know if there's any concern about it.

[Details]

I'm now trying to remove a shutdown sequence (=RenderThreadImpl::Shutdown) from the renderer process. RenderThreadImpl::Shutdown has caused a lot of use-after-free bugs and many engineers have spent lots of time fixing complicated ordering issues around the shutdown. As discussed in blink-dev and platform-architecture-dev, there is no reason we have to shut down the renderer gracefully. It's just causing use-after-free bugs and wasting performance.

At first, I tried to simply remove RenderThreadImpl::Shutdown but it broke a lot of things. If we simply remove RenderThreadImpl::Shutdown, we end up with destructing RenderThreadImpl and MessageLoop without stopping worker threads. Then the worker threads crash when they post a task to an already-destructed MessageLoop.

Next, I tried to forcibly call exit(0) at the beginning of RenderThreadImpl::Shutdown(). However, this didn't work well in a single-process mode and browser tests because it forcibly exits the process before the browser side is ready to exit.

So I'm now trying yet another approach: Don't call RenderThreadImpl::Shutdown() and leak RenderThreadImpl. If we go this way, we need to leak all LazyInstances as well (otherwise we hit an error like this). With that change, I confirmed that the approach passes all tests.

As far as I understand, there would be no benefit in destructing LazyInstances when OnExit is called. So I think it would be okay to leak LazyInstances, but if I'm missing something, please let me know.

Thanks!

--

Kentaro Hara, Tokyo, Japan

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--

Kentaro Hara, Tokyo, Japan

[John Abd-El-Malek]

On Wed, Oct 26, 2016 at 11:20 AM, Kentaro Hara <har...@google.com> wrote:

Nico: 

How did you come up with 1200? I can find under 276 non-leaky LazyInstances: https://cs.chromium.org/search/?q=LazyInstance%3C%5B%5E%3E%5D*%3E%5Cs&sq=package:chromium&type=cs That seems auditable.

Ah, your query looks correct. Yeah, that seems auditable.

On Wed, Oct 26, 2016 at 8:02 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, Oct 26, 2016 at 8:47 AM, Kentaro Hara <har...@google.com> wrote:

On Wed, Oct 26, 2016 at 4:43 PM, Alexei Svitkine <asvi...@chromium.org> wrote:

Is your plan to change both browser process and child process semantics?

Yes. Specifically, I'm planning to remove the OnExit handler from LazyInstance.

At first I tried to add "::Leaky" to only LazyInstances that hit the DCHECK when removing RenderThreadImpl::Shutdown, but I've learned that it's not realistic because there're many LazyInstances that hit the DCHECK :/ So I'm proposing making LazyInstances leaky by default.

 

How do you know we don't have LazyInstances in browser process that rely on doing something useful in their destructor? Have you done any analysis/audit of this?

Yeah, this is a good point. If there is any LazyInstance that is doing some meaningful thing in its destructor, the proposed change will cause a problem. It's hard to check >1200 LazyInstances in Chromium though.

This is what worries me, i.e. unintended consequences.

The renderer side change is great; if we can do below that seems like it would have less side effects.

 

If you're concerned about the side effect, here is a more complicated but safer option:

- In a multi-process mode, just call exit(0) at the beginning of RenderThreadImpl::Shutdown().

- In a single-process mode (including browser tests), leak RenderThreadImpl and all LazyInstances. This would not be problematic because it just happens in a single-process mode.

THere are only a very small number of browser tests that run in single process mode. We can try writing them in other ways. 

However, we still need to support a single-process mode

The only mode of single-process we support is when it never shuts down (i.e. for android webview). Are you thinking of others?

[Gabriel Charette]

I prefer early termination (exit(0)) to making everything leaky.

I've been thinking a lot about the browser side equivalence of this and I think we want to keep a clear ownership graph that allows us to wind things down when we want to to make it possible for things like LSAN, unit tests, and single-process mode to still work properly.

Deciding to terminate early in the product is a very different decision and paradigm then intentionally dropping ownership semantics and leaking everything.

[Kentaro Hara]

Thanks all!

So the conclusion is:

1) Rewrite a couple of single-process-mode tests to multi-process-mode tests.

2) Simply call exit(0) at the beginning of RenderThreadImpl::Shutdown().

Then we won't need to any change to LazyInstance. Does it sound good?

[Torne (Richard Coles)]

On Thu, 27 Oct 2016 at 10:40 Kentaro Hara <har...@google.com> wrote:

Thanks all!

So the conclusion is:

1) Rewrite a couple of single-process-mode tests to multi-process-mode tests.

If that's possible then this seems reasonable.

 

2) Simply call exit(0) at the beginning of RenderThreadImpl::Shutdown().

You want to call _exit(), or just SIGKILL the current process; exit() implies various shutdown work on POSIX.

[Gabriel Charette]

On Thu, Oct 27, 2016 at 5:43 AM Torne (Richard Coles) <to...@chromium.org> wrote:

On Thu, 27 Oct 2016 at 10:40 Kentaro Hara <har...@google.com> wrote:

Thanks all!

So the conclusion is:

1) Rewrite a couple of single-process-mode tests to multi-process-mode tests.

If that's possible then this seems reasonable.

 

2) Simply call exit(0) at the beginning of RenderThreadImpl::Shutdown().

You want to call _exit(), or just SIGKILL the current process; exit() implies various shutdown work on POSIX.

Actually, you'll just want to use base::Process::Terminate() to be cross-platform (and if that does the wrong thing on POSIX it should be fixed in process_posix.cc as it's used in similar scenarios elsewhere).

[Torne (Richard Coles)]

base::Process::Terminate sends SIGTERM, waits, and then sends SIGKILL, so that should be fine :)

[Nico Weber]

Why do you prefer that? LSAN, unit tests, single-process mode all don't require cleanup of globals.

And "leaking everything" at process end isn't a leak.

[Bernhard Bauer]

On Thu, Oct 27, 2016 at 5:32 PM Nico Weber <tha...@chromium.org> wrote:

Why do you prefer that? LSAN, unit tests, single-process mode all don't require cleanup of globals.

(I would be very happy if unit tests did clean up their singletons during teardown, as carrying state over into the next test can break things in very annoying ways. I tried doing that once, but it turned out to break other things; see https://crbug.com/522692.)

 

Bernhard.

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

[Primiano Tucci]

> I would be very happy if unit tests did clean up their singletons during teardown, as carrying state over into the next test can break things in very annoying ways. I tried doing that once, but it turned out to break other things; seehttps://crbug.com/522692.

I think that is going to create even more problems. Either you cleanup singletons *and* all global variables (which is tough) or you will end up with a lot of inconsistent state. I am thinking at all the cases where you have some g_ptr which points to some member owned by the singleton, or g_flag which refers to some state where the singleton is supposed to be in.

So, yes, I agree with you it's annoying. But I don't think it's easily solvable in practice.

[Nico Weber]

And also, the vast majority of LazyInstances is already Leaky, which suggests that this is a better default. So I think swapping the default and auditing the non-leaky lazy instances sounds like a great change.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Marshall Greenblatt]

On Thu, Oct 27, 2016 at 12:59 PM, Nico Weber <tha...@chromium.org> wrote:

And also, the vast majority of LazyInstances is already Leaky, which suggests that this is a better default. So I think swapping the default and auditing the non-leaky lazy instances sounds like a great change.

It would be great if we could also replace Singleton<> usage with leaky LazyInstances. Is that on anyone's radar currently?

[Gabriel Charette]

@Nico: yes sorry I extended my thoughts beyond globals. I agree that we should leak globals (as I've mentioned multiple times already on previous threads). I think this is okay for existing globals/singletons. What I don't want to see happening is a paradigm shift where everyone makes their thing a LazyInstance to avoid thinking about memory management, globals should ideally only be for stateless things (i.e. whose state can't affect independent tests). Other things should be part of a clear ownership graph (even if in practice the process is eventually always terminated before they're actually deleted).

I also overall prefer early process termination as everything dying at once makes things so much simpler (at least in the browser's use case which is the one on my mind).

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Kentaro Hara]

So, to sum up, we want to make all of the following changes, right?

a) Rewrite single-process-mode tests with multi-process-mode tests

b) Call base::Process::Terminate() at the beginning of RenderThreadImpl::Shutdown

c) Make LazyInstances Leaky

To remove the renderer's shutdown sequence (which is my original goal), we need "a) and b)" or "c) and making RenderThreadImpl leaky". If we go with "a) and b)", we of course don't need to make RenderThreadImpl leaky.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Will Harris]

Just a few thoughts - but do any of these changes have the potential to play havoc with our sanitiser bots e.g. valgrind, drmemory, or asan leak detectors? How will they handle the base::Process::Terminate()? Does this change the process exit code profile in any way? Are we planning to monitor these changes in UMA or crash in any way?

Will

[Kentaro Hara]

On Thu, Oct 27, 2016 at 11:28 PM, Will Harris <w...@chromium.org> wrote:

Just a few thoughts - but do any of these changes have the potential to play havoc with our sanitiser bots e.g. valgrind, drmemory, or asan leak detectors? How will they handle the base::Process::Terminate()?

Good point.

When is valgrind, dmemory and LSan running? Is it before RenderThreadImpl::Shutdown? If it's running after RenderThreadImpl::Shutdown, calling exit(0) will just make those sanitizers useless (which should not be acceptable).

[Dirk Pranke]

On Fri, Oct 28, 2016 at 6:40 AM, 'Kentaro Hara' via Chromium-dev <chromi...@chromium.org> wrote:

On Thu, Oct 27, 2016 at 11:28 PM, Will Harris <w...@chromium.org> wrote:

Just a few thoughts - but do any of these changes have the potential to play havoc with our sanitiser bots e.g. valgrind, drmemory, or asan leak detectors? How will they handle the base::Process::Terminate()?

Good point.

When is valgrind, dmemory and LSan running? Is it before RenderThreadImpl::Shutdown? If it's running after RenderThreadImpl::Shutdown, calling exit(0) will just make those sanitizers useless (which should not be acceptable).

valgrind and drmemory aren't running at all anymore :).

-- Dirk

[Lei Zhang]

Just LSAN now. Look for __lsan_do_leak_check().

[Will Harris]

So just LSAN then? :) +inferno might know?

Will

On Fri, Oct 28, 2016 at 11:39 AM, Dirk Pranke <dpr...@chromium.org> wrote:

[Kentaro Hara]

Is there any way to run LSan in a meaningful manner even when we call exit(0) at RenderThreadImpl::Shutdown()?

If the answer is no, we'll need to make LazyInstances and RenderThreadImpl leaky at least when LSan is enabled. (We can call exit(0) when LSan is disabled.)

[Inferno]

+cc Kostya for question on LSan.

[Kentaro Hara]

The question is:

Will LSan work as expected even when we cal exit(0) at the beginning of RenderThreadImpl::Shutdown()?

If it just works, I'm very happy :)

On Tue, Nov 1, 2016 at 4:37 AM, Kostya Serebryany <k...@google.com> wrote:

Can you rephrase the questions? 

What's wrong with lsan+exit(0)? 

[Primiano Tucci]

On Tue, Nov 1, 2016 at 4:46 AM Kostya Serebryany <k...@google.com> wrote:

On Mon, Oct 31, 2016 at 7:38 PM, Kentaro Hara <har...@google.com> wrote:

The question is:

Will LSan work as expected even when we cal exit(0) at the beginning of RenderThreadImpl::Shutdown()?

There are two things here:

1) IIRC the plan here is eventually to call _exit(0) not exit(0) (discussion here) 

I think LSan depends on the atexit hook to do its final analysis, which _exit(0) will kill entirely. Unless there is extra magic that hooks LSan differently in our test harness.

2) LazyInstance::Leaky has the right ANNOTATE_SCOPED_MEMORY_LEAK which tells LSan to not alert on a given object.

I think the question here is: does it also:

A) apply recursively to all the objects owned by that?

Or

B) does the exclusion apply only to the owned objects that have been created in the ctor, but not owned later?

I think B.

In more practical words, if you have:

class RenderThreadImpl {

  RenderThreadImpl() x(new Something) {}

  void InitializeLaterY() { y.reset(new SomethingElse); }

  unique_ptr<..> x;

  unique_ptr<..> y;

}

And you make a LazyInstance<RenderThreadImpl>::Leaky

And later at some point you just exit(0) without destroying.

I think LSan will not complain about x but will complain about y.

If this is the case, I think this means that if you make RenderThreadImpl ::Leaky, all the smart pointers that have been initialized *after* the RTI ctor will be detected as leaks if you abruptly exit before tearing down RTI.

Yes, it should. 

It's easy to verify -- introduce an intentional leak and see what happens:

  static volatile int *leaky_var;  // global

...

  leaky_var = new int;

  leaky_var = nullptr; // previous value leaked 

[Kentaro Hara]

On Tue, Nov 1, 2016 at 8:14 PM, Primiano Tucci <prim...@chromium.org> wrote:

On Tue, Nov 1, 2016 at 4:46 AM Kostya Serebryany <k...@google.com> wrote:

On Mon, Oct 31, 2016 at 7:38 PM, Kentaro Hara <har...@google.com> wrote:

The question is:

Will LSan work as expected even when we cal exit(0) at the beginning of RenderThreadImpl::Shutdown()?

There are two things here:

1) IIRC the plan here is eventually to call _exit(0) not exit(0) (discussion here) 

I think LSan depends on the atexit hook to do its final analysis, which _exit(0) will kill entirely. Unless there is extra magic that hooks LSan differently in our test harness.

2) LazyInstance::Leaky has the right ANNOTATE_SCOPED_MEMORY_LEAK which tells LSan to not alert on a given object.

I think the question here is: does it also:

A) apply recursively to all the objects owned by that?

Or

B) does the exclusion apply only to the owned objects that have been created in the ctor, but not owned later?

I think B.

In more practical words, if you have:

class RenderThreadImpl {

  RenderThreadImpl() x(new Something) {}

  void InitializeLaterY() { y.reset(new SomethingElse); }

  unique_ptr<..> x;

  unique_ptr<..> y;

}

And you make a LazyInstance<RenderThreadImpl>::Leaky

And later at some point you just exit(0) without destroying.

I think LSan will not complain about x but will complain about y.

If this is the case, I think this means that if you make RenderThreadImpl ::Leaky, all the smart pointers that have been initialized *after* the RTI ctor will be detected as leaks if you abruptly exit before tearing down RTI.

IIUC, the answer is A. For example, Oilpan depends on A.

[Kentaro Hara]

On Tue, Nov 1, 2016 at 8:20 PM, Kentaro Hara <har...@google.com> wrote:

On Tue, Nov 1, 2016 at 8:14 PM, Primiano Tucci <prim...@chromium.org> wrote:

On Tue, Nov 1, 2016 at 4:46 AM Kostya Serebryany <k...@google.com> wrote:

On Mon, Oct 31, 2016 at 7:38 PM, Kentaro Hara <har...@google.com> wrote:

The question is:

Will LSan work as expected even when we cal exit(0) at the beginning of RenderThreadImpl::Shutdown()?

There are two things here:

1) IIRC the plan here is eventually to call _exit(0) not exit(0) (discussion here) 

I think LSan depends on the atexit hook to do its final analysis, which _exit(0) will kill entirely. Unless there is extra magic that hooks LSan differently in our test harness.

I noticed one more thing.

If we need to call _exit(0) (by calling base::Process::Terminate), it means not only that LSan doesn't run but also that LeakyInstances are not destructed. Doesn't it mean that we'll anyway need to make all LeakyInstances (at least all LeakyInstances on the renderer process) leaky?

If my understanding is correct, we'll need to make the following changes:

1) Make all LeakyInstances leaky.

2) Move the LSan verification to the beginning of RenderThreadImpl::Shutdown.

3) Call _exit(0) after the LSan verification.

Here is another question: Do we really need to call _exit(0) instead of exit(0)? If we can call exit(0), we can just the current LSan verification and keep LeakyInstances non-leaky.

[Primiano Tucci]

On Wed, Nov 2, 2016 at 2:16 AM Kentaro Hara <har...@google.com> wrote:

On Tue, Nov 1, 2016 at 8:20 PM, Kentaro Hara <har...@google.com> wrote:

On Tue, Nov 1, 2016 at 8:14 PM, Primiano Tucci <prim...@chromium.org> wrote:

On Tue, Nov 1, 2016 at 4:46 AM Kostya Serebryany <k...@google.com> wrote:

On Mon, Oct 31, 2016 at 7:38 PM, Kentaro Hara <har...@google.com> wrote:

The question is:

Will LSan work as expected even when we cal exit(0) at the beginning of RenderThreadImpl::Shutdown()?

There are two things here:

1) IIRC the plan here is eventually to call _exit(0) not exit(0) (discussion here) 

I think LSan depends on the atexit hook to do its final analysis, which _exit(0) will kill entirely. Unless there is extra magic that hooks LSan differently in our test harness.

I noticed one more thing.

If we need to call _exit(0) (by calling base::Process::Terminate), it means not only that LSan doesn't run but also that LeakyInstances are not destructed. Doesn't it mean that we'll anyway need to make all LeakyInstances (at least all LeakyInstances on the renderer process) leaky?

Well calling _exit means that all the LazyInstance-s will be de-facto leaky.

At this point it might be conceptually cleaner to make them Leaky to reflect what's really happening.

Let me wrap the question around: what is the benefit of triggering the AtExit handlers? Why those non-laky LazyInstance are such? 

If my understanding is correct, we'll need to make the following changes:

1) Make all LeakyInstances leaky.

2) Move the LSan verification to the beginning of RenderThreadImpl::Shutdown.

3) Call _exit(0) after the LSan verification.

Yup this makes sense to me % the fact that I honestly got a bit lost in this thread, and I am not sure what are the concerns against leaky-fying the LazyInstances

 

Here is another question: Do we really need to call _exit(0) instead of exit(0)? If we can call exit(0), we can just the current LSan verification and keep LeakyInstances non-leaky.

My only fear here is that by calling all the non-leaky LazyInstance dtors you might still hit (D)Checks if they make assumptions on each other lifetime and their destruction order violates those assumptions.

Which, if I understood correctly, is the original problem you are trying to solve.

[Kentaro Hara]

On Wed, Nov 2, 2016 at 6:59 PM, Primiano Tucci <prim...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 2:16 AM Kentaro Hara <har...@google.com> wrote:

On Tue, Nov 1, 2016 at 8:20 PM, Kentaro Hara <har...@google.com> wrote:

On Tue, Nov 1, 2016 at 8:14 PM, Primiano Tucci <prim...@chromium.org> wrote:

On Tue, Nov 1, 2016 at 4:46 AM Kostya Serebryany <k...@google.com> wrote:

On Mon, Oct 31, 2016 at 7:38 PM, Kentaro Hara <har...@google.com> wrote:

The question is:

Will LSan work as expected even when we cal exit(0) at the beginning of RenderThreadImpl::Shutdown()?

There are two things here:

1) IIRC the plan here is eventually to call _exit(0) not exit(0) (discussion here) 

I think LSan depends on the atexit hook to do its final analysis, which _exit(0) will kill entirely. Unless there is extra magic that hooks LSan differently in our test harness.

I noticed one more thing.

If we need to call _exit(0) (by calling base::Process::Terminate), it means not only that LSan doesn't run but also that LeakyInstances are not destructed. Doesn't it mean that we'll anyway need to make all LeakyInstances (at least all LeakyInstances on the renderer process) leaky?

Well calling _exit means that all the LazyInstance-s will be de-facto leaky.

At this point it might be conceptually cleaner to make them Leaky to reflect what's really happening.

Let me wrap the question around: what is the benefit of triggering the AtExit handlers? Why those non-laky LazyInstance are such? 

If my understanding is correct, we'll need to make the following changes:

1) Make all LeakyInstances leaky.

2) Move the LSan verification to the beginning of RenderThreadImpl::Shutdown.

3) Call _exit(0) after the LSan verification.

Yup this makes sense to me % the fact that I honestly got a bit lost in this thread, and I am not sure what are the concerns against leaky-fying the LazyInstances 

The concern some people raised on this thread is that destructors of some non-leaky LeakyInstances might be doing meaningful things -- so we want to keep the destructors.

However, Niko found that there are only(?) 276 non-leaky LeakyInstances, so we can just manually check their destructors and make them leaky by default.

 

Here is another question: Do we really need to call _exit(0) instead of exit(0)? If we can call exit(0), we can just the current LSan verification and keep LeakyInstances non-leaky.

My only fear here is that by calling all the non-leaky LazyInstance dtors you might still hit (D)Checks if they make assumptions on each other lifetime and their destruction order violates those assumptions.

Which, if I understood correctly, is the original problem you are trying to solve.

You're right. (Sorry, it looks like I also got lost in this long thread :-)

[Kentaro Hara]

So, to sum up, here is a final call.

Is there any objection to the following approach? Or do you have any simpler idea than this?

1) Make all LeakyInstances leaky after manually checking if their destructors are not doing meaningful things.

2) Move the LSan verification to the beginning of RenderThreadImpl::Shutdown.

3) Call _exit(0) just after the LSan verification.

[Gabriel Charette]

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

--

[Gabriel Charette]

Also, re. "doing work in LazyInstance's destructor": it sounds highly unlikely to me that any meaningful work is done there as (1) anything that doesn't make it to persistent state can't be useful and (2) anything that must make it to persistent state should have been flushed long before (especially in the renderers as they can't access persistent state directly nor can they communicate with the browser to do so on their behalf as pipes are closed as pipes are long-closed by static uninitialization time -- well I guess that's also after the sandbox was teared down so *maybe* writing to persistent state is allowed again but still sounds wrong).

I guess that merely leaves explicitly releasing resources like handles/etc. back to the OS, but I would assume that any sane kernel knows to free resources still owned by a terminated process, also making such operations useless.

[Kentaro Hara]

On Thu, Nov 3, 2016 at 12:16 AM, Gabriel Charette <g...@chromium.org> wrote:

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

What's the point of skipping _exit(0)?

I'm assuming you're proposing leaking all LeakyInstances and RenderThreadImpl (annotated with MEMORY_LEAK) and then run LSan at the AtExit hook. However, how is it different from just running LSan before calling _exit(0)?

[Alexei Svitkine]

Just to clarify my original concern and why I liked the exit approach better. The original suggestion was to change all LazyInstance behaviour - including browser process. My concern is there could be LazyInstances in browser process that do meaningful work in the dtor. (I am totally not concerned about the ones in renderer process - for the reasons Gab mentions.) So doing an exit only in renderer process seemed like the safest solution - provided someone is not willing to spend the time to audit all LazyInstance classes. If someone's willing to spend the time on the audit though, I have no concerns for changing the default for all process types - I just don't want us to break something subtle that we didn't know about.

[Gabriel Charette]

My points also apply to browser process (I was mostly highlighting that they applied even more to renderer processes), i.e. by the time main() unwinds, it's way too late IMO to do any meaningful (persistent) work even in the browser process. On the flip side, I agree that it makes me kind of uncomfortable recommending to flip the default for 250 unsurveyed callsites...

[Gabriel Charette]

On Wed, Nov 2, 2016 at 11:30 AM 'Kentaro Hara' via Chromium-dev <chromi...@chromium.org> wrote:

On Thu, Nov 3, 2016 at 12:16 AM, Gabriel Charette <g...@chromium.org> wrote:

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

What's the point of skipping _exit(0)?

I'm assuming you're proposing leaking all LeakyInstances and RenderThreadImpl (annotated with MEMORY_LEAK) and then run LSan at the AtExit hook. However, how is it different from just running LSan before calling _exit(0)?

Running LSan before calling _exit(0) earlier than it currently I expect will require further leak annotations (on non-globals). I would like to see a world where things are clearly owned (few leak annotations) but where we're also okay to terminate early when not verifying the sanity of the ownership graph (i.e. when #if !defined(LEAK_SANITIZER)).

[Kentaro Hara]

Then our plan is like this?

1) Rewrite single-process-mode tests to multi-process-mode tests. This is needed to make 2) work in single-process-mode tests.

2) If !#defined(LSAN), call _exit(0) at the beginning of RenderThreadImpl::Shutdown(). Otherwise, leak all LazyInstances and RenderThreadImpl and run LSan at the AtExit hook. Here we need to add MEMORY_LEAK to all LazyInstances and RenderThreadImpl.

In 2), I'd still prefer just calling LSan before _exit(0) to avoid having different code paths between LSan and !LSan, but let's see if it works.

[Primiano Tucci]

On Wed, Nov 2, 2016 at 3:16 PM Gabriel Charette <g...@chromium.org> wrote:

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

I think that will create a maintenance unbalance on LSan. Effectively LSan will become the only thing that will still cover the dependencies on destruction order, which is the problem haraken is trying to get rid of.

In other words sounds like we are saying "destruction order is causing a lot of headaches for our tests today due to UAF & co. Let's make that a headache only for tests that run under Lsan"

IMHO if we decide to go for the leak everything and die road the shutdown sequence should be:

#if defined(LEAK_SANITIZER).

__lsan_do_leak_check()

#endif

_exit(0);

I wonder if the auditing could be automated via some plugin which checks that is_recursively_trivial_destructible<T> for each T in those 250 non-leaky LazyInstance<T>.

The trick is that, AFAIK, there is no such thing like is_recursively_trivial_destructible. What I mean here is that it should be safe to make Leaky a class which:

(is_trivially_destructible) OR (has only unique_ptr/refptr to other classes which have the same property). Maybe some plugin enthusiast can come up with something like this?

[Primiano Tucci]

On Wed, Nov 2, 2016 at 3:37 PM Gabriel Charette <g...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 11:30 AM 'Kentaro Hara' via Chromium-dev <chromi...@chromium.org> wrote:

On Thu, Nov 3, 2016 at 12:16 AM, Gabriel Charette <g...@chromium.org> wrote:

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

What's the point of skipping _exit(0)?

I'm assuming you're proposing leaking all LeakyInstances and RenderThreadImpl (annotated with MEMORY_LEAK) and then run LSan at the AtExit hook. However, how is it different from just running LSan before calling _exit(0)?

Running LSan before calling _exit(0) earlier than it currently I expect will require further leak annotations (on non-globals).

Hmm why? It should be the other way round. By running LSan before calling (_)exit(0) we'd end up having a larger retaining graph which whitelists more allocations for LSan checks.

The only case that might require an annotation would be if you have an allocation which is unreachable from the LSan viewpoint but that is somehow still tracked by some class and would get freed by the non-leaky LazyInstance dtor (or by the dtor of one of the objects retained by the LazyInstances).

[Primiano Tucci]

On Wed, Nov 2, 2016 at 3:47 PM Kentaro Hara <har...@google.com> wrote:

Then our plan is like this?

1) Rewrite single-process-mode tests to multi-process-mode tests. This is needed to make 2) work in single-process-mode tests. 

Well, that or make them work in leaked single-process mode. If we agree with the rest of the plan I can try to do the mental and coding exercise of making those single-process tracing tests work in SPM. cannot volunteer for the other SPM tests though.

 

 2) If !#defined(LSAN), call _exit(0) at the beginning of RenderThreadImpl::Shutdown(). Otherwise, leak all LazyInstances and RenderThreadImpl and run LSan at the AtExit hook.

Hmm not sure I get the "otherwise" part. Are you suggesting that non-leaky LazyInstance should become Leaky when defined(LSAN)? If so what is the advantage of just invoking lsan before _exit?

 

Here we need to add MEMORY_LEAK to all LazyInstances and RenderThreadImpl.

LazyInstances (and RTI itself) are globals. Globals are retaining roots, you shouldn't need any annotation for these.

 

In 2), I'd still prefer just calling LSan before _exit(0) to avoid having different code paths between LSan and !LSan, but let's see if it works.

Yup I'd definitely avoid to keep different paths for LSan.

Maybe I'm missing something here, but to me it sounds that if you call __lsan_do_leak_check() + _exit(0) on RTI::Shutdown you achieve the same result of having made the non-leaky LazyInstance in the renderer leaky.

At which point switching all LazyIstance to be leaky becomes an orthogonal, IMHO still desirable, action that can be dealt with incrementally.

[Gabriel Charette]

On Wed, Nov 2, 2016 at 11:57 AM Primiano Tucci <prim...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 3:37 PM Gabriel Charette <g...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 11:30 AM 'Kentaro Hara' via Chromium-dev <chromi...@chromium.org> wrote:

On Thu, Nov 3, 2016 at 12:16 AM, Gabriel Charette <g...@chromium.org> wrote:

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

What's the point of skipping _exit(0)?

I'm assuming you're proposing leaking all LeakyInstances and RenderThreadImpl (annotated with MEMORY_LEAK) and then run LSan at the AtExit hook. However, how is it different from just running LSan before calling _exit(0)?

Running LSan before calling _exit(0) earlier than it currently I expect will require further leak annotations (on non-globals).

Hmm why? It should be the other way round. By running LSan before calling (_)exit(0) we'd end up having a larger retaining graph which whitelists more allocations for LSan checks.

Doesn't "retaining graph" == "leak annotations"? i.e. we have to mark more things as "leaks" == "not yet deleted when running the check"?

[Gabriel Charette]

On Wed, Nov 2, 2016 at 11:52 AM Primiano Tucci <prim...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 3:16 PM Gabriel Charette <g...@chromium.org> wrote:

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

I think that will create a maintenance unbalance on LSan. Effectively LSan will become the only thing that will still cover the dependencies on destruction order, which is the problem haraken is trying to get rid of.

In other words sounds like we are saying "destruction order is causing a lot of headaches for our tests today due to UAF & co. Let's make that a headache only for tests that run under Lsan"

Ah right, I lost track of the original goal... But I still don't like tying memory management to application lifetime policy. If we can whitelist a few top-level objects for LSAN from which it can derive everything that is intentionally not deleted when we do the early LSAN check and as such decouple the two then that's fine by me. On the flipside if we will constantly need to add leak annotations with comments like "owned but LSAN check happens before owner goes away", then I'm not excited by this..

[Nico Weber]

I'd like to reiterate that I think it's a great change to make LazyInstance leaky by default. There aren't many of those, they're auditable (especially if people feel that renderer-side instances don't need any auditing), and the majority of them are probably just non-leaky because they predate the recommendation to prefer leaky LazyInstances.

[Primiano Tucci]

On Wed, Nov 2, 2016 at 4:33 PM, Gabriel Charette <g...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 11:57 AM Primiano Tucci <prim...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 3:37 PM Gabriel Charette <g...@chromium.org> wrote:

On Wed, Nov 2, 2016 at 11:30 AM 'Kentaro Hara' via Chromium-dev <chromi...@chromium.org> wrote:

On Thu, Nov 3, 2016 at 12:16 AM, Gabriel Charette <g...@chromium.org> wrote:

That SGTM. But I'd like to ponder another potential approach:

We could skip _exit(0) #if defined(LEAK_SANITIZER). I was thinking of using this approach for the browser-side of things. That way we disassociate memory management from application lifetime. LSAN ensures that the ownership graph is sane (everything can be and is teared down in a full shutdown), but in the official product we make the decision of terminating the application early (which is orthogonal to memory management).

WDYT?

What's the point of skipping _exit(0)?

I'm assuming you're proposing leaking all LeakyInstances and RenderThreadImpl (annotated with MEMORY_LEAK) and then run LSan at the AtExit hook. However, how is it different from just running LSan before calling _exit(0)?

Running LSan before calling _exit(0) earlier than it currently I expect will require further leak annotations (on non-globals).

Hmm why? It should be the other way round. By running LSan before calling (_)exit(0) we'd end up having a larger retaining graph which whitelists more allocations for LSan checks.

Doesn't "retaining graph" == "leak annotations"? i.e. we have to mark more things as "leaks" == "not yet deleted when running the check"?

After doing some tests and reading this, turns out that everything that is still reachable (it seems stack and globals) is *not* considered a leak.

If you look at the example in http://pastebin.com/wkCmZWXd and run with LSan that one does NOT report any leak.

It reports a leak if you "new X" twice and lose track of the old |obj| or just do that in another function where the stack gets lost on return.

Anything to which you still have a pointer is considered as a non-leak.

From https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizerDesignDocument

"LSan scans this memory for byte patterns that look like pointers to heap blocks (interior pointers are treated the same as pointers to the beginning of a block). Those blocks are considered reachable and their contents are also treated as live memory. In this manner we discover all blocks reachable from the root set. We mark such blocks by setting a flag in the block's metadata. We then iterate over all existing heap blocks and report unreachable blocks as leaks, together with stack traces of corresponding allocations. Instead of reporting each allocation individually, it might be more useful to aggregate them by the last k frames in the stack trace."

Essentially what is considered a leak is a memory allocation for which no pointer was discovered in memory accessible from the roots.

Now if you think to LazyInstance, they are essentially global roots that keep everything referenced. Hence I believe that the suppressions are a non-problem here.

 

...

[Message clipped]  

[Gabriel Charette]

Ah ok, awesome, then terminating process early, even under LSAN is the best option IMO.

And I guess that makes the whole Leaky LazyInstance discussion orthogonal but I would still very much like to see this happen as we discussed a few months ago.

[Peter Kasting]

Random note: Implementing the idea of "call LSAN function before _exit(0)" sounds like it might be a motivator to use std::[at_]quick_exit(), which is currently in the "TBD" section of the Chromium C++11 style guide.  If you think that would be useful for this, I'd send a proposal to the cxx list that we allow this.  I doubt there would be any objection.

PK

[Kentaro Hara]

Just a quick update:

I've finally removed a shutdown sequence from a renderer process (CL). As a follow-up, I'll remove blink::shutdown etc because they're no longer needed :)

FWIW, I didn't have to make LeakyInstances leaky by default or rewrite single-process-mode tests in multi-process-mode to remove the shutdown sequence (it turned out that they are orthogonal issues). So they are still TODOs.

Thanks!

On Sat, Nov 5, 2016 at 5:59 AM, Peter Kasting <pkas...@chromium.org> wrote:

Random note: Implementing the idea of "call LSAN function before _exit(0)" sounds like it might be a motivator to use std::[at_]quick_exit(), which is currently in the "TBD" section of the Chromium C++11 style guide.  If you think that would be useful for this, I'd send a proposal to the cxx list that we allow this.  I doubt there would be any objection.

PK

[Scott Graham]

Hi, I landed a slightly different approach on this.

I switched us from defaulting to running a destructor at shutdown to not having a default at at all. That is, the author now needs to select LazyInstance<T>::Leaky, or LazyInstance<T>::DestructorAtExit explicitly.

I believe many LazyInstance<T>s using (previously) default behaviour are unnecessarily and often unintentionally causing the AtExitManager to run their destructor at process shutdown.

It is my position that one ought to normally select ::Leaky. When writing or reviewing a ::DestructorAtExit, please consider if it should be ::Leaky instead.

There's a tracking bug at https://crbug.com/698982 which you can jump on to if you want to switch some |DestructorAtExit|s, or have opinions on LazyInstance, or renderer shutdown, or browser shutdown, or leaks, or threadsafe static initialization, or maybe garbage collection, or possibly say TLS, or you know whatever. That bug is here to listen.

--

[Daniel Cheng]

I threw together a simple spreadsheet to track switching away from DestructorAtExit in https://docs.google.com/spreadsheets/d/1OkrLRcI3vIUAbCeqLn0mD-0Oj1SGvEHb6d-9q9rYfP8/edit#gid=0 to help coordinate and prevent collisions.

Daniel

[Wez]

Do we need new guidelines for making sure unit-tests don't have inter-dependencies due to leaky singletons/lazy-instances?

[Scott Graham]

Hmm, good question.

It seems unfortunate to have a bunch of LazyInstances that in the browser could happily be Leaky, but that are DestructorAtExit only for test purposes. (context TestSuite::PreInitialize)

I suppose we could:

1) do nothing, and rely on our exceedingly good judgement and careful testing to avoid inter-test dependencies;

2) decide that what Leaky really means is: LeakyButOnlyInTheBrowserAndOtherwisePerTestSuite;

3) add a third ::Something to distinguish between fully always leaky, and those that are leaky, but cleaned up per test in test situations.

#3 seems better at first, but it mightn't be obvious which to choose unless we think of a good name.

#2 seems subtle, but may actually be what people expect. The drawback is that when you write ::Leaky, you're probably expecting that you don't need to think about the destructor ever being called.

Maybe just fewer LazyInstances is the real answer.

[Primiano Tucci]

Would be nice if leaky objects could still be leaky in tests (because invoking dtors often causes ordering problems that we established we don't care about) but re-created (hence the old instance *actually* leaked) when we start a new test (fixture? Unit?)

I am thinking something on the lines of :
- LazyInstance having a global int g_generation_for_testing
- the Pointer() method having something like: if (this.generation ! = g_generation_for_testing) obj_ = new T() ; (behind #if official so we don't pay the cost of that "if" in official builds)
- the test harness invoking LazyInstance::IncrementGenerationForTesting()

[Scott Hess]

But sometimes singletons represent things that are literal singletons.

It would be nice if we could force people to think this issue through using a cute naming strategy, but I think the evidence is against us on that one.  I'd kinda lean towards default-leaky and require developers to actively work to schedule shutdown work.  And maybe call it DestructorAtExitAndDestructorsRunInRandomOrder().

-scott

[Daniel Cheng]

Hmm... I was just reminded that leaky lazy instances can often be replaced with a function-level static. Is there any reason we'd want to keep around Leaky?

Daniel

[Wez]

LazyInstance::Leaky copes with threads, where a function-level static doesn't, right?

[Daniel Cheng]

We have thread-safe statics now: https://groups.google.com/a/chromium.org/d/msg/cxx/j5rFewBzSBQ/JFvyU959AAAJ

Daniel

[Daniel Cheng]

Hmm... I guess one difference is that LazyInstance will allocate into BSS, while using a function-level static will allocate off the general heap... I'm not sure if that really matters though.

Daniel

[Scott Graham]

For now, I guess the important thing is just to be aware of how you're changing test behaviour if you decide to convert DestructorAtExit to either Leaky or a function level static. --gtest_shuffle --single-process-tests might be useful.

I don't personally have strong feelings on using Leaky vs. function level statics (though that's why I was looking at LazyInstance). If we need something like Primiano's suggestion for test reliability, then having Leaky as a separate concept makes that more tractable.

[Primiano Tucci]

Oh you are right about leaky lazy instances going away. I think Scott himself (or somebody else, I can't remembere) already refactored most of them.

>guess one difference is that LazyInstance will allocate into BSS, while using a function-level static will allocate off the general heap... I'm not sure if that really matters though.

what I see that most leaky instances got translated into: static  T* x = new T() so they are heap based anyways. Not sure if there is a good reason to though.

[Daniel Cheng]

Well, LazyInstance uses AlignedMemory internally and placement-news into the AlignedMemory, so there's no heap allocation: that's the difference I meant.

Daniel

[Scott Graham]

After shess mentioned singletons, I see we have quite a lot of those too, and that they also default to running destructors at process exit, which seems even more questionable.

I tried browsing for 30s at head, and there's 382 in the stack of AtExitManager Closure()s. Maybe we ought to be pleasantly surprised we accomplish orderly shutdown in finite time!

It might be a "fun" project to track how long those take to run in the real world (tricky since metrics will be shutting down too), and work on driving that to 0.

[Primiano Tucci]

Isn't alignedmemory just  a wrapper around malloc? 

Re tracking down singletons at shutdown : add a trace event to singleton dtor and then use --trace-to-console. 

As long as the tracing controller is not a singleton itself (even if it is, it  *should* be I initialized quite early and hence torn down quite late) it should do the job. I think you can still feed that std out into chrome://tracing 

[Daniel Cheng]

No, AlignedMemory is equivalent to std::aligned_storage: it's a POD type that's sized to the template parameter. You allocate directly into the POD storage with placement-new, and free by directly invoking the destructor; thus, heap allocation is never involved.

Daniel

[Gabriel Charette]

On Thu, Mar 9, 2017 at 7:47 AM Scott Graham <sco...@chromium.org> wrote:

After shess mentioned singletons, I see we have quite a lot of those too, and that they also default to running destructors at process exit, which seems even more questionable.

I tried browsing for 30s at head, and there's 382 in the stack of AtExitManager Closure()s. Maybe we ought to be pleasantly surprised we accomplish orderly shutdown in finite time!

I think that perhaps what we ought do is not run any of those closures on browser shutdown (i.e. skip AtExitManager's signal altogether). As discussed previously in this thread: there are many cases where Chrome self-terminates and never gets to unwind main() anyways (i.e. always on Android and on end-session on Windows).

As such, it's way too late to do anything which involves persisting worthwhile state when main() unwinds (re. "services should have a shutdown phase, not a teardown phase" which I stressed multiple times in the past) -- and anything that doesn't persist state is futile.

Therefore the only purpose of AtExitManager is avoiding memory leaks which is really only useful in unit tests for isolation. But even then... unit tests shouldn't generally depend on the state of global objects..?

Primiano's idea of generations to allow leaky+isolation is neat but has the potential of bloating unit test processes which run many tests in a row, potentially causing other subtle issues? Maybe we should just shard to one process per fixture or something and get rid of AtExitManager altogether?

PS: Getting rid of AtExitManager would allow us to terminate process at the end of main() (and I have ideas to move that even earlier) instead of letting things unwind and would allow us to make the default TaskShutdownBehavior be CONTINUE_ON_SHUTDOWN (not currently the case because any task that uses objects registered with AtExitManager one way or another must not outlive main() -- no longer a problem if we terminate process :)).

It might be a "fun" project to track how long those take to run in the real world (tricky since metrics will be shutting down too), and work on driving that to 0.

Best way would probably be to track timings in a leaky singleton which saves its state to a memory backed file (the OS will be responsible to flush it to disk after process termination) and upload metric in next run of Chrome -- this is similar to how the new fangled "persistent UMA" system works.