The Service Manager should be able to launch variously sandboxed processes on all platforms. It should not know about specific process types. Configuration should be expressed in service manifests.
@rockot:
With Chromium Embedded Framework we currently call the ContentMainRunner Initialize, Run and Shutdown methods separately [1]. This is for the following reasons:
1. Initialize() initializes the global CommandLine object. CEF references the global CommandLine object for some CEF-specific initialization between Initialize() and Run().
2. CEF optionally supports integration into an existing application message loop instead of using Chromium's default message loop. This requires the embedder to call Shutdown() separately at a later time.
This can also work with the new service_manager model if service_manager::Main() is split into Initialize, Run and Shutdown components. The Main() method then becomes:
int Main(MainParams& params) {
int exit_code = MainInitialize(params);
if (exit_code >= 0)
return exit_code;
exit_code = MainRun(params);
MainShutdown(params);
return exit_code;
}
I've prototyped this change locally and the only somewhat unpleasant requirement is moving the ScopedNSAutoreleasePool to MainParams so that it can be referenced from MainInitialize() and MainShutdown().
If these changes sound reasonable I can submit a CR.
Thanks,
Marshall
[1] https://bitbucket.org/chromiumembedded/cef/src/2f6475c0d89c8b94458b6f7a1146724dbccde09f/libcef/browser/context.cc?at=master&fileviewer=file-view-default#context.cc-309
service_manager::MainDelegate has Initialize, Run, and Shutdown already. I
don't see why it would make sense to introduce MainInitialize, MainRun, and
MainShutdown in addition. You just implement the delegate methods and run
Main().
If you need more control over when/how Run and/or Shutdown are called, we
can discuss those details and find a way to make the delegate API
sufficient for that purpose.
I would discourage any such changes.
Note that there is work in progress here already anyway. For example I'm
changing MainDelegate to look something like:
https://chromium-review.googlesource.com/c/477393/1/services/service_manager/embedder/main_delegate.h
(so you'd implement RunEmbedderProcess and ShutDownEmbedderProcess instead)
@rockot: The problem is that you've moved startup logic from ContentMainRunner to service_manager::Main(). I cannot properly initialize Chromium for use with CEF without either duplicating or splitting the functionality in service_manager::Main(). This is why I originally refactored [1] ContentMainRunner into the form that you found it. Lets please discuss how to improve this situation without requiring code duplication.
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=112507
I've moved code around but otherwise have not changed the way content
embedders see startup in any way. You till implement ContentMainDelegate
yes? I don't understand how this could have an effect on CEF?
@rocket: Not all content embedders call ContentMain(). CEF calls the ContentMainRunner Initialize, Run and Shutdown methods separately as linked in comment #17. Consequently when you move code out of ContentMainRunner and into service_manager::Main() that code is no longer executed when (for example) ContentMainRunner::Initialize() is called directly by CEF.
I see. My assumption was that all content embedders called ContentMain.
I don't like the idea of introducing a MainInitialize, MainRun, and
MainShutdown, so let's find a better solution.
@rockot: Keeping all functionality in a delegate class instead of a single runtime function would likely resolve the issue. For example, if service_manager::Main() can be implemented as follows using your proposed MainDelegate class:
int Main(MainDelegate* delegate, ...) {
int exit_code = delegate->Initialize(...);
if (exit_code >= 0)
return exit_code;
exit_code = delegate->Run(...);
delegate->Shutdown(...);
return exit_code;
}
All of the functionality that currently exists in service_manager::Main() would then be moved to a MainDelegate base class or similar where it can be shared by embedders that are calling the MainDelegate methods directly instead of using service_manager::Main().
I was re-directed from the CEF website to this issue regarding my comments on the implementation of InitializeMac in mac_init.mm.
Setting NSApplicationCrashOnExceptions to YES is very problematic and should be removed.
In Photoshop's use of Cef we have seen crashes caused by this when interacting with the IME. If you select all in an edit field that contains composite characters and then type a character, then sometimes we see a crash because Apple's IME handler throws an obj-c exception. In normal circumstances Apple's internal (carbon) event dispatcher catches and ignores this exception, but with NSApplicationCrashOnExceptions set to YES the host process is terminated.
We discussed this with Apple, and their input is that NSApplicationCrashOnExceptions=YES should only be used during development and not in release products. For development we do not need to set this from code, but you can instead set this using the normal defaults write mechanism.
I (strongly) advocate that Chromium removes this override.
This bug exists to track refactoring of existing code.
Re comment #27: The change to set NSApplicationCrashOnExceptions=YES was made 10 months ago in r403545. If you think that's a bug, please file a new bug for it instead of posting about it here.
Re comment #28: If you believe Ozone Chrome OS, please file a new bug for it instead of posting about it here.
Comment 1 by ben@chromium.org
, Oct 12 2016