Issue metadata
Sign in to add a comment
|
Crash when using ContentMainDelegate::RunProcess due to BrowserProcessSubThread destruction |
||||||||||||||||||||||
Issue descriptionChrome Version: M68 and master revision 3ca9e649739 (#565518). OS: Linux, macOS What steps will reproduce the problem? (1) Create an application using the Content API that implements ContentMainDelegate::RunProcess. The custom RunProcess implementation eventually calls BrowserMainRunner::Initialize. For example: https://bitbucket.org/chromiumembedded/cef/src/4f28c5ffffd00f9a8fa67392e594e5207cb65158/libcef/common/main_delegate.cc#lines-573 (2) Run the application. What is the expected result? The application should not crash. What happens instead? The application crashes with the following call stack: [0614/113806.881233:FATAL:thread_restrictions.cc(29)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as block ing was called from a scope that disallows blocking! If this task is running inside the TaskScheduler, it needs to have MayBlock () in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBl ocking in a narrow scope. #0 0x7f56e3c5390d base::debug::StackTrace::StackTrace() #1 0x7f56e39ef25c base::debug::StackTrace::StackTrace() #2 0x7f56e3a4c66a logging::LogMessage::~LogMessage() #3 0x7f56e3b9b83b base::AssertBlockingAllowed() #4 0x7f56e3c82b27 base::PlatformThread::Join() #5 0x7f56e3b95234 base::Thread::Stop() #6 0x7f56dfef2fd9 content::BrowserProcessSubThread::~BrowserProcessSubThread() #7 0x7f56dfef31c9 content::BrowserProcessSubThread::~BrowserProcessSubThread() #8 0x7f56e351aaa0 content::ContentMainRunnerImpl::Run() #9 0x7f56e350f2b5 content::ContentServiceManagerMainDelegate::RunEmbedderProcess() #10 0x7f56e737dc1a service_manager::MainRun() Please use labels and text to provide additional information. This problem is caused by https://crrev.com/6740d6248c which adds a std::unique_ptr<BrowserProcessSubThread> argument to RunBrowserProcessMain [1]. This argument is not passed to ContentMainDelegate::RunProcess. Consequently the BrowserProcessSubThread object is destructed when RunBrowserProcessMain returns, resulting in the above assertion. Proposed solution: The std::unique_ptr<BrowserProcessSubThread> argument should be passed to ContentMainDelegate::RunProcess and the BrowserMainRunnerImpl::Initialize variant that accepts this argument should be exposed via BrowserMainRunner. The BrowserProcessSubThread header likely needs to be abstracted so that it can live in content/public/browser. [1] https://cs.chromium.org/chromium/src/content/app/content_main_runner_impl.cc?type=cs&q=RunBrowserProcessMain&sq=package:chromium&g=0&l=602
,
Jun 15 2018
> Passing std::unique_ptr<BrowserProcessSubThread> directly to ContentMainDelegate::RunProcess() will break DEPS. Right, that's why I suggested abstracting BrowserProcessSubThread so that the header can live in content/public/browser and thereby not break DEPS. Alternately, perhaps we can pass as the parent type (base::Thread) and BrowserMainRunnerImpl can static_cast back to BrowserProcessSubThread. There is some precedent for this pattern within content/browser currently. > An alternative solution is: we could move the calls of running BrowserMainRunner (Create/Initialize/Run)from each subclasses that implements ContentMainDelegate::RunProcess(), and make them all go through the call of BrowserMain() in ContentMainRunnerImpl::RunBrowserProcessMain. In my code (linked above) I do two things that I don't think would be possible with your alternate proposal: 1. I support a run mode on Windows that calls BrowserMainRunner::Initialize on a different thread (e.g. the UI thread is different from the main application thread). 2. I expose the BrowserMainRunner Create/Initialize/Run calls separately so that the embedding application can integrate with an existing message loop instead of always running the Chromium message loop. In conclusion I would prefer to keep the ContentMainDelegate pattern and find an acceptable way to pass the BrowserProcessSubThread object.
,
Jun 15 2018
Just reading your code, yes, it is a different issue and won't work with my solution. Actually, I think for the failure you see, the fix could be when exit_code > 0, call service_manager_thread_.reset() before return in https://cs.chromium.org/chromium/src/content/app/content_main_runner_impl.cc?rcl=36ec54e24c89454587c466d697df3885daf3d9d2&l=616 and the Android case.
,
Jun 19 2018
I'm fine with a public header for BrowserProcessSubThread (we had this at some point in a patch set but got rid of it before landing as it didn't end up needing to be public in the last revision). It makes sense to pass it to Run() I think as this thread shouldn't outlive Run() (and passing ownership enforces that). Le ven. 15 juin 2018 17 h 14, hanxi via monorail < monorail+v2.684488008@chromium.org> a écrit :
,
Jun 19 2018
SGTM, I will fix it.
,
Jul 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4fbe7c352ce0ef0550831d66fe0377a0bf018f04 commit 4fbe7c352ce0ef0550831d66fe0377a0bf018f04 Author: Xi Han <hanxi@google.com> Date: Tue Jul 10 22:09:12 2018 Introduce StartupData Interface. Introduce StartupData in /content/public/browser, and StartupDataImpl in /content/browser. This is a precursor CL for: https://crrev.com/c/1108178. In the follow up CL, StartupData* will be plumbed via MainFunctionParams to create the browser main loop. Bug: 846846 , 853308 Change-Id: Ic192cfa696439996dec07435f1980d78282f15db Reviewed-on: https://chromium-review.googlesource.com/1117471 Commit-Queue: Xi Han <hanxi@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#573943} [modify] https://crrev.com/4fbe7c352ce0ef0550831d66fe0377a0bf018f04/content/browser/BUILD.gn [add] https://crrev.com/4fbe7c352ce0ef0550831d66fe0377a0bf018f04/content/browser/startup_data_impl.cc [add] https://crrev.com/4fbe7c352ce0ef0550831d66fe0377a0bf018f04/content/browser/startup_data_impl.h [modify] https://crrev.com/4fbe7c352ce0ef0550831d66fe0377a0bf018f04/content/public/browser/BUILD.gn [add] https://crrev.com/4fbe7c352ce0ef0550831d66fe0377a0bf018f04/content/public/browser/startup_data.h
,
Jul 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4090dccedd829fec4832c4c35546501ca6dc6108 commit 4090dccedd829fec4832c4c35546501ca6dc6108 Author: Xi Han <hanxi@google.com> Date: Wed Jul 11 03:15:20 2018 Plumb pre-created service manager thread when creating BrowserMainLoop. The service manager thread and (TODO) ServiceManager might be created before the full browser starts, and we want to reuse them when starting the full browser. Therefore, we add a pointer of BrowserStartupData in MainFunctionParams. Particularly, in this CL, ContentMainRunnerImpl creates and owns a BrowserStartupData object. It passes a pointer of the BrowserStartupData through the main function parameter to BrowserMainLoop. The BrowserStartupData interface was introduced in: https://crrev.com/c/1117471. Bug: 846846 , 853308 Change-Id: Ie11063227a670cd8d72935131e854ee2b5c46e4e Reviewed-on: https://chromium-review.googlesource.com/1108178 Commit-Queue: Xi Han <hanxi@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#574054} [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/app/content_main_runner_impl.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/app/content_main_runner_impl.h [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/app/content_service_manager_main_delegate.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/app/content_service_manager_main_delegate.h [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/browser/browser_main.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/browser/browser_main.h [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/browser/browser_main_loop.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/browser/browser_main_loop.h [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/browser/browser_main_loop_unittest.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/browser/browser_main_runner_impl.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/browser/browser_main_runner_impl.h [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/public/common/main_function_params.h [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/content/public/test/browser_test_base.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/services/service_manager/embedder/main.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/services/service_manager/embedder/main_delegate.cc [modify] https://crrev.com/4090dccedd829fec4832c4c35546501ca6dc6108/services/service_manager/embedder/main_delegate.h
,
Jul 11
hanxi@ - Could you please provide manual repro steps or app to verify the issue from TE-end. Thanks...!!
,
Jul 11
Hi marshall@: could you provide your app to verify whether it works?
,
Jul 11
Thanks for working on this. I'll update my app for these changes in the next few days and report back with any issues.
,
Jul 13
I've tested the changes and they look good. Thanks!
,
Jul 13
Awesome! Thanks for verifying! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by hanxi@chromium.org
, Jun 15 2018