Revert of memory-infra: Finish moving to Mojo (2nd attempt) (issue 2738093003 by msw@chromium.org)

[m...@chromium.org]

Reviewers: Primiano Tucci (slow - perf), ssid, kenrb, jam, chiniforooshan
CL: https://codereview.chromium.org/2738093003/

Message:
Created Revert of memory-infra: Finish moving to Mojo (2nd attempt)

Description:
Revert of memory-infra: Finish moving to Mojo (2nd attempt) (patchset #8
id:140001 of https://codereview.chromium.org/2734193002/ )

Reason for revert:
Prospective revert for nacl_integration crash starting on this build:

https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29/builds/61497

[13142:13142:0309/150217.391584:FATAL:memory_dump_manager.cc(220)] Check failed:
!delegate_.
#0 0x7ff7db583b5b base::debug::StackTrace::StackTrace()
#1 0x7ff7db5821ec base::debug::StackTrace::StackTrace()
#2 0x7ff7db5f06df logging::LogMessage::~LogMessage()
#3 0x7ff7db7ac18e base::trace_event::MemoryDumpManager::Initialize()
#4 0x7ff7dd27a73a
memory_instrumentation::MemoryDumpManagerDelegateImpl::MemoryDumpManagerDelegateImpl()
#5 0x7ff7dd27a293
memory_instrumentation::MemoryDumpManagerDelegateImpl::Create()
#6 0x7ff7dd26e723 memory_instrumentation::CoordinatorImpl::CoordinatorImpl()
#7 0x7ff7dd26e4da memory_instrumentation::CoordinatorImpl::GetInstance()
#8 0x7ff7dd7dbdc0 ChromeContentBrowserClient::ExposeInterfacesToRenderer()
#9 0x7ff7d54717e8 content::RenderProcessHostImpl::RegisterMojoInterfaces()
#10 0x7ff7d546eb2f content::RenderProcessHostImpl::Init()
#11 0x7ff7d4f41faa content::RenderFrameHostManager::InitRenderView()
#12 0x7ff7d4f3933d content::RenderFrameHostManager::ReinitializeRenderFrame()
#13 0x7ff7d4f37e53 content::RenderFrameHostManager::Navigate()
#14 0x7ff7d4ee1ad3 content::NavigatorImpl::NavigateToEntry()
#15 0x7ff7d4ee2f68 content::NavigatorImpl::NavigateToPendingEntry()
#16 0x7ff7d4eb487a
content::NavigationControllerImpl::NavigateToPendingEntryInternal()
#17 0x7ff7d4eacf17 content::NavigationControllerImpl::NavigateToPendingEntry()
#18 0x7ff7d4ead365 content::NavigationControllerImpl::LoadEntry()
#19 0x7ff7d4eaef5c content::NavigationControllerImpl::LoadURLWithParams()
#20 0x7ff7e00a88f4 (anonymous namespace)::LoadURLInContents()
#21 0x7ff7e00a75aa chrome::Navigate()
#22 0x7ff7e00dcac7 StartupBrowserCreatorImpl::OpenTabsInBrowser()
#23 0x7ff7e00dd687 StartupBrowserCreatorImpl::RestoreOrCreateBrowser()
#24 0x7ff7e00dc04d
StartupBrowserCreatorImpl::ProcessLaunchUrlsUsingConsolidatedFlow()
#25 0x7ff7e00db4c5 StartupBrowserCreatorImpl::Launch()
#26 0x7ff7e00d5662 StartupBrowserCreator::LaunchBrowser()
#27 0x7ff7e00d4b57 StartupBrowserCreator::ProcessCmdLineImpl()
#28 0x7ff7e00d3ef2 StartupBrowserCreator::Start()
#29 0x7ff7de08f54c ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#30 0x7ff7de08e130 ChromeBrowserMainParts::PreMainMessageLoopRun()
#31 0x7ff7d4b8e831 content::BrowserMainLoop::PreMainMessageLoopRun()
#32 0x7ff7d3feafe5
_ZN4base8internal13FunctorTraitsIMN7content22IndexedDBCallbacksImpl13InternalStateEFvvEvE6InvokeIPS4_JEEEvS6_OT_DpOT0_
#33 0x7ff7d4b99991
_ZN4base8internal12InvokeHelperILb0EiE8MakeItSoIRKMN7content15BrowserMainLoopEFivEJPS5_EEEiOT_DpOT0_
#34 0x7ff7d4b99937
_ZN4base8internal7InvokerINS0_9BindStateIMN7content15BrowserMainLoopEFivEJNS0_17UnretainedWrapperIS4_EEEEEFivEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEiOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#35 0x7ff7d4b9987c
_ZN4base8internal7InvokerINS0_9BindStateIMN7content15BrowserMainLoopEFivEJNS0_17UnretainedWrapperIS4_EEEEEFivEE3RunEPNS0_13BindStateBaseE
#36 0x7ff7d3ecb53b base::internal::RunMixin<>::Run()
#37 0x7ff7d575e9bb content::StartupTaskRunner::RunAllTasksNow()
#38 0x7ff7d4b8c400 content::BrowserMainLoop::CreateStartupTasks()
#39 0x7ff7d4b9d3c7 content::BrowserMainRunnerImpl::Initialize()
#40 0x7ff7d4b886af content::BrowserMain()
#41 0x7ff7d63fcbd6 content::RunNamedProcessTypeMain()
#42 0x7ff7d63fef7c content::ContentMainRunnerImpl::Run()
#43 0x7ff7d63fbb02 content::ContentMain()
#44 0x7ff7dc6d57da ChromeMain
#45 0x7ff7dc6d5702 main
#46 0x7ff7c7a80f45 __libc_start_main
#47 0x7ff7dc6d5605 <unknown>

Original issue's description:
> memory-infra: Finish moving to Mojo (2nd attempt)
>
> This was originally landed in https://codereview.chromium.org/2694083005.
> But, it broke webview perf tests, and so, it was reverted. It turned out that
> in some scenarios, the memory dump manager is not initialized early enough (in
> the browser process). So, now, I initialize it as soon as the UI thread is
> created.
>
> Modifications since the original CL are:
>
> - The Coordinator service is created when the UI thread is created
> (browser_main_loop.cc).
> - When Coordinator is created, it initializes memory dump manager's TPID
> (memory_dump_manager_delegate_impl.cc).
> - To avoid dependency from //services/resource_coordinator to //content the
> definition of the service TPID is moved from child_process_host.h to
> constants.mojom.
>
> I tested the CL by building for Android and running the following with an
> android device attached:
>
> run_benchmark memory.top_10_mobile --browser=android-webview
> --also-run-disabled-tests
>
> In the original reverted CL, the above command results in
> 'Unable to obtain memory dump' exceptions. With this CL the test runs
> successfully.
>
> An example interaction diagram (to see the large image, click on it):
>
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4/edit#heading=h.1ku5wcoxqifr
>
> BUG=679830, 697773, 697384, 697062
>
> Review-Url: https://codereview.chromium.org/2734193002
> Cr-Commit-Position: refs/heads/master@{#455866}
> Committed:
https://chromium.googlesource.com/chromium/src/+/0a3a302b6df6458b21b3ce0951c5251413cadc48

TBR=prim...@chromium.org,ss...@chromium.org,ke...@chromium.org,j...@chromium.org,chinifo...@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=679830, 697773, 697384, 697062

Affected files (+954, -403 lines):
M base/trace_event/memory_dump_manager.h
M base/trace_event/memory_dump_manager.cc
M base/trace_event/memory_dump_manager_unittest.cc
M chrome/browser/BUILD.gn
M chrome/browser/DEPS
M chrome/browser/chrome_content_browser_client.cc
M components/BUILD.gn
M components/tracing/BUILD.gn
A components/tracing/child/child_memory_dump_manager_delegate_impl.h
A components/tracing/child/child_memory_dump_manager_delegate_impl.cc
M components/tracing/child/child_trace_message_filter.h
M components/tracing/child/child_trace_message_filter.cc
A components/tracing/child/child_trace_message_filter_browsertest.cc
M content/browser/BUILD.gn
M content/browser/browser_main.cc
M content/browser/browser_main_loop.cc
M content/browser/gpu/browser_gpu_channel_host_factory.cc
M content/browser/tracing/trace_message_filter.h
M content/browser/tracing/trace_message_filter.cc
M content/browser/tracing/tracing_controller_impl.h
M content/browser/tracing/tracing_controller_impl.cc
M content/child/BUILD.gn
M content/child/DEPS
M content/child/child_thread_impl.cc
M content/common/BUILD.gn
M content/common/DEPS
M content/common/child_process_host_impl.h
M content/common/child_process_host_impl.cc
M content/public/app/mojo/content_browser_manifest.json
M content/public/common/child_process_host.h
M content/test/BUILD.gn
M services/resource_coordinator/BUILD.gn
M services/resource_coordinator/memory/coordinator/coordinator_impl.h
M services/resource_coordinator/memory/coordinator/coordinator_impl.cc
M services/resource_coordinator/memory/coordinator/coordinator_impl_unittest.cc
M services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
M services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
D services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl_unittest.cc
M services/resource_coordinator/public/interfaces/BUILD.gn
D services/resource_coordinator/public/interfaces/memory/constants.mojom

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2738093003/1


https://codereview.chromium.org/2738093003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/034bdcd0c7f1a735dd7f03813a38d1a498c80535

https://codereview.chromium.org/2738093003/

[the...@chromium.org]

Hit this locally as well. I wonder how the CL passed CQ...

https://codereview.chromium.org/2738093003/

[Ehsan Chiniforooshan]

This is very confusing. I noticed that my static local variables get initialized more than once when "is_component_build" is true. I have no idea why, yet.

thestig: about the fact that CL passed CQ: is it by any chance true that CQ bots don't use component build?

On Thu, Mar 9, 2017 at 7:17 PM <the...@chromium.org> wrote:

Hit this locally as well. I wonder how the CL passed CQ...

https://codereview.chromium.org/2738093003/

--
You received this message because you are subscribed to the Google Groups "tracing" group.
To unsubscribe from this group and stop receiving emails from it, send an email to tracing+u...@chromium.org.
To post to this group, send email to tra...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/tracing/f403045c5762d7bc83054a5549a3%40google.com.

[prim...@chromium.org]

On 2017/03/10 16:38:55, chromium-reviews wrote:
> This is very confusing. I noticed that my static local variables get
> initialized more than once when "is_component_build" is true.

Uh, hold on, which ones?

>I have no idea why, yet.

That sounds like an ODR violation. It can happen if you end up deining variables
(or static locals) in headers.

> is it by any chance true that CQ bots don't use component build?

I wouldn't be surprised if the CQ had only component builders but not testers.

https://codereview.chromium.org/2738093003/

[chinifo...@chromium.org]

On 2017/03/10 16:51:19, Primiano Tucci (slow - perf) wrote:
> On 2017/03/10 16:38:55, chromium-reviews wrote:
> > This is very confusing. I noticed that my static local variables get
> > initialized more than once when "is_component_build" is true.
> Uh, hold on, which ones?

The one created in CoordinatorImpl::GetInstance. And, I think also the one in
MemoryDumpManagerDelegateImpl::Create.

> >I have no idea why, yet.
> That sounds like an ODR violation. It can happen if you end up deining
variables
> (or static locals) in headers.

But those are not in headers.

[m...@chromium.org]

Won't this also happen when your static library is linked into different
dynamically-linked modules (ie. foo.dll and bar.dll both include your static
library and thus have different static locals)?

https://codereview.chromium.org/2738093003/

[chinifo...@chromium.org]

Yes. Thanks a lot Mike and Primiano! I think I know how to fix this now.

https://codereview.chromium.org/2738093003/