memory-infra: Finish moving to Mojo (2nd attempt)

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

[chiniforooshan, 2017-03-07 16:52:02]

chiniforooshan@chromium.org changed reviewers:
+ jam@chromium.org, kenrb@chromium.org, primiano@chromium.org, ssid@chromium.org

[chiniforooshan, 2017-03-07 16:52:03]

Please review this CL that has the old CL for ease of comparison. I will delete
2732873002. Thanks!

[kenrb, 2017-03-08 00:52:50]

ipc lgtm

[Primiano Tucci (use gerrit), 2017-03-08 12:05:43]

LGTM with some comments, would feel a bit safer if ssid did a 2nd pass here.
Thanks a lot for checking also the webview case.
In all honesty, would be great to have a 1-pager doc (or a README.md in the new
folder) that explains the lifetime of all these objects and how they interact in
the browser/child cases. Back in the IPC day we had a nice 1.5 page doc [1] that
was picturing all this.

Honestly right now I am getting a bit lost when trying to follow the lifetime
and interaction of MDM <> MDM::Delegate <> Coordinator/Service .
I suspect that once we do we will find out that we can further simplify all
this.

[1]
https://docs.google.com/document/d/1h-rmd2Mh9IMQizKEDLw3z2r8fzoR0ITlQVBN9PFtl...

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:48: void
InitializeForTest(
nit: +"ing", I think the standard pattern is "ForTesting"

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:87:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
why are you changing this? Looks like you are using task_runner_ only to do
thread checks, but that is what thread_checker was supposed to do? If the
problem is just Initialize() can't you just sovle it with
thread_checker_.DetachFromThread() ?

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:44:
base::AutoLock lock(initialized_lock_);
Hmm this is a bit of an odd pattern and honestly smells a bit like an unclear
initialization sequence.
Honestly I don't think here you really need to separate instance creation and
initialization.
I think all this would be more readable and easier to follow if you did
something like this:

- Have a static Method called MemoryDumpManagerImpl::CreateInstance(const
MemoryDumpManagerImpl::Config&) and just forward the config to the real ctor
where MemoryDumpManagerImpl::Config is a simple struct that takes all the arg
required for both cases.

- Use a plain static local which is thread safe in C++11, so you can also get
rid of the LazyInstance above, i.e.
MemoryDumpManagerImpl* MemoryDumpManagerImpl::Create(const
MemoryDumpManagerImpl::Config&) {
   static MemoryDumpManagerImpl* instance = new MemoryDumpManagerImpl(config);
   DCHECK(instance.config == config)
   return instance; 
}
this will make it structurally impossible to initialize the impl twice and will
remove the confusion that arises when you think about "what happens if you
GetInstance() before initialize()?" because now you have only CreateInstance and
no GetInstance (nor initialize) anymore :)

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:96:
// Note: This condition is here to match the old behavior. If the delegate is
Ah, TIL. Not sure this would ever intended. I think once this sticks we might
want to drop this special case and throttle everything (unless I missed some
piece of the discussion and this behavior is really needed) .
Can you just add a todo in this cl as a reminder to simplify this?

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:105:
if (task_runner_) {
I think that IsCoordinator()  here woul make this if statement more readable

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:106:
DCHECK(task_runner_);
uh? how can this fail given the if above?

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h:20:
class MemoryDumpManagerDelegateImpl
would be nice to have some comment here explaining what this class is for, why
do we need this and why this is not the MDMdelegate interface is not just
implemented by the Coordinator

[ssid, 2017-03-08 18:37:13]

Thanks for uploading both patch sets side by side.
I do not quite understand how this solves the issue about the TPID. Can you
please explain why BrowserMainRunner::Initialize() didn't work in previous CL?

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:79:
mojom::kServiceTracingProcessId);
How does this fix the problem of browser process not getting the right TPID in
some cases?
Why do we assign the same TPID for all processes? Shouldn't this be setting
invalid process id instead?

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:96:
// Note: This condition is here to match the old behavior. If the delegate is
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> Ah, TIL. Not sure this would ever intended. I think once this sticks we might
> want to drop this special case and throttle everything (unless I missed some
> piece of the discussion and this behavior is really needed) .
> Can you just add a todo in this cl as a reminder to simplify this?

Some unittests are assuming that the child does ont queue memory dumps. Fixing
them would cause much larger CL. There is already a TODO below to fix the
unittests in next CL.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:105:
if (task_runner_) {
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> I think that IsCoordinator()  here woul make this if statement more readable

Sorry about this. I asked to remove is_coordinator here because i was expecting
there should be no difference between the child and corrdinator's delegates. But
apparently we need the task runner. In this case, I agree with primiano about
having the coordinator back.
Sorry about the back and forth.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File services/resource_coordinator/public/interfaces/memory/constants.mojom
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/constants.mojom:7: const
uint64 kServiceTracingProcessId = 0xffffffffffffffff;
IIUC we should be having 2 constants here, browser tracing id and invalid
tracing id.

[chiniforooshan, 2017-03-08 19:03:43]

Thanks Primiano. I added a brief doc to the beginning of
memory_dump_manager_delegate_impl.h. I'll add an interaction diagram, similar to
what you had in
https://docs.google.com/document/d/1h-rmd2Mh9IMQizKEDLw3z2r8fzoR0ITlQVBN9PFtl...,
to the design doc and link it here when it's ready.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:48: void
InitializeForTest(
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> nit: +"ing", I think the standard pattern is "ForTesting"

Done.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:87:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> why are you changing this? Looks like you are using task_runner_ only to do
> thread checks, but that is what thread_checker was supposed to do? If the
> problem is just Initialize() can't you just sovle it with
> thread_checker_.DetachFromThread() ?

Done.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:44:
base::AutoLock lock(initialized_lock_);
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> Hmm this is a bit of an odd pattern and honestly smells a bit like an unclear
> initialization sequence.
> Honestly I don't think here you really need to separate instance creation and
> initialization.
> I think all this would be more readable and easier to follow if you did
> something like this:
> 
> - Have a static Method called MemoryDumpManagerImpl::CreateInstance(const
> MemoryDumpManagerImpl::Config&) and just forward the config to the real ctor
> where MemoryDumpManagerImpl::Config is a simple struct that takes all the arg
> required for both cases.
> 
> - Use a plain static local which is thread safe in C++11, so you can also get
> rid of the LazyInstance above, i.e.
> MemoryDumpManagerImpl* MemoryDumpManagerImpl::Create(const
> MemoryDumpManagerImpl::Config&) {
>    static MemoryDumpManagerImpl* instance = new MemoryDumpManagerImpl(config);
>    DCHECK(instance.config == config)
>    return instance; 
> }
> this will make it structurally impossible to initialize the impl twice and
will
> remove the confusion that arises when you think about "what happens if you
> GetInstance() before initialize()?" because now you have only CreateInstance
and
> no GetInstance (nor initialize) anymore :)
> 
>  

I learned about static locals today. Thanks!

One reason for this complexity was that when we initialize the delegate from the
coordinator process, we have to initialize it on the UI thread (the same thread
that binds to remote requests). That's why InitializeWithCoordinator does a
PostTask. Of course, we cannot PostTask in the constructor. But, I think since
your suggestion makes the code a lot cleaner, it's reasonable to move that
requirement to whoever calls the Create method; we have DCHECKs for this in the
Coordinator.

Done.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:96:
// Note: This condition is here to match the old behavior. If the delegate is
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> Ah, TIL. Not sure this would ever intended. I think once this sticks we might
> want to drop this special case and throttle everything (unless I missed some
> piece of the discussion and this behavior is really needed) .
> Can you just add a todo in this cl as a reminder to simplify this?

Done.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:105:
if (task_runner_) {
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> I think that IsCoordinator()  here woul make this if statement more readable

Done.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:106:
DCHECK(task_runner_);
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> uh? how can this fail given the if above?

Done.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h:20:
class MemoryDumpManagerDelegateImpl
On 2017/03/08 12:05:43, Primiano Tucci (slow - perf) wrote:
> would be nice to have some comment here explaining what this class is for, why
> do we need this and why this is not the MDMdelegate interface is not just
> implemented by the Coordinator

Done.

[chiniforooshan, 2017-03-08 21:09:41]

Description was changed from

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

BUG=679830, 697773, 697384, 697062
==========

to

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

BUG=679830, 697773, 697384, 697062
==========

[chiniforooshan, 2017-03-08 21:10:55]

Added an example interaction diagram here:

https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_...

Please click on the image to see the full size version.

[chiniforooshan, 2017-03-08 22:39:03]

About the fix: my understanding of

https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?dr=C&...

is that BrowserMain does not run on Android. So,
instead of setting TPID in BrowserMain and creating
Coordinator lazily (first CL), now I create the
Coordinator in content::BrowserMainRunnerImpl::Initialize
(which is always run, I think) and the Coordinator sets
TPID when its created.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:79:
mojom::kServiceTracingProcessId);
On 2017/03/08 18:37:13, ssid wrote:
> How does this fix the problem of browser process not getting the right TPID in
> some cases?
> Why do we assign the same TPID for all processes? Shouldn't this be setting
> invalid process id instead?

Note that InitializeWithCoordinator is only called from the browser process.
Children connect remotely and so call InitializeWithInterfaceProvider. So,
mojom::kServiceTracingProcessId is set only if we are in the browser process
(method names are changed after I applied Primiano's comments. But, the concept
is the same, we call set_tracing_process_id only if we are the coordinator.

For children, the initial value of TPID is set in
//base/trace_event/memory_dump_manager, and that's where
kInvalidTracingProcessId is defined.

I moved set_tracing_process_id here so that it's more clear that we initialize
TPID on the browser when Coordinator is created.

About the fix: My understanding of
https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?dr=C&...
was that BrowserMain does not run on Android. So, instead of setting TPID in
BrowserMain and creating Coordinator lazily (first CL), now I create the
Coordinator in content::BrowserMainRunnerImpl::Initialize (which is always run,
I think) and Coordinator sets TPID when its created.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File services/resource_coordinator/public/interfaces/memory/constants.mojom
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/constants.mojom:7: const
uint64 kServiceTracingProcessId = 0xffffffffffffffff;
On 2017/03/08 18:37:13, ssid wrote:
> IIUC we should be having 2 constants here, browser tracing id and invalid
> tracing id.

The invalid tracing id is defined in //base/trace_event/memory_dump_manager and
is used only there. I think we better leave it there since 1- we don't need it
here, 2- //base cannot depend on //services.

[ssid, 2017-03-09 00:23:16]

lgtm, thanks

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:79:
mojom::kServiceTracingProcessId);
On 2017/03/08 22:39:03, chiniforooshan wrote:
> On 2017/03/08 18:37:13, ssid wrote:
> > How does this fix the problem of browser process not getting the right TPID
in
> > some cases?
> > Why do we assign the same TPID for all processes? Shouldn't this be setting
> > invalid process id instead?
> 
> Note that InitializeWithCoordinator is only called from the browser process.
> Children connect remotely and so call InitializeWithInterfaceProvider. So,
> mojom::kServiceTracingProcessId is set only if we are in the browser process
> (method names are changed after I applied Primiano's comments. But, the
concept
> is the same, we call set_tracing_process_id only if we are the coordinator.
> 
> For children, the initial value of TPID is set in
> //base/trace_event/memory_dump_manager, and that's where
> kInvalidTracingProcessId is defined.
> 
> I moved set_tracing_process_id here so that it's more clear that we initialize
> TPID on the browser when Coordinator is created.
> 
> About the fix: My understanding of
>
https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?dr=C&...
> was that BrowserMain does not run on Android. So, instead of setting TPID in
> BrowserMain and creating Coordinator lazily (first CL), now I create the
> Coordinator in content::BrowserMainRunnerImpl::Initialize (which is always
run,
> I think) and Coordinator sets TPID when its created.

Ah sorry. I thought they both call the same function, got mixed up with the
names.

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
File services/resource_coordinator/public/interfaces/memory/constants.mojom
(right):

https://codereview.chromium.org/2734193002/diff/40001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/constants.mojom:7: const
uint64 kServiceTracingProcessId = 0xffffffffffffffff;
On 2017/03/08 22:39:03, chiniforooshan wrote:
> On 2017/03/08 18:37:13, ssid wrote:
> > IIUC we should be having 2 constants here, browser tracing id and invalid
> > tracing id.
> 
> The invalid tracing id is defined in //base/trace_event/memory_dump_manager
and
> is used only there. I think we better leave it there since 1- we don't need it
> here, 2- //base cannot depend on //services.

Makes sense to keep it there. Thanks

[jam, 2017-03-09 16:40:48]

lgtm

[chiniforooshan, 2017-03-09 17:36:23]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-03-09 17:36:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, kenrb@chromium.org, jam@chromium.org, ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2734193002/#ps120001
(title: "nit")

[commit-bot: I haz the power, 2017-03-09 17:36:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 19:06:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 19:06:09]

Failed to apply patch for
services/resource_coordinator/public/interfaces/BUILD.gn:
While running git apply --index -p1;
  error: patch failed:
services/resource_coordinator/public/interfaces/BUILD.gn:6
  error: services/resource_coordinator/public/interfaces/BUILD.gn: patch does
not apply

Patch:       services/resource_coordinator/public/interfaces/BUILD.gn
Index: services/resource_coordinator/public/interfaces/BUILD.gn
diff --git a/services/resource_coordinator/public/interfaces/BUILD.gn
b/services/resource_coordinator/public/interfaces/BUILD.gn
index
959f3ecdee5e52c8505029f19b670d46afd19e90..725a8eceee6f9be2bd0c043f379630d533de0bba
100644
--- a/services/resource_coordinator/public/interfaces/BUILD.gn
+++ b/services/resource_coordinator/public/interfaces/BUILD.gn
@@ -6,6 +6,7 @@ import("//mojo/public/tools/bindings/mojom.gni")
 
 mojom("interfaces") {
   sources = [
+    "memory/constants.mojom",
     "memory/memory_instrumentation.mojom",
   ]
 }

[chiniforooshan, 2017-03-09 19:25:02]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-03-09 19:25:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, kenrb@chromium.org, jam@chromium.org, ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2734193002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-09 19:25:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 19:45:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 19:45:18]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[chiniforooshan, 2017-03-09 20:33:58]

The CQ bit was checked by chiniforooshan@chromium.org

[commit-bot: I haz the power, 2017-03-09 20:34:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 21:20:13]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489091638383520,
"parent_rev": "25402621d78f7cad11d719a55f6e68ca81678108", "commit_rev":
"0a3a302b6df6458b21b3ce0951c5251413cadc48"}

[commit-bot: I haz the power, 2017-03-09 21:20:54]

Description was changed from

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

BUG=679830, 697773, 697384, 697062
==========

to

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

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/+/0a3a302b6df6458b21b3ce0951c5...
==========

[commit-bot: I haz the power, 2017-03-09 21:20:56]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0a3a302b6df6458b21b3ce0951c5...

[msw, 2017-03-09 23:30:39]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2738093003/ by msw@chromium.org.

The reason for reverting is: Prospective revert for nacl_integration crash
starting on this build:
 
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...

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

[Geoff Lang, 2017-03-09 23:32:54]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2741723003/ by geofflang@chromium.org.

The reason for reverting is: Causing assertion failures on the GPU FYI
waterfall:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20Debug%20%28Intel....