Shutdown leaks for the win!

[Gabriel Charette]

Hello all,

I think we should default to make LazyInstances and Singletons use Leaky traits (I'm not proposing changing the 1000+ instances myself just yet, but merely having everyone agree that devs and reviewers should lean towards Leaky traits).

I can think of at least two recent problems caused by non-leaky instances (which are deleted by the AtExitManager when main returns):

1) Long-standing stack overflow in AssertSingletonAllowed() on Linux as the NOTREACHED() trigger uses the SandboxSymbolizeHelper which was a non-leaky Singleton (which calls AssertSingletonAllowed() on the thread it's obtained..!): https://codereview.chromium.org/2221063004/

2) Non-leaky Singleton/LazyInstances that were used from CONTINUE_ON_SHUTDOWN tasks (potentially causing use-after-frees): https://codereview.chromium.org/1904273002/, https://codereview.chromium.org/2183253002 

In all cases of LazyInstance/Singleton I've seen, the instances are meant to live throughout the process' lifetime and do not have any cleanup to do in their destructor. And even for those that actually do cleanup in their destructor, it's probably incorrect in most cases:

• Closing handles/freeing resources is overkill as the kernel will do just that on the imminent process exit (don't clean the carpets before burning down the building).
  
• It's way too late to save any critical state, in fact on Windows the OS considers the process ripe for termination as soon as the last window goes away and, as such, we self-TerminateProcess early on end-session to make this deterministic (skipping any AtExit registration anyway).

In the end, I guess I'm asking what the point of AtExit even is? I can't think of a valid use case but I must be missing knowledge of some components' shutdown requirements.

Cheers,

Gab

[Nico Weber]

+1, I always ask for this in reviews as well.

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

[Alexei Svitkine]

+1

[Scott Hess]

On Tue, Aug 9, 2016 at 10:04 AM, Gabriel Charette <g...@chromium.org> wrote:
> I think we should default to make LazyInstances and Singletons use Leaky
> traits (I'm not proposing changing the 1000+ instances myself just yet, but
> merely having everyone agree that devs and reviewers should lean towards
> Leaky traits).

+1

> In the end, I guess I'm asking what the point of AtExit even is? I can't
> think of a valid use case but I must be missing knowledge of some
> components' shutdown requirements.

I believe it's because if we don't manage the process ourselves, then
the underlying system will step in and manage it for us, generally
even less correctly.

-scott

[Elliott Sprehn]

At least in the render process there's no reason to ever run exit callbacks since we kill the process to shutdown.

[Wez]

IIRC we instantiate an AtExitManager in renderer processes, but not in the browser process, right now.

Do our unit-tests perhaps rely on the AtExit handlers in LazyInstance and Singleton?

[Gabriel Charette]

On Tue, Aug 9, 2016 at 3:25 PM Wez <w...@chromium.org> wrote:

IIRC we instantiate an AtExitManager in renderer processes, but not in the browser process, right now.

There seems to be an AtExitManager in every process.

 

Do our unit-tests perhaps rely on the AtExit handlers in LazyInstance and Singleton?

Ah, unit-testing is an interesting use case per independent tests sharing a process -- though unittests should generally be written to be independent of global state (or otherwise be aware of and support interaction with relevant tests).

[Marc-Antoine Ruel]

IIRC, the main reason singleton was not leaking by default was due to ChromeFrame. RIP.

M-A

M-A

[Greg Thompson]

On Thu, Aug 11, 2016 at 3:40 AM Marc-Antoine Ruel <mar...@chromium.org> wrote:

IIRC, the main reason singleton was not leaking by default was due to ChromeFrame. RIP.

Interesting. Are there implications with CEF or headless Chrome, then?

[Marc-Antoine Ruel]

I was not directly involved as a recall and this is from ~7 years ago. It was specifically a problem in the case where chrome.dll is unloaded and reloaded repeatedly. I don't know if this is a use case we care about at all.

TL;DR; We should aim at making Chrome fast.

M-A

--

M-A

[Sami Kyostila]

to 11. elokuuta 2016 klo 13.21 Greg Thompson <g...@chromium.org> kirjoitti:

On Thu, Aug 11, 2016 at 3:40 AM Marc-Antoine Ruel <mar...@chromium.org> wrote:

IIRC, the main reason singleton was not leaking by default was due to ChromeFrame. RIP.

Interesting. Are there implications with CEF or headless Chrome, then?

With headless we generally expect people to use a long lived browser instance whose lifetime matches that of the process, so I don't think it needs any special treatment here. I *think* CEF is similar but I don't know for sure.

- Sami

 

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.

[Marshall Greenblatt]

On Fri, Aug 12, 2016 at 4:28 PM, Sami Kyostila <skyo...@chromium.org> wrote:

to 11. elokuuta 2016 klo 13.21 Greg Thompson <g...@chromium.org> kirjoitti:

On Thu, Aug 11, 2016 at 3:40 AM Marc-Antoine Ruel <mar...@chromium.org> wrote:

IIRC, the main reason singleton was not leaking by default was due to ChromeFrame. RIP.

Interesting. Are there implications with CEF or headless Chrome, then?

With headless we generally expect people to use a long lived browser instance whose lifetime matches that of the process, so I don't think it needs any special treatment here. I *think* CEF is similar but I don't know for sure.

We have the same expectation in CEF, but only because it's a requirement from Chromium.

Being able to shut down and re-initialize CEF/Chromium multiple times is a pretty common request from embedders. For example, multiple plugins that are loaded/unloaded in a third-party application like Photoshop or Unreal Engine might want to use CEF as the browser control. This becomes complicated if the third-party application itself does not manage CEF's lifespan (which internally uses ContentMainRunner, etc.) by calling initialize and shutdown at the appropriate times. There are ways to work around the single-initialization limitation (e.g. create another DLL that manages CEF with reference counting, and have all the other plugins share that DLL instead of calling into CEF/Chromium directly), but that's quite fragile and falls apart if a single entity doesn't control all of the plugins.

As an aside, please don't add forced/early termination in the Chromium layer. Many embedders need to perform their own cleanup in the main process after shutting down Chromium and before exiting the application.

 

- Sami

 

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.

[Torne (Richard Coles)]

It's not that we need to add early termination explicitly; it's that we *don't* want to guarantee that we have a "correct" shutdown process. CEF embedders should probably not even be trying to shut down chromium - they should just do their own cleanup, and then exit without caring about CEF :)

 

- Sami

 

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
---
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.

[Marshall Greenblatt]

On Aug 12, 2016, at 5:33 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

It's not that we need to add early termination explicitly; it's that we *don't* want to guarantee that we have a "correct" shutdown process. CEF embedders should probably not even be trying to shut down chromium - they should just do their own cleanup, and then exit without caring about CEF :)

That sounds good to me, provided all caches and databases are finalized/shut down correctly to ensure data persistence and avoid corruption. That doesn't really work currently unless you tear down various objects and threads in the correct order, and consequently it's likely not possible to trigger those steps from a process termination hook.

CEF handles shutdown currently by blocking in the shutdown function until everything has been torn down. Maybe we should instead always trigger data cleanup steps when the last browser window is destroyed and then provide an async callback for when it's safe to terminate the app. However, everything would need to restart gracefully if the app then created a new browser window instead of terminating since not all apps have a browser window existing/visible at all times.

[Nico Weber]

On Fri, Aug 12, 2016 at 11:17 AM, Marshall Greenblatt <magree...@gmail.com> wrote:

On Aug 12, 2016, at 5:33 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

It's not that we need to add early termination explicitly; it's that we *don't* want to guarantee that we have a "correct" shutdown process. CEF embedders should probably not even be trying to shut down chromium - they should just do their own cleanup, and then exit without caring about CEF :)

That sounds good to me, provided all caches and databases are finalized/shut down correctly to ensure data persistence and avoid corruption.

That needs to work when something crashes too, so if you know examples of "if I exit the process at this point, then file X gets corrupted", please file bugs for those :-)

 

 

- Sami

 

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.

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