memory-infra: Finish moving memory_infra from TracingController

memory-infra: Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2694083005
Cr-Commit-Position: refs/heads/master@{#453243}
Committed: https://chromium.googlesource.com/chromium/src/+/e7bee2f02da7fb7afbdb387197ad9461d8d2eba2

[chiniforooshan, 2017-02-14 18:29:19]

Description was changed from

==========
WIP: Finish moving from TracingController

This finishes moving memory instrumentation code from
TracingController to its own location in //services.

BUG=679830
==========

to

==========
WIP: Finish moving from TracingController

This finishes moving memory instrumentation code from
TracingController to its own location in //services.

BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[chiniforooshan, 2017-02-14 18:29:20]

Description was changed from

==========
WIP: Finish moving from TracingController

This finishes moving memory instrumentation code from
TracingController to its own location in //services.

BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
WIP: Finish moving from TracingController

This finishes moving memory instrumentation code from
TracingController to its own location in //services.

BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chiniforooshan, 2017-02-14 18:35:27]

Patchset #2 (id:20001) has been deleted

[chiniforooshan, 2017-02-14 21:21:08]

Description was changed from

==========
WIP: Finish moving from TracingController

This finishes moving memory instrumentation code from
TracingController to its own location in //services.

BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Finish moving from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
The patch is relative to 2693063002 and should be submitted after it.

BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chiniforooshan, 2017-02-14 21:21:17]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 21:21:58]

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

[chiniforooshan, 2017-02-14 21:51:47]

Description was changed from

==========
Finish moving from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
The patch is relative to 2693063002 and should be submitted after it.

BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Finish moving from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
==========

[chiniforooshan, 2017-02-14 21:52:25]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 21:52:55]

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

[chiniforooshan, 2017-02-14 21:53:54]

chiniforooshan@chromium.org changed reviewers:
+ oysteine@chromium.org, primiano@chromium.org

[chiniforooshan, 2017-02-14 21:53:54]

PTAL

[chiniforooshan, 2017-02-14 21:54:58]

Description was changed from

==========
Finish moving from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
==========

to

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
==========

[chiniforooshan, 2017-02-14 22:04:13]

Description was changed from

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
==========

to

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[chiniforooshan, 2017-02-14 22:04:14]

Description was changed from

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chiniforooshan, 2017-02-14 22:06:47]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 22:07:51]

Dry run: 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-02-14 23:00:28]

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

[commit-bot: I haz the power, 2017-02-14 23:00:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chiniforooshan, 2017-02-15 00:58:46]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 00:59:51]

Dry run: 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-02-15 02:04:30]

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

[commit-bot: I haz the power, 2017-02-15 02:04:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[chiniforooshan, 2017-02-15 06:23:09]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 06:24:00]

Dry run: 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-02-15 07:00:17]

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

[commit-bot: I haz the power, 2017-02-15 07:00:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chiniforooshan, 2017-02-15 17:08:37]

Description was changed from

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chiniforooshan, 2017-02-15 17:08:37]

chiniforooshan@chromium.org changed reviewers:
- oysteine@chromium.org, primiano@chromium.org

[chiniforooshan, 2017-02-15 17:08:47]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 17:09:27]

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

[chiniforooshan, 2017-02-15 17:59:44]

Description was changed from

==========
Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
memory-infra: Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-02-15 18:14:02]

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

[commit-bot: I haz the power, 2017-02-15 18:14:04]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chiniforooshan, 2017-02-16 07:00:56]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 07:02:30]

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

[chiniforooshan, 2017-02-16 07:06:37]

The CQ bit was unchecked by chiniforooshan@chromium.org

[chiniforooshan, 2017-02-16 07:30:42]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 07:31:39]

Dry run: 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-02-16 10:36:08]

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

[commit-bot: I haz the power, 2017-02-16 10:36:09]

Dry run: This issue passed the CQ dry run.

[chiniforooshan, 2017-02-16 14:08:28]

chiniforooshan@chromium.org changed reviewers:
+ oysteine@chromium.org, primiano@chromium.org

[chiniforooshan, 2017-02-16 14:08:30]

OK, tests pass know. PTAL :)

[oystein (OOO til 10th of July), 2017-02-16 20:50:01]

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:132: uint64_t tracing_process_id() const;
You can just inline these two.

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:227: delegate_ =
delegate_for_mdm_.get();
This is confusing to me, why are you storing the raw ptr of the delegate as well
as the unique_ptr?

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:256:
delegate_for_mdm_->SetIsCoordinator(is_coordinator);
Can you just create the MemoryDumpManagerDelegateForTesting here, instead of
creating it in the destructor with is_coordinator = false and thus needing an
API for changing it?

https://codereview.chromium.org/2694083005/diff/260001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3011: ui_task_runner);
Does this need to be running on the UI thread?

https://codereview.chromium.org/2694083005/diff/260001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/content/child/child_th...
content/child/child_thread_impl.cc:510:
std::unique_ptr<memory_instrumentation::MemoryDumpManagerDelegateImpl>
auto delegate =
base::MakeUnique<MemoryDumpManagerDelegateImpl>(GetRemoteInterfaces());

https://codereview.chromium.org/2694083005/diff/260001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/260001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:64:
std::unique_ptr<MemoryDumpManagerDelegateImpl> delegate(
auto delegate = base::MakeUnique<blah>(...)

[chiniforooshan, 2017-02-16 22:54:15]

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:132: uint64_t tracing_process_id() const;
On 2017/02/16 20:50:00, oystein wrote:
> You can just inline these two.

Done.

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:227: delegate_ =
delegate_for_mdm_.get();
On 2017/02/16 20:50:00, oystein wrote:
> This is confusing to me, why are you storing the raw ptr of the delegate as
well
> as the unique_ptr?

It is because MDM assumes that the ownership of the delegate will be transferred
to it, which is OK because in the non-test scenarios only MDM uses the delegate
and it's OK to create a delegate and just pass it to MDM and forget about it.
But, in unittests we need the delegate outside MDM to verify its methods are
called correctly (i.e. EXPECT_CALLs) before and after MDM initialization.

I cannot just store a unique_ptr because after MDM initialization I'll loose
access to it. And, if I just store a raw pointer I have to take care of deleting
it if a a test case decides to not transfer ownership to MDM (i.e. doesn't call
mdm->Initialize). I know it's confusing, but is there a better way?

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:256:
delegate_for_mdm_->SetIsCoordinator(is_coordinator);
On 2017/02/16 20:50:00, oystein wrote:
> Can you just create the MemoryDumpManagerDelegateForTesting here, instead of
> creating it in the destructor with is_coordinator = false and thus needing an
> API for changing it?

No, because of the test case "InitializedAfterStartOfTracing". It does not call
InitializeMemoryDumpManager but uses the delegate:

EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(0);

https://codereview.chromium.org/2694083005/diff/260001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3011: ui_task_runner);
On 2017/02/16 20:50:00, oystein wrote:
> Does this need to be running on the UI thread?

Coordinator is not thread safe and all calls should be made on the same thread.
Also, the old cold always runs on the UI thread (e.g.
//content/browser/tracing/tracing_controller_impl.cc:899) and I didn't want to
change that behaviour.

I remember primiano@ or ssid@ mentioned a reason for running on the UI thread at
some point but I don't remember what it was :)

https://codereview.chromium.org/2694083005/diff/260001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/content/child/child_th...
content/child/child_thread_impl.cc:510:
std::unique_ptr<memory_instrumentation::MemoryDumpManagerDelegateImpl>
On 2017/02/16 20:50:01, oystein wrote:
> auto delegate =
> base::MakeUnique<MemoryDumpManagerDelegateImpl>(GetRemoteInterfaces());

Done.

https://codereview.chromium.org/2694083005/diff/260001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/260001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:64:
std::unique_ptr<MemoryDumpManagerDelegateImpl> delegate(
On 2017/02/16 20:50:01, oystein wrote:
> auto delegate = base::MakeUnique<blah>(...)

Done.

[oystein (OOO til 10th of July), 2017-02-16 23:19:49]

lgtm w/ comments but wait for primiano to look at the memory_infra specific
bits.

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:227: delegate_ =
delegate_for_mdm_.get();
On 2017/02/16 at 22:54:15, chiniforooshan wrote:
> On 2017/02/16 20:50:00, oystein wrote:
> > This is confusing to me, why are you storing the raw ptr of the delegate as
well
> > as the unique_ptr?
> 
> It is because MDM assumes that the ownership of the delegate will be
transferred to it, which is OK because in the non-test scenarios only MDM uses
the delegate and it's OK to create a delegate and just pass it to MDM and forget
about it. But, in unittests we need the delegate outside MDM to verify its
methods are called correctly (i.e. EXPECT_CALLs) before and after MDM
initialization.
> 
> I cannot just store a unique_ptr because after MDM initialization I'll loose
access to it. And, if I just store a raw pointer I have to take care of deleting
it if a a test case decides to not transfer ownership to MDM (i.e. doesn't call
mdm->Initialize). I know it's confusing, but is there a better way?

Can the MDM have a get_delegate_for_testing() which returns the raw ptr?

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:256:
delegate_for_mdm_->SetIsCoordinator(is_coordinator);
On 2017/02/16 at 22:54:15, chiniforooshan wrote:
> On 2017/02/16 20:50:00, oystein wrote:
> > Can you just create the MemoryDumpManagerDelegateForTesting here, instead of
> > creating it in the destructor with is_coordinator = false and thus needing
an
> > API for changing it?
> 
> No, because of the test case "InitializedAfterStartOfTracing". It does not
call InitializeMemoryDumpManager but uses the delegate:
> 
> EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(0);

Okay, though that line seems to be testing the test itself :). i.e. the
delegate_ hasn't gotten passed anywhere at this point, so it's effectively
testing that InitializeMemoryDumpManager() didn't get called by the test.

https://codereview.chromium.org/2694083005/diff/260001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3011: ui_task_runner);
On 2017/02/16 at 22:54:15, chiniforooshan wrote:
> On 2017/02/16 20:50:00, oystein wrote:
> > Does this need to be running on the UI thread?
> 
> Coordinator is not thread safe and all calls should be made on the same
thread. Also, the old cold always runs on the UI thread (e.g.
//content/browser/tracing/tracing_controller_impl.cc:899) and I didn't want to
change that behaviour.
> 
> I remember primiano@ or ssid@ mentioned a reason for running on the UI thread
at some point but I don't remember what it was :)

Acknowledged, not changing existing behavior makes sense.

[Primiano Tucci (use gerrit), 2017-02-17 16:22:44]

primiano@chromium.org changed reviewers:
+ hjd@chromium.org

[Primiano Tucci (use gerrit), 2017-02-17 16:22:46]

I'll get to this very soon (O(Hours)) sorry for the delay, bad week.
in the meantime, +hjd, who is ramping up on memory-infra!

[chiniforooshan, 2017-02-17 17:20:42]

Thanks.

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:227: delegate_ =
delegate_for_mdm_.get();
On 2017/02/16 23:19:49, oystein wrote:
> On 2017/02/16 at 22:54:15, chiniforooshan wrote:
> > On 2017/02/16 20:50:00, oystein wrote:
> > > This is confusing to me, why are you storing the raw ptr of the delegate
as
> well
> > > as the unique_ptr?
> > 
> > It is because MDM assumes that the ownership of the delegate will be
> transferred to it, which is OK because in the non-test scenarios only MDM uses
> the delegate and it's OK to create a delegate and just pass it to MDM and
forget
> about it. But, in unittests we need the delegate outside MDM to verify its
> methods are called correctly (i.e. EXPECT_CALLs) before and after MDM
> initialization.
> > 
> > I cannot just store a unique_ptr because after MDM initialization I'll loose
> access to it. And, if I just store a raw pointer I have to take care of
deleting
> it if a a test case decides to not transfer ownership to MDM (i.e. doesn't
call
> mdm->Initialize). I know it's confusing, but is there a better way?
> 
> Can the MDM have a get_delegate_for_testing() which returns the raw ptr?

How? That function would return a MemoryDumpManagerDelegate and we need the
MemoryDumpManagerDelegateForTesting subclass in tests and I "cannot use
dynamic_cast with -fno-rtti".

https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:256:
delegate_for_mdm_->SetIsCoordinator(is_coordinator);
On 2017/02/16 23:19:49, oystein wrote:
> On 2017/02/16 at 22:54:15, chiniforooshan wrote:
> > On 2017/02/16 20:50:00, oystein wrote:
> > > Can you just create the MemoryDumpManagerDelegateForTesting here, instead
of
> > > creating it in the destructor with is_coordinator = false and thus needing
> an
> > > API for changing it?
> > 
> > No, because of the test case "InitializedAfterStartOfTracing". It does not
> call InitializeMemoryDumpManager but uses the delegate:
> > 
> > EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(0);
> 
> Okay, though that line seems to be testing the test itself :). i.e. the
> delegate_ hasn't gotten passed anywhere at this point, so it's effectively
> testing that InitializeMemoryDumpManager() didn't get called by the test.

Agreed. Let me delete that EXPECT_CALL unless Primiano objects.

[Primiano Tucci (use gerrit), 2017-02-17 19:08:44]

LGTM with some comments, plz look at them before landing.
My only concern is that some test coverage seems to be  disappearing.
I don't care about the complex test that check the general interaction at the
IPC level, but I'd like the throttling and queueing mechanism to still be
covered, as that might regress without noticing and have very subtle side
effects on telemetry.

Also +ssid as he also has some patches

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:219: delegate_.swap(delegate);
As commented above I think this doesn't need to be  a unique_ptr, but in the
case it does, AFAIK the pattern here is delegate_ = std::move(delegate)

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
note this cl will very likely need to be rebased on top of ssid's
https://codereview.chromium.org/2582453002/

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:60: void
Initialize(std::unique_ptr<MemoryDumpManagerDelegate> delegate);
the MDM is a leaky singleton, passing ownership of an object to a leaky
singleton is IMHO a bit an obscure way to make the original object singleton in
the first place. can you just make the delegate leaky singleton?

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:135: uint64_t tracing_process_id() const
{ return tracing_process_id_; }
Renaming this getter will require you a lot more owner dances and increase the
likeliness of merge conflicts along the road.
I'd honestly keep this called GetProcessId(), even if becomes an inliner getter,
and eventually renaming in a separate CL.

https://codereview.chromium.org/2694083005/diff/300001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3200: 
just checking: will all this work still fine in the case of webview, which is
--single-process and has browser and renderer in the same process?

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
File components/tracing/child/child_trace_message_filter.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
components/tracing/child/child_trace_message_filter.cc:36: #if
!defined(OS_LINUX) && !defined(OS_NACL)
don't we have an equivalent notification in the new service thingy that says
when a client connects to our service ? (not a huge deal, I'm perfectly fine if
this one stays here for the moment, mostly curious)

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
File components/tracing/child/child_trace_message_filter_browsertest.cc (left):

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
components/tracing/child/child_trace_message_filter_browsertest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
it's ok to lose this coverage I guess. these days tracing is dependen on by so
many places (e.g. telemetry) that everything will go read if we break it :)
I guess the only thing I still care to keep tested is the throttling logic of
the memmory dumps. Is this test going to be moved somewhere else?

https://codereview.chromium.org/2694083005/diff/300001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/300001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:43: void
CoordinatorImpl::Initialize(base::SingleThreadTaskRunner* task_runner) {
Hmm you should't keep pointers to the TaskRunner themselves as they are proxy
objects and can be short lived. Instead you should have your own
scoped_refptr<STTR>.
That wil NOT keep the taskrunner alive, but will give you a long lived proxy
(and PostTask will fail if the taskrunner goes away)

[chiniforooshan, 2017-02-22 05:16:34]

Thanks.

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:219: delegate_.swap(delegate);
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> As commented above I think this doesn't need to be  a unique_ptr, but in the
> case it does, AFAIK the pattern here is delegate_ = std::move(delegate)

Done.

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> note this cl will very likely need to be rebased on top of ssid's
> https://codereview.chromium.org/2582453002/

Acknowledged.

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:60: void
Initialize(std::unique_ptr<MemoryDumpManagerDelegate> delegate);
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> the MDM is a leaky singleton, passing ownership of an object to a leaky
> singleton is IMHO a bit an obscure way to make the original object singleton
in
> the first place. can you just make the delegate leaky singleton?

Done.

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:135: uint64_t tracing_process_id() const
{ return tracing_process_id_; }
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> Renaming this getter will require you a lot more owner dances and increase the
> likeliness of merge conflicts along the road.
> I'd honestly keep this called GetProcessId(), even if becomes an inliner
getter,
> and eventually renaming in a separate CL.

Done.

https://codereview.chromium.org/2694083005/diff/300001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3200: 
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> just checking: will all this work still fine in the case of webview, which is
> --single-process and has browser and renderer in the same process?

It should work. In content/child/child_thread_impl.cc, it connects to the memory
instrumentation service only if the child is not in the browser process.

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
File components/tracing/child/child_trace_message_filter.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
components/tracing/child/child_trace_message_filter.cc:36: #if
!defined(OS_LINUX) && !defined(OS_NACL)
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> don't we have an equivalent notification in the new service thingy that says
> when a client connects to our service ? (not a huge deal, I'm perfectly fine
if
> this one stays here for the moment, mostly curious)

I was thinking it might be better to not leak concepts of child/browser process
to the code under //services/resource_coordinator. Here we want to do this only
on child processes (when not sandboxed). The service thingy knows when a
ProcessLocalDumpManager connects, but it doesn't know whether the
ProcessLocalDumpManager is for a child process or for the browser process unless
it assumes that the browser process == the process that runs the service.

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
File components/tracing/child/child_trace_message_filter_browsertest.cc (left):

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
components/tracing/child/child_trace_message_filter_browsertest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> it's ok to lose this coverage I guess. these days tracing is dependen on by so
> many places (e.g. telemetry) that everything will go read if we break it :)
> I guess the only thing I still care to keep tested is the throttling logic of
> the memmory dumps. Is this test going to be moved somewhere else?

Added throttling to the delegate and tests for it.

https://codereview.chromium.org/2694083005/diff/300001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/300001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:43: void
CoordinatorImpl::Initialize(base::SingleThreadTaskRunner* task_runner) {
On 2017/02/17 19:08:44, Primiano Tucci wrote:
> Hmm you should't keep pointers to the TaskRunner themselves as they are proxy
> objects and can be short lived. Instead you should have your own
> scoped_refptr<STTR>.
> That wil NOT keep the taskrunner alive, but will give you a long lived proxy
> (and PostTask will fail if the taskrunner goes away)

Done.

[chiniforooshan, 2017-02-22 05:17:37]

chiniforooshan@chromium.org changed reviewers:
+ jochen@chromium.org, kenrb@chromium.org

[chiniforooshan, 2017-02-22 05:17:38]

+kenrb +jochen

[jochen (gone - plz use gerrit), 2017-02-22 09:03:50]

which files do you want me to look at?

[chiniforooshan, 2017-02-22 14:34:45]

On 2017/02/22 09:03:50, jochen wrote:
> which files do you want me to look at?

Could you please take a look at the following (very few lines changed):

- content/child/child_thread_impl.cc
- content/browser/browser_main.cc
- content/child/BUILD.gn]
- content/child/DEPS]
- chrome/browser/DEPS
- components/BUILD.gn

And, Ken, could you please do the security review for
content/public/app/mojo/content_browser_manifest.json?

Thanks a lot!

[chiniforooshan, 2017-02-22 19:02:11]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

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

Dry run: 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-02-22 19:43:12]

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

[commit-bot: I haz the power, 2017-02-22 19:43:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  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-02-22 20:25:00]

On 2017/02/22 14:34:45, chiniforooshan wrote:
> On 2017/02/22 09:03:50, jochen wrote:
> > which files do you want me to look at?
> 
> Could you please take a look at the following (very few lines changed):
> 
> - content/child/child_thread_impl.cc
> - content/browser/browser_main.cc
> - content/child/BUILD.gn]
> - content/child/DEPS]
> - chrome/browser/DEPS
> - components/BUILD.gn
> 
> And, Ken, could you please do the security review for
> content/public/app/mojo/content_browser_manifest.json?
> 
> Thanks a lot!

Looks like I messed up some browser tests. I'll ping this patch again when I fix
them.

[chiniforooshan, 2017-02-22 22:09:20]

chiniforooshan@chromium.org changed reviewers:
- jochen@chromium.org, kenrb@chromium.org

[chiniforooshan, 2017-02-22 22:12:38]

On 2017/02/22 20:25:00, chiniforooshan wrote:
> On 2017/02/22 14:34:45, chiniforooshan wrote:
> > On 2017/02/22 09:03:50, jochen wrote:
> > > which files do you want me to look at?
> > 
> > Could you please take a look at the following (very few lines changed):
> > 
> > - content/child/child_thread_impl.cc
> > - content/browser/browser_main.cc
> > - content/child/BUILD.gn]
> > - content/child/DEPS]
> > - chrome/browser/DEPS
> > - components/BUILD.gn
> > 
> > And, Ken, could you please do the security review for
> > content/public/app/mojo/content_browser_manifest.json?
> > 
> > Thanks a lot!
> 
> Looks like I messed up some browser tests. I'll ping this patch again when I
fix
> them.

-jochen@ -kenrb@ for now.

primiano@, ssid@: I think before this patch global memory dump requests would
NOT be throttled in the single process mode because the throttling logic was
implemented in the ChildTraceMessageFilter which would not be used in the single
process mode. Was that intended? I think that's why
SingleProcessMemoryTracingTest.QueuedDumps test was working before this patch
and is failing by the patch.

[ssid, 2017-02-23 20:21:47]

ssid@chromium.org changed reviewers:
+ ssid@chromium.org

[ssid, 2017-02-23 20:21:50]

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:98:
callback.Run(args.dump_guid, false);
Sorry if I am mis-remembering something. But, how does this work in the browser
process? Currently we trigger periodic dumps in browser process and if we get
explicit triggers when the dump is in progress then we queue the memory dump.
But now with this change we would not queue the requests in browser process,
which is not expected. Right? We want to continue queuing requests in browser.

What I think should be done here is we get rid of the logic about the pending
memory dump guid and always queue the memory dumps from all process. Unless we
check for is_coordinator_ here and do it differently for each process to keep it
consistent with the current state where the queueing is not done in child and
done in browser.

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
(right):

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h:56:
void OnMemoryDump(const base::trace_event::MemoryDumpCallback& callback,
Not sure if this was already discussed. Could you please re-name this method to
MemoryDumpCallbackProxy or MemoryDumpCallbackWrapper, or similar. OnMemoryDump
is the interface method for MemoryDumpProvider. It is confusing to have the same
name here.

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl_unittest.cc:79:
void OnMemoryDump(int more_requests, uint64_t dump_guid, bool success) {
OnGlobalMemoryDumpDone?
OnMemoryDump is overloaded name

[ssid, 2017-02-23 20:25:22]

On 2017/02/22 22:12:38, chiniforooshan wrote:
> On 2017/02/22 20:25:00, chiniforooshan wrote:
> > On 2017/02/22 14:34:45, chiniforooshan wrote:
> > > On 2017/02/22 09:03:50, jochen wrote:
> > > > which files do you want me to look at?
> > > 
> > > Could you please take a look at the following (very few lines changed):
> > > 
> > > - content/child/child_thread_impl.cc
> > > - content/browser/browser_main.cc
> > > - content/child/BUILD.gn]
> > > - content/child/DEPS]
> > > - chrome/browser/DEPS
> > > - components/BUILD.gn
> > > 
> > > And, Ken, could you please do the security review for
> > > content/public/app/mojo/content_browser_manifest.json?
> > > 
> > > Thanks a lot!
> > 
> > Looks like I messed up some browser tests. I'll ping this patch again when I
> fix
> > them.
> 
> -jochen@ -kenrb@ for now.
> 
> primiano@, ssid@: I think before this patch global memory dump requests would
> NOT be throttled in the single process mode because the throttling logic was
> implemented in the ChildTraceMessageFilter which would not be used in the
single
> process mode. Was that intended? I think that's why
> SingleProcessMemoryTracingTest.QueuedDumps test was working before this patch
> and is failing by the patch.

It looks like we should get rid of the logic which drops requests from renderer.
I think this is a bug when we moved from "ignore multiple dumps in all process"
-> "queue memory dumps in browser". At this point we ignored the fact that the
renderer drops and not queues request. But, this could be because we do not want
to send extra IPC messages. But, I don't think we should be ignoring requests
from renderer and not browser. So, lets just do queuing consistently in all
processes. Does it make sense?

[chiniforooshan, 2017-02-23 21:14:40]

On 2017/02/23 20:25:22, ssid wrote:
> On 2017/02/22 22:12:38, chiniforooshan wrote:
> > On 2017/02/22 20:25:00, chiniforooshan wrote:
> > > On 2017/02/22 14:34:45, chiniforooshan wrote:
> > > > On 2017/02/22 09:03:50, jochen wrote:
> > > > > which files do you want me to look at?
> > > > 
> > > > Could you please take a look at the following (very few lines changed):
> > > > 
> > > > - content/child/child_thread_impl.cc
> > > > - content/browser/browser_main.cc
> > > > - content/child/BUILD.gn]
> > > > - content/child/DEPS]
> > > > - chrome/browser/DEPS
> > > > - components/BUILD.gn
> > > > 
> > > > And, Ken, could you please do the security review for
> > > > content/public/app/mojo/content_browser_manifest.json?
> > > > 
> > > > Thanks a lot!
> > > 
> > > Looks like I messed up some browser tests. I'll ping this patch again when
I
> > fix
> > > them.
> > 
> > -jochen@ -kenrb@ for now.
> > 
> > primiano@, ssid@: I think before this patch global memory dump requests
would
> > NOT be throttled in the single process mode because the throttling logic was
> > implemented in the ChildTraceMessageFilter which would not be used in the
> single
> > process mode. Was that intended? I think that's why
> > SingleProcessMemoryTracingTest.QueuedDumps test was working before this
patch
> > and is failing by the patch.
> 
> It looks like we should get rid of the logic which drops requests from
renderer.
> I think this is a bug when we moved from "ignore multiple dumps in all
process"
> -> "queue memory dumps in browser". At this point we ignored the fact that the
> renderer drops and not queues request. But, this could be because we do not
want
> to send extra IPC messages. But, I don't think we should be ignoring requests
> from renderer and not browser. So, lets just do queuing consistently in all
> processes. Does it make sense?

It makes sense to me. But, that would be a fairly large change on top of an
already large CL. Is it OK if I match the current behaviour, i.e. drop parallel
requests from children but not from the browser, and then fix it to queue
client-side, in all processes, in a follow up CL?

[ssid, 2017-02-24 02:17:01]

On 2017/02/23 21:14:40, chiniforooshan wrote:
> On 2017/02/23 20:25:22, ssid wrote:
> > On 2017/02/22 22:12:38, chiniforooshan wrote:
> > > On 2017/02/22 20:25:00, chiniforooshan wrote:
> > > > On 2017/02/22 14:34:45, chiniforooshan wrote:
> > > > > On 2017/02/22 09:03:50, jochen wrote:
> > > > > > which files do you want me to look at?
> > > > > 
> > > > > Could you please take a look at the following (very few lines
changed):
> > > > > 
> > > > > - content/child/child_thread_impl.cc
> > > > > - content/browser/browser_main.cc
> > > > > - content/child/BUILD.gn]
> > > > > - content/child/DEPS]
> > > > > - chrome/browser/DEPS
> > > > > - components/BUILD.gn
> > > > > 
> > > > > And, Ken, could you please do the security review for
> > > > > content/public/app/mojo/content_browser_manifest.json?
> > > > > 
> > > > > Thanks a lot!
> > > > 
> > > > Looks like I messed up some browser tests. I'll ping this patch again
when
> I
> > > fix
> > > > them.
> > > 
> > > -jochen@ -kenrb@ for now.
> > > 
> > > primiano@, ssid@: I think before this patch global memory dump requests
> would
> > > NOT be throttled in the single process mode because the throttling logic
was
> > > implemented in the ChildTraceMessageFilter which would not be used in the
> > single
> > > process mode. Was that intended? I think that's why
> > > SingleProcessMemoryTracingTest.QueuedDumps test was working before this
> patch
> > > and is failing by the patch.
> > 
> > It looks like we should get rid of the logic which drops requests from
> renderer.
> > I think this is a bug when we moved from "ignore multiple dumps in all
> process"
> > -> "queue memory dumps in browser". At this point we ignored the fact that
the
> > renderer drops and not queues request. But, this could be because we do not
> want
> > to send extra IPC messages. But, I don't think we should be ignoring
requests
> > from renderer and not browser. So, lets just do queuing consistently in all
> > processes. Does it make sense?
> 
> It makes sense to me. But, that would be a fairly large change on top of an
> already large CL. Is it OK if I match the current behaviour, i.e. drop
parallel
> requests from children but not from the browser, and then fix it to queue
> client-side, in all processes, in a follow up CL?

SGTM. Lets keep the existing behavior using is_coordinator_ flag for now. Making
this consistent and fixing the tests accordingly can be next CL.

[chiniforooshan, 2017-02-24 15:51:03]

Thanks!

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:98:
callback.Run(args.dump_guid, false);
On 2017/02/23 20:21:50, ssid wrote:
> Sorry if I am mis-remembering something. But, how does this work in the
browser
> process? Currently we trigger periodic dumps in browser process and if we get
> explicit triggers when the dump is in progress then we queue the memory dump.
> But now with this change we would not queue the requests in browser process,
> which is not expected. Right? We want to continue queuing requests in browser.
> 
> What I think should be done here is we get rid of the logic about the pending
> memory dump guid and always queue the memory dumps from all process. Unless we
> check for is_coordinator_ here and do it differently for each process to keep
it
> consistent with the current state where the queueing is not done in child and
> done in browser.

As discussed, I'll change this to match the current behaviour, that is not
dropping parallel requests for the browser process and dropping them for child
processes. Parallel requests from the browser will be queued by the coordinator
service.

I will fix things to queue requests in every processes in a follow up CL. A TODO
is added.

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
(right):

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h:56:
void OnMemoryDump(const base::trace_event::MemoryDumpCallback& callback,
On 2017/02/23 20:21:50, ssid wrote:
> Not sure if this was already discussed. Could you please re-name this method
to
> MemoryDumpCallbackProxy or MemoryDumpCallbackWrapper, or similar. OnMemoryDump
> is the interface method for MemoryDumpProvider. It is confusing to have the
same
> name here.

Done.

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2694083005/diff/420001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl_unittest.cc:79:
void OnMemoryDump(int more_requests, uint64_t dump_guid, bool success) {
On 2017/02/23 20:21:50, ssid wrote:
> OnGlobalMemoryDumpDone?
> OnMemoryDump is overloaded name

Done.

[chiniforooshan, 2017-02-24 15:52:24]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 15:52:38]

Dry run: 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-02-24 17:10:38]

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

[commit-bot: I haz the power, 2017-02-24 17:10:40]

Dry run: This issue passed the CQ dry run.

[chiniforooshan, 2017-02-24 18:00:05]

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

[chiniforooshan, 2017-02-24 18:00:07]

Adding jam@ and kenrb@, now that tests pass.

jam@ could you please review:

- content/child/child_thread_impl.cc
- content/browser/browser_main.cc
- content/child/BUILD.gn
- content/child/DEPS
- chrome/browser/DEPS
- components/BUILD.gn

kenrb@ could you please review:
- content/public/app/mojo/content_browser_manifest.json

Thanks a lot!

[kenrb, 2017-02-24 21:31:48]

lgtm

[ssid, 2017-02-24 23:19:05]

Thanks for the TODO. Just have few questions about having task_runner and
is_coordinator_ in the delegate.

https://codereview.chromium.org/2694083005/diff/440001/content/browser/tracin...
File content/browser/tracing/memory_tracing_browsertest.cc (right):

https://codereview.chromium.org/2694083005/diff/440001/content/browser/tracin...
content/browser/tracing/memory_tracing_browsertest.cc:49: LOG(ERROR) <<
"MDM::RequestGlobalDump";
Do we need these logs?

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:95:
// TODO(chiniforooshan): This condition is here to match the current
Match the old behavior? and add "behavior of queuing global dump requests only
in browser process". It is not very clear from the code that it is queuing.
does "the transition" mean moving to mojo?

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
(right):

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h:67:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Why do we have task runner here? It sounds cleaner if we do the same thing for
all processes in the delegate instead of posting tasks.
The CoordinatorImpl::RequestGlobalMemoryDump could handle the thread jumps,
right?

I'd prefer not to have is_coordintor_ flag also here. But, we need it
temporarily here.

[jam, 2017-02-24 23:54:37]

lgtm

[chiniforooshan, 2017-02-25 00:04:51]

https://codereview.chromium.org/2694083005/diff/440001/content/browser/tracin...
File content/browser/tracing/memory_tracing_browsertest.cc (right):

https://codereview.chromium.org/2694083005/diff/440001/content/browser/tracin...
content/browser/tracing/memory_tracing_browsertest.cc:49: LOG(ERROR) <<
"MDM::RequestGlobalDump";
On 2017/02/24 23:19:05, ssid wrote:
> Do we need these logs?

Ooops!

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:95:
// TODO(chiniforooshan): This condition is here to match the current
On 2017/02/24 23:19:05, ssid wrote:
> Match the old behavior? and add "behavior of queuing global dump requests only
> in browser process". It is not very clear from the code that it is queuing.
> does "the transition" mean moving to mojo?

Yes. Is the new comment more clear?

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
(right):

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h:67:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2017/02/24 23:19:05, ssid wrote:
> Why do we have task runner here? It sounds cleaner if we do the same thing for
> all processes in the delegate instead of posting tasks.
> The CoordinatorImpl::RequestGlobalMemoryDump could handle the thread jumps,
> right?
> 
> I'd prefer not to have is_coordintor_ flag also here. But, we need it
> temporarily here.

In the case of child processes, CoordinatorImpl will be called on the correct
thread automatically. Mojo handles that for us.

In the case of the process that runs the Coordinator service (i.e. browser), the
delegate has to call CoordinatorImpl on the correct thread or else Mojo will
complain.

So, I think CoordinatorImpl should just assume that it is called on the correct
thread and the browser delegate should do the thread hopping.

I think is_coordinator_ is equivalent to task_runner_ == nullptr and so is
redundant. I deleted it.

[ssid, 2017-02-25 16:07:21]

Lg

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:95:
// TODO(chiniforooshan): This condition is here to match the current
On 2017/02/25 00:04:51, chiniforooshan wrote:
> On 2017/02/24 23:19:05, ssid wrote:
> > Match the old behavior? and add "behavior of queuing global dump requests
only
> > in browser process". It is not very clear from the code that it is queuing.
> > does "the transition" mean moving to mojo?
> 
> Yes. Is the new comment more clear?

Yes thanks

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
File
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h
(right):

https://codereview.chromium.org/2694083005/diff/440001/services/resource_coor...
services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h:67:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2017/02/25 00:04:51, chiniforooshan wrote:
> On 2017/02/24 23:19:05, ssid wrote:
> > Why do we have task runner here? It sounds cleaner if we do the same thing
for
> > all processes in the delegate instead of posting tasks.
> > The CoordinatorImpl::RequestGlobalMemoryDump could handle the thread jumps,
> > right?
> > 
> > I'd prefer not to have is_coordintor_ flag also here. But, we need it
> > temporarily here.
> 
> In the case of child processes, CoordinatorImpl will be called on the correct
> thread automatically. Mojo handles that for us.
> 
> In the case of the process that runs the Coordinator service (i.e. browser),
the
> delegate has to call CoordinatorImpl on the correct thread or else Mojo will
> complain.
> 
> So, I think CoordinatorImpl should just assume that it is called on the
correct
> thread and the browser delegate should do the thread hopping.
> 
> I think is_coordinator_ is equivalent to task_runner_ == nullptr and so is
> redundant. I deleted it.

Hmm thanks for explaining. Sg

[chiniforooshan, 2017-02-27 15:40:16]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-02-27 15:40:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, primiano@chromium.org, kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2694083005/#ps460001
(title: "review")

[commit-bot: I haz the power, 2017-02-27 15:40:28]

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-02-27 15:54:37]

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

[commit-bot: I haz the power, 2017-02-27 15:54:38]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chiniforooshan, 2017-02-27 16:16:46]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-02-27 16:16:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, kenrb@chromium.org, oysteine@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2694083005/#ps470001
(title: "style: auto must not deduce to raw pointer")

[commit-bot: I haz the power, 2017-02-27 16:17:08]

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-02-27 17:34:46]

CQ is committing da patch.

Bot data: {"patchset_id": 470001, "attempt_start_ts": 1488212206160760,
"parent_rev": "a42cfdc27822fb57673106a4b7a953d6c8cda233", "commit_rev":
"e7bee2f02da7fb7afbdb387197ad9461d8d2eba2"}

[commit-bot: I haz the power, 2017-02-27 17:35:26]

Description was changed from

==========
memory-infra: Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
memory-infra: Finish moving memory_infra from TracingController

This finishes moving memory instrumentation code from TracingController
to its own location in //services/resource_coordinator.
    
BUG=679830
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2694083005
Cr-Commit-Position: refs/heads/master@{#453243}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7bee2f02da7fb7afbdb387197ad...
==========

[commit-bot: I haz the power, 2017-02-27 17:35:37]

Committed patchset #25 (id:470001) as
https://chromium.googlesource.com/chromium/src/+/e7bee2f02da7fb7afbdb387197ad...

[Primiano Tucci (use gerrit), 2017-03-01 11:33:38]

A revert of this CL (patchset #25 id:470001) has been created in
https://codereview.chromium.org/2724793002/ by primiano@chromium.org.

The reason for reverting is: Unfortunately this broke lot of memory-infra
metrics and caused flakiness in various bot.
Have to revert because this is causing problems to the sheriff rotation. More
details:
crbug.com/697384 : metrics broken
crbug.com/697062 : memory dumps failing on webview.