[memory-infra] Remove MemoryDumpManagerDelegate

[memory-infra] Remove MemoryDumpManagerDelegate

This CL removes the unnecessary Delegate in MemoryDumpManager and just
uses a callback to request global dumps.

BUG=703184
Review-Url: https://codereview.chromium.org/2819413002
Cr-Commit-Position: refs/heads/master@{#467197}
Committed: https://chromium.googlesource.com/chromium/src/+/f91ad00d0d78fe00a5d1ca4fc50ace6cd63718a7

[ssid, 2017-04-18 02:21:12]

Description was changed from

==========
[memory-infra] Remove MemoryDumpManagerDelegate and just use a callback

This CL removes the unnecessary Delegate in MemoryDumpManager and just
uses a callback to request global dumps.

BUG=703184
==========

to

==========
[memory-infra] Remove MemoryDumpManagerDelegate

This CL removes the unnecessary Delegate in MemoryDumpManager and just
uses a callback to request global dumps.

BUG=703184
==========

[ssid, 2017-04-18 02:25:11]

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

[ssid, 2017-04-18 02:25:40]

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

[ssid, 2017-04-18 02:25:40]

ptal thanks

[commit-bot: I haz the power, 2017-04-18 02:25:49]

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-04-18 04:01:12]

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

[commit-bot: I haz the power, 2017-04-18 04:01:13]

Dry run: This issue passed the CQ dry run.

[fmeawad, 2017-04-18 14:48:11]

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

[fmeawad, 2017-04-18 14:48:11]

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:180: // todo fix
Don't forget this TODO

[chiniforooshan, 2017-04-18 15:11:24]

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

[chiniforooshan, 2017-04-18 15:11:25]

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
File
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
(right):

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:23:
static ProcessLocalDumpManagerImpl* instance = nullptr;
Did you test this in a component build? This is part of the client library.
Unlike the service part, several components can (do) link to the client library.
I think if those components happen to be in the same process, e.g. tests running
in the single process mode, there will be multiple copies of this static
variable and you should get a DCHECK error that MDM is initialized more than
once.

That's why I did not use static variables in the client library in the original
CL.

[ssid, 2017-04-18 19:11:45]

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
File
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
(right):

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:23:
static ProcessLocalDumpManagerImpl* instance = nullptr;
On 2017/04/18 15:11:24, chiniforooshan wrote:
> Did you test this in a component build? This is part of the client library.
> Unlike the service part, several components can (do) link to the client
library.
> I think if those components happen to be in the same process, e.g. tests
running
> in the single process mode, there will be multiple copies of this static
> variable and you should get a DCHECK error that MDM is initialized more than
> once.
> 
> That's why I did not use static variables in the client library in the
original
> CL.

I can't think of better way to manage the lifetime of this object than using a
singleton. I am not very familiar with this component build static variable
issue. Will this problem go away if I used base::Singleton? Since the static
variable will be present in base library rather than this library.
Also this function is not GetInstance() it is InitializeInstance() and this
should only be called once.
I can change to base::Singleton if you confirm that will fix the issue.

I guess the issue will not show up in component build because it is anyway
called only once.

[Primiano Tucci (use gerrit), 2017-04-18 19:31:25]

Sorry didn't have any time to look at this today, will do tomorrow.

I haven't looked at the code, so can't express right now any opinion about
singleton vs non-singleton, but I'm only sure about that thing: our decision
shouldn't be driven by a ODR violation. 
In other words, we should stop work around the component build issue. That's a
pure ODR violation.
We should be free to use singletons or just static locals if we need to.

The fact that today a translation unit is linked in different link units (i.e.
.so(s)) is just a bug and we should fix that.
Having a translation unit where "don't add static or globals or bad thing will
happen" is unprecedented in chrome.
I totally understand that we happened to be in that state and avoiding the
singleton was a short gap solution to get things to work, but now should fix
that up.
Very likely that is just a matter of s/source_set/component/ in the BUILD.gn
file. Can somebody plz take a look to that?

[chiniforooshan, 2017-04-18 19:49:10]

On 2017/04/18 19:31:25, Primiano Tucci wrote:
> Sorry didn't have any time to look at this today, will do tomorrow.
> 
> I haven't looked at the code, so can't express right now any opinion about
> singleton vs non-singleton, but I'm only sure about that thing: our decision
> shouldn't be driven by a ODR violation. 
> In other words, we should stop work around the component build issue. That's a
> pure ODR violation.
> We should be free to use singletons or just static locals if we need to.
> 
> The fact that today a translation unit is linked in different link units (i.e.
> .so(s)) is just a bug and we should fix that.
> Having a translation unit where "don't add static or globals or bad thing will
> happen" is unprecedented in chrome.
> I totally understand that we happened to be in that state and avoiding the
> singleton was a short gap solution to get things to work, but now should fix
> that up.
> Very likely that is just a matter of s/source_set/component/ in the BUILD.gn
> file. Can somebody plz take a look to that?

I agree and disagree :) I agree that we should be able to use singletons and so
we should fix the BUILD.gn if necessary. And I disagree that creating an object
with an explicit lifetime is a "hack" around not being able to use a singleton.
I think whenever it is possible, lifetime of objects should be explicit.
Singletons make it really hard to understand when the object is actually
initialized.

In this specific case, for example, the NOTREACHED() check in the
InitializeInstance method tells me that probably we can explicitly instantiate
the ProcessLocalDumpManager wherever InitializeInstance is called, no?

[Primiano Tucci (use gerrit), 2017-04-18 19:51:55]

On 2017/04/18 19:49:10, chiniforooshan wrote:
> And I disagree that creating an object
> with an explicit lifetime is a "hack" around not being able to use a
singleton.
> I think whenever it is possible, lifetime of objects should be explicit.
Oh sorry didn't reach this part yet (i.e. didn't look at the CL).
I neither agree nor disagree for the moment, but reserve to pick one of the two
tomorrow after looking at this :)
I was just saying that our decision, whatever it is, should be driven by
discussions like this, and not by ODR violations. But we seem to all agree on
this part.
See you tomorrow on this channel

[Primiano Tucci (use gerrit), 2017-04-19 14:24:09]

Okay I took a look to the CL (which looks great % this discussion), now I
understand the controversy.
Here's my viewpoint:

> Singletons make it really hard to understand when the object is actually
initialized.
True, but I think that ssid's current patchset  is solving that problem by
allowing only one instance to be created by one explicit static method.

IMHO the arguments pro and against are:
Against: A singleton (either base::Singleton or home-brewed) is harder to test.
Specifically, if we end up in a state where the singleton needs to be
initialized more than once within a test unit.
Pro: A singleton avoids to have to reason about unregistration, race conditions
about that (can't use Unretained, require WeakPtr, an stuff like that) which, in
some cases, might come with extra code complexity which is unnecessary. 

Also, consistency-wise, I quickly glanced at the code, and I see only a
SetParentPipeHandle() call in InitializeMojoIPCChannel(), without any teardown,
which seems to suggest the latter.
Generally, after last haraken refactor (see "Making LazyInstance Leaky by
default" on chromium-dev) we don't call shutdown anymore in the child processes,
and my understanding is that the general guidance is that we shouldn't bother
with shutdown code unless there is a necessity (i.e. having to persist state to
the disk and things like this which is not our case). 

So, my suggestion here would be: unless I am wrong and something unexpectedly is
constructing ChildThread more than once within a process (the only doubt here
is: what do tests do?) I wouldn't bother trying to handling shutdown and just
leak all the things.
I really value chiniforooshan@ point about making the place where initialization
happens explicit and code searchable, but I think that ssid's approach should
solve that (I'd just rename that InitializeInstance -> CreateInstance(), as
there seems to be some other precedent for that naming wise*

* https://cs.chromium.org/search/?q=void%5C+CreateInstance%5C(%5C)&type=cs

[chiniforooshan, 2017-04-19 14:48:04]

On 2017/04/19 14:24:09, Primiano Tucci wrote:
> Okay I took a look to the CL (which looks great % this discussion), now I
> understand the controversy.
> Here's my viewpoint:
> 
> > Singletons make it really hard to understand when the object is actually
> initialized.
> True, but I think that ssid's current patchset  is solving that problem by
> allowing only one instance to be created by one explicit static method.
> 
> IMHO the arguments pro and against are:
> Against: A singleton (either base::Singleton or home-brewed) is harder to
test.
> Specifically, if we end up in a state where the singleton needs to be
> initialized more than once within a test unit.
> Pro: A singleton avoids to have to reason about unregistration, race
conditions
> about that (can't use Unretained, require WeakPtr, an stuff like that) which,
in
> some cases, might come with extra code complexity which is unnecessary. 
> 
> Also, consistency-wise, I quickly glanced at the code, and I see only a
> SetParentPipeHandle() call in InitializeMojoIPCChannel(), without any
teardown,
> which seems to suggest the latter.
> Generally, after last haraken refactor (see "Making LazyInstance Leaky by
> default" on chromium-dev) we don't call shutdown anymore in the child
processes,
> and my understanding is that the general guidance is that we shouldn't bother
> with shutdown code unless there is a necessity (i.e. having to persist state
to
> the disk and things like this which is not our case). 
> 
> So, my suggestion here would be: unless I am wrong and something unexpectedly
is
> constructing ChildThread more than once within a process (the only doubt here
> is: what do tests do?) I wouldn't bother trying to handling shutdown and just
> leak all the things.
> I really value chiniforooshan@ point about making the place where
initialization
> happens explicit and code searchable, but I think that ssid's approach should
> solve that (I'd just rename that InitializeInstance -> CreateInstance(), as
> there seems to be some other precedent for that naming wise*
> 
> * https://cs.chromium.org/search/?q=void%5C+CreateInstance%5C(%5C)&type=cs

LGTM. InitializeInstance -> CreateInstance would be great.

[Primiano Tucci (use gerrit), 2017-04-19 15:44:36]

Took a look to the rest, just minor comments, LG on the rest if we agree on the
ownership pattern.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:69: using RequestGlobalDumpCallback =
small nit: type declarations should go up, after public:
see https://google.github.io/styleguide/cppguide.html#Declaration_Order

we have a bunch of things called "callback" around these days. Can we pick a
more specific name for this?
I'd probably not call it "callback" at all (there is precedent for this *) as
this is semantically not quite a callback but more an inverted function call.
probably I'd call it RequestGlobalDumpFunction.

Also can you add a "TODO(primiano): in the long term clients should just reach
into the memory service. For the moment however that requires extra dependencies
and we use this as a way to reach the service from this base class."

*
https://cs.chromium.org/search/?q=using%5C+%5Cw%2Bfunction%5C+%3D%5C+.*Callba...

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:126: // NOTE: Use RequestGlobalDump() to
create memory dumps. Creates a memory dump
Since you have the "This method should only be used..." I'd remote the NOTE
here.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:293: RequestGlobalDumpCallback
request_dump_callback_;
s/callback/function/

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:299: // |is_coordinator_| to guard
against disabling logging while dumping on
I think over time more stuff got serialized aroudn this lock (the dump_thread_
iirc) so I'd probably just say: "Protects form concurrent access to the local
state, e.g. to guard against ..."

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:131: class
GlobalMemoryDumpHandler {
I'd keep a comment saing: this mocks the ReqGlobalDumpFunction which is
typically handled by process_local_dump_manager_impl.cc, by short-circuiting
....without an actual service

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/BUILD.gn:5: source_set("cpp") {
I think this needs to become a component("cpp") to get around the ODR violations

precedent: see device/generic_sensor/public/cpp/BUILD.gn

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
File
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
(right):

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:22:
void ProcessLocalDumpManagerImpl::InitializeInstance(const Config& config) {
I'd s/InitializeInstance/CreateInstance/

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:23:
static ProcessLocalDumpManagerImpl* instance = nullptr;
On 2017/04/18 19:11:45, ssid wrote:
> On 2017/04/18 15:11:24, chiniforooshan wrote:
> > Did you test this in a component build? This is part of the client library.
> > Unlike the service part, several components can (do) link to the client
> library.
> > I think if those components happen to be in the same process, e.g. tests
> running
> > in the single process mode, there will be multiple copies of this static
> > variable and you should get a DCHECK error that MDM is initialized more than
> > once.
> > 
> > That's why I did not use static variables in the client library in the
> original
> > CL.
> 
> I can't think of better way to manage the lifetime of this object than using a
> singleton. I am not very familiar with this component build static variable
> issue. Will this problem go away if I used base::Singleton? Since the static
> variable will be present in base library rather than this library.
> Also this function is not GetInstance() it is InitializeInstance() and this
> should only be called once.
> I can change to base::Singleton if you confirm that will fix the issue.
> 
> I guess the issue will not show up in component build because it is anyway
> called only once.

If we agree on this pattern, this seems fine to me.
The problem with component build is really ODR violation. As itis, this
translation unit is linked into several .so files, and each of them gets its own
instance of |instance| (wouldn't be any different with base::Singleton or
base::Leakyinstance or using a static g_instance in the anonymous namepace) 
see my comment on the build.gn file, the fix should be just source_set ->
component

[ssid, 2017-04-20 01:18:21]

Thanks made suggested changes.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:69: using RequestGlobalDumpCallback =
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> small nit: type declarations should go up, after public:
> see https://google.github.io/styleguide/cppguide.html#Declaration_Order
> 
> we have a bunch of things called "callback" around these days. Can we pick a
> more specific name for this?
> I'd probably not call it "callback" at all (there is precedent for this *) as
> this is semantically not quite a callback but more an inverted function call.
> probably I'd call it RequestGlobalDumpFunction.
> 
> Also can you add a "TODO(primiano): in the long term clients should just reach
> into the memory service. For the moment however that requires extra
dependencies
> and we use this as a way to reach the service from this base class."
> 

I don't think this should be moved. Let's say in future we have a new allocator
in base that does a GC and we want to request global dumps when GC is done. This
would become impossible because it can't reach the service. Similarly lot of
areas in codebase cannot depend on service, like sql/ and gin/. It will be
better if we keep this api in base.

> *
>
https://cs.chromium.org/search/?q=using%5C+%5Cw%2Bfunction%5C+%3D%5C+.*Callba...

Yeah I was expecting this comment. Just felt function was not used in chromium.
Fixed.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:126: // NOTE: Use RequestGlobalDump() to
create memory dumps. Creates a memory dump
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> Since you have the "This method should only be used..." I'd remote the NOTE
> here.
I'd like to keep this note because it is really easy for someone to call this
function by mistake since it's named CreateProcessDump()

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:180: // todo fix
On 2017/04/18 14:48:11, fmeawad wrote:
> Don't forget this TODO

Done.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:293: RequestGlobalDumpCallback
request_dump_callback_;
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> s/callback/function/

Done.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:299: // |is_coordinator_| to guard
against disabling logging while dumping on
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> I think over time more stuff got serialized aroudn this lock (the dump_thread_
> iirc) so I'd probably just say: "Protects form concurrent access to the local
> state, e.g. to guard against ..."

Done.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager_unittest.cc:131: class
GlobalMemoryDumpHandler {
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> I'd keep a comment saing: this mocks the ReqGlobalDumpFunction which is
> typically handled by process_local_dump_manager_impl.cc, by short-circuiting
> ....without an actual service

Done.

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
File services/resource_coordinator/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/BUILD.gn:5: source_set("cpp") {
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> I think this needs to become a component("cpp") to get around the ODR
violations
> 
> precedent: see device/generic_sensor/public/cpp/BUILD.gn

Unfortunately since this exists I cannot create another component("cpp"). So,
named it resource_coordinator_cpp.
I think the existing one should also be named different.

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
File
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
(right):

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:22:
void ProcessLocalDumpManagerImpl::InitializeInstance(const Config& config) {
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> I'd s/InitializeInstance/CreateInstance/

Done.

https://codereview.chromium.org/2819413002/diff/1/services/resource_coordinat...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:23:
static ProcessLocalDumpManagerImpl* instance = nullptr;
On 2017/04/19 15:44:34, Primiano Tucci wrote:
> On 2017/04/18 19:11:45, ssid wrote:
> > On 2017/04/18 15:11:24, chiniforooshan wrote:
> > > Did you test this in a component build? This is part of the client
library.
> > > Unlike the service part, several components can (do) link to the client
> > library.
> > > I think if those components happen to be in the same process, e.g. tests
> > running
> > > in the single process mode, there will be multiple copies of this static
> > > variable and you should get a DCHECK error that MDM is initialized more
than
> > > once.
> > > 
> > > That's why I did not use static variables in the client library in the
> > original
> > > CL.
> > 
> > I can't think of better way to manage the lifetime of this object than using
a
> > singleton. I am not very familiar with this component build static variable
> > issue. Will this problem go away if I used base::Singleton? Since the static
> > variable will be present in base library rather than this library.
> > Also this function is not GetInstance() it is InitializeInstance() and this
> > should only be called once.
> > I can change to base::Singleton if you confirm that will fix the issue.
> > 
> > I guess the issue will not show up in component build because it is anyway
> > called only once.
> 
> If we agree on this pattern, this seems fine to me.
> The problem with component build is really ODR violation. As itis, this
> translation unit is linked into several .so files, and each of them gets its
own
> instance of |instance| (wouldn't be any different with base::Singleton or
> base::Leakyinstance or using a static g_instance in the anonymous namepace) 
> see my comment on the build.gn file, the fix should be just source_set ->
> component

Done.

[ssid, 2017-04-20 01:19:03]

On 2017/04/20 01:18:21, ssid wrote:
> Thanks made suggested changes.
except the 2 changes in comments.

[ssid, 2017-04-20 01:21:39]

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

[ssid, 2017-04-20 01:21:39]

+jochen for 2 small changes in content/child.

[jochen (gone - plz use gerrit), 2017-04-20 09:49:51]

lgtm

[fmeawad, 2017-04-20 16:06:49]

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

[Primiano Tucci (use gerrit), 2017-04-20 19:28:29]

And when I wrote LG I actually meant lgtm assuming comments have been addressed
in the meantime

[ssid, 2017-04-20 19:31:41]

On 2017/04/20 19:28:29, Primiano Tucci wrote:
> And when I wrote LG I actually meant lgtm assuming comments have been
addressed
> in the meantime

I am assuming you are fine with these 2 replies then?
https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...

[ssid, 2017-04-20 19:31:48]

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

[commit-bot: I haz the power, 2017-04-20 19:32:52]

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-04-20 19:36:41]

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

[commit-bot: I haz the power, 2017-04-20 19:36:42]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Primiano Tucci (use gerrit), 2017-04-20 20:30:05]

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:69: using RequestGlobalDumpCallback =
On 2017/04/20 01:18:21, ssid wrote:
> On 2017/04/19 15:44:34, Primiano Tucci wrote:
> > small nit: type declarations should go up, after public:
> > see https://google.github.io/styleguide/cppguide.html#Declaration_Order
> > 
> > we have a bunch of things called "callback" around these days. Can we pick a
> > more specific name for this?
> > I'd probably not call it "callback" at all (there is precedent for this *)
as
> > this is semantically not quite a callback but more an inverted function
call.
> > probably I'd call it RequestGlobalDumpFunction.
> > 
> > Also can you add a "TODO(primiano): in the long term clients should just
reach
> > into the memory service. For the moment however that requires extra
> dependencies
> > and we use this as a way to reach the service from this base class."
> > 
> 
> I don't think this should be moved. Let's say in future we have a new
allocator
> in base that does a GC and we want to request global dumps when GC is done.
This
> would become impossible because it can't reach the service. Similarly lot of
> areas in codebase cannot depend on service, like sql/ and gin/. It will be
> better if we keep this api in base.

Hmm good point. ACK

> 
> > *
> >
>
https://cs.chromium.org/search/?q=using%5C+%5Cw%2Bfunction%5C+%3D%5C+.*Callba...
> 
> Yeah I was expecting this comment. Just felt function was not used in
chromium.
> Fixed.

https://codereview.chromium.org/2819413002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:126: // NOTE: Use RequestGlobalDump() to
create memory dumps. Creates a memory dump
On 2017/04/20 01:18:21, ssid wrote:
> On 2017/04/19 15:44:34, Primiano Tucci wrote:
> > Since you have the "This method should only be used..." I'd remote the NOTE
> > here.
> I'd like to keep this note because it is really easy for someone to call this
> function by mistake since it's named CreateProcessDump()

Acknowledged.

[ssid, 2017-04-20 22:25:34]

Patchset #3 (id:40001) has been deleted

[ssid, 2017-04-20 22:26:06]

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

[commit-bot: I haz the power, 2017-04-20 22:26:55]

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-04-20 22:29:08]

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

[commit-bot: I haz the power, 2017-04-20 22:29:09]

Dry run: 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...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[ssid, 2017-04-20 22:44:10]

Patchset #3 (id:60001) has been deleted

[ssid, 2017-04-20 22:44:25]

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

[commit-bot: I haz the power, 2017-04-20 22:44:58]

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-04-20 22:47:30]

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

[commit-bot: I haz the power, 2017-04-20 22:47:31]

Dry run: 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...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[ssid, 2017-04-20 22:55:14]

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

[commit-bot: I haz the power, 2017-04-20 22:55:41]

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-04-20 22:58:54]

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

[commit-bot: I haz the power, 2017-04-20 22:58:55]

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

[ssid, 2017-04-20 23:13:55]

Patchset #4 (id:100001) has been deleted

[ssid, 2017-04-20 23:32:42]

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

[commit-bot: I haz the power, 2017-04-20 23:33:02]

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

[ssid, 2017-04-20 23:49:57]

Sorry, I hadn't checked the build before sending out CL. I had to add EXPORT
macros and a new file for it. sorry, ptal at the change. Thanks!

[commit-bot: I haz the power, 2017-04-21 00:22:05]

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

[commit-bot: I haz the power, 2017-04-21 00:22:06]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Primiano Tucci (use gerrit), 2017-04-21 17:11:50]

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

[Primiano Tucci (use gerrit), 2017-04-21 17:11:51]

+rockot can you please take a look to the diff between PS2 and PS3, where ssid
is adding _EXPORT macros?
It makes sense to me, but wouldn't mind an extra pair of eyes. 

From my viewpoint this still LG if some mojo person is happy.

https://codereview.chromium.org/2819413002/diff/80001/services/resource_coord...
File services/resource_coordinator/BUILD.gn (right):

https://codereview.chromium.org/2819413002/diff/80001/services/resource_coord...
services/resource_coordinator/BUILD.gn:8: source_set("lib") {
I think this needs to become component("lib") otherwise you'll be bitten by all
sort of ODR violations. this lib is linked in by different linker unit in
component builds.

[Ken Rockot(use gerrit already), 2017-04-21 17:32:52]

On 2017/04/21 at 17:11:51, primiano wrote:
> +rockot can you please take a look to the diff between PS2 and PS3, where ssid
is adding _EXPORT macros?
> It makes sense to me, but wouldn't mind an extra pair of eyes. 
> 

If you make your client library (i.e. public/cpp) a component -- which is
uncommon and should be avoided if possible -- you will almost certainly be
bitten by linker errors regarding your mojom interfaces and/or typemaps, since
those definitions end up in a generated source_set. In fact maybe that's the
Windows link failure we're already seeing here?

If you really really want this to be a component target, we have some messy
trickery to merge the mojom target's C++ stuff into the same component. Your
mojom rule has to add:

  export_class_attribute =
"SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_MEMORY_EXPORT"
  export_define =
"SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_MEMORY_IMPLEMENTATION"
  export_header = "services/resource_coordinator/public/cpp/memory_export.h"

You should then make the mojom target visible *only* to the client library
target, and add it to the client library's public_deps (along with an
allow_circular_includes_from = [ "your_mojom_target" ])

For an example, ash does this (see ash/public/cpp:ash_public_cpp and
ash/public/interfaces:interfaces_internal).

Oooor you could just not make this a component target :)

> From my viewpoint this still LG if some mojo person is happy.
> 
>
https://codereview.chromium.org/2819413002/diff/80001/services/resource_coord...
> File services/resource_coordinator/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2819413002/diff/80001/services/resource_coord...
> services/resource_coordinator/BUILD.gn:8: source_set("lib") {
> I think this needs to become component("lib") otherwise you'll be bitten by
all sort of ODR violations. this lib is linked in by different linker unit in
component builds.

Not sure what different linker units you're referring to. This is a common
pattern for service implementations and we don't seem to ever hit ODR
violations. I'm not sure how we would without incorrect dependencies somewhere.

[Primiano Tucci (use gerrit), 2017-04-21 18:53:41]

> If you make your client library (i.e. public/cpp) a component -- which is
uncommon and should be avoided if possible
What is the alternative to avoid ODR violation then?
The pattern that violates ODR is quite simple and is the following

- client_library has a static global (say a singleton, or any function-level
static variable)
- client_library is a source-set (this is what is today)
- component("foo") depends on client_library
- component("bar") depends on client_library

Now you have foo.so and bar.so both linking the same source-set. Hence all the
global state in client_library is duplicated in component builds.
How else to solve this if not making client_library a component itself?
My understanding is that a client-library is by design going to be dpended on by
several clients, right?

This caused problems in the past, causing a revert to chiniforoohans's cl
https://codereview.chromium.org/2694083005

[Primiano Tucci (use gerrit), 2017-04-21 18:56:12]

On 2017/04/21 18:53:41, Primiano Tucci wrote:
> > If you make your client library (i.e. public/cpp) a component -- which is
> uncommon and should be avoided if possible
> What is the alternative to avoid ODR violation then?
> The pattern that violates ODR is quite simple and is the following
> 
> - client_library has a static global (say a singleton, or any function-level
> static variable)
> - client_library is a source-set (this is what is today)
> - component("foo") depends on client_library
> - component("bar") depends on client_library
> 
> Now you have foo.so and bar.so both linking the same source-set. Hence all the
> global state in client_library is duplicated in component builds.
> How else to solve this if not making client_library a component itself?
> My understanding is that a client-library is by design going to be dpended on
by
> several clients, right?
> 
> This caused problems in the past, causing a revert to chiniforoohans's cl
> https://codereview.chromium.org/2694083005

The specific instance "foo" of "bar" where "content_child" and
"resource_coordinator:service" which in webview are hosted in the same process.

[Ken Rockot(use gerrit already), 2017-04-21 19:04:42]

Ah I see re: the static global. Didn't realize it had one.


On Fri, Apr 21, 2017 at 11:56 AM, <primiano@chromium.org> wrote:

> On 2017/04/21 18:53:41, Primiano Tucci wrote:
> > > If you make your client library (i.e. public/cpp) a component -- which
> is
> > uncommon and should be avoided if possible
> > What is the alternative to avoid ODR violation then?
> > The pattern that violates ODR is quite simple and is the following
> >
> > - client_library has a static global (say a singleton, or any
> function-level
> > static variable)
> > - client_library is a source-set (this is what is today)
> > - component("foo") depends on client_library
> > - component("bar") depends on client_library
> >
> > Now you have foo.so and bar.so both linking the same source-set. Hence
> all the
> > global state in client_library is duplicated in component builds.
> > How else to solve this if not making client_library a component itself?
> > My understanding is that a client-library is by design going to be
> dpended on
> by
> > several clients, right?
> >
> > This caused problems in the past, causing a revert to chiniforoohans's cl
> > https://codereview.chromium.org/2694083005
>
> The specific instance "foo" of "bar" where "content_child" and
> "resource_coordinator:service" which in webview are hosted in the same
> process.
>
> https://codereview.chromium.org/2819413002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2017-04-24 13:58:06]

On 2017/04/21 19:04:42, Ken Rockot wrote:
> Ah I see re: the static global. Didn't realize it had one.

Well in general we should never hand in the state where we have translation
units in chrome where people should be worried of having a static globals.
Can't tell if this is a general problem with all current client libraries and
just nothing else has globals, but I would like not have to worry about being in
this state in the first place, at least as far as the GRC/memory client library
is concerned.
ODR-violation bugs are extremely subtle and people can easily spend full
eng-days to narrow them down.

[ssid, 2017-04-25 01:43:30]

Patchset #6 (id:160001) has been deleted

[ssid, 2017-04-25 01:43:41]

Patchset #5 (id:140001) has been deleted

[ssid, 2017-04-25 01:43:48]

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

[commit-bot: I haz the power, 2017-04-25 01:44:24]

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-04-25 02:42:01]

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

[commit-bot: I haz the power, 2017-04-25 02:42:02]

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

[ssid, 2017-04-25 02:49:16]

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

[commit-bot: I haz the power, 2017-04-25 02:49:46]

Dry run: CQ is trying da patch.

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

[ssid, 2017-04-25 03:23:56]

Ken, ptal. I think I have made changes as you suggested and the bots that were
failing turned green.

[ssid, 2017-04-25 03:24:54]

Patchset #5 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-04-25 04:19:14]

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

[commit-bot: I haz the power, 2017-04-25 04:19:15]

Dry run: This issue passed the CQ dry run.

[chiniforooshan, 2017-04-25 14:40:09]

On 2017/04/25 03:23:56, ssid wrote:
> Ken, ptal. I think I have made changes as you suggested and the bots that were
> failing turned green.

Just a passerby comment; feel free to ignore: I don't understand why we should
make the BUILD files so complicated. Looks unnecessary to me. Why don't we let
ChildThreadImpl own ProcessLocalDumpManagerImpl? If I'm not missing something,
it looks like that's the only place that CreateInstance is called.

[chiniforooshan, 2017-04-25 14:44:25]

On 2017/04/25 14:40:09, chiniforooshan wrote:
> On 2017/04/25 03:23:56, ssid wrote:
> > Ken, ptal. I think I have made changes as you suggested and the bots that
were
> > failing turned green.
> 
> Just a passerby comment; feel free to ignore: I don't understand why we should
> make the BUILD files so complicated. Looks unnecessary to me. Why don't we let
> ChildThreadImpl own ProcessLocalDumpManagerImpl? If I'm not missing something,
> it looks like that's the only place that CreateInstance is called.

Let me revise my comment: why not CoordinatorImpl and ChildThreadImpl, when it's
not running in the browser process, each own an instance of
ProcessLocalDumpManagerImpl?

[Primiano Tucci (use gerrit), 2017-04-25 15:01:36]

On 2017/04/25 14:44:25, chiniforooshan wrote:
> On 2017/04/25 14:40:09, chiniforooshan wrote:
> > On 2017/04/25 03:23:56, ssid wrote:
> > > Ken, ptal. I think I have made changes as you suggested and the bots that
> were
> > > failing turned green.
> > 
> > Just a passerby comment; feel free to ignore: I don't understand why we
should
> > make the BUILD files so complicated. Looks unnecessary to me. Why don't we
let
> > ChildThreadImpl own ProcessLocalDumpManagerImpl? If I'm not missing
something,
> > it looks like that's the only place that CreateInstance is called.
> 
> Let me revise my comment: why not CoordinatorImpl and ChildThreadImpl, when
it's
> not running in the browser process, each own an instance of
> ProcessLocalDumpManagerImpl?

That is definitely a possibility code-wise, but feels to me more a warkound
around something that shouldn't happen in the first place.
I just really don't like to live in the state where every time I review a CL in
the client library I have to think "ah, check that nobody is adding globals,
because they will break component builds".
Have to live with ODR violation is, AFAIK, unprecedented in our codebase.
ODR violations can be really subtle. One day somebody might refactor base and
introduce a static local in an inline function. We shouldn't just be in this
state in the first place.

[Ken Rockot(use gerrit already), 2017-04-25 15:13:35]

On Tue, Apr 25, 2017 at 8:01 AM, <primiano@chromium.org> wrote:

> On 2017/04/25 14:44:25, chiniforooshan wrote:
> > On 2017/04/25 14:40:09, chiniforooshan wrote:
> > > On 2017/04/25 03:23:56, ssid wrote:
> > > > Ken, ptal. I think I have made changes as you suggested and the bots
> that
> > were
> > > > failing turned green.
> > >
> > > Just a passerby comment; feel free to ignore: I don't understand why we
> should
> > > make the BUILD files so complicated. Looks unnecessary to me. Why
> don't we
> let
> > > ChildThreadImpl own ProcessLocalDumpManagerImpl? If I'm not missing
> something,
> > > it looks like that's the only place that CreateInstance is called.
> >
> > Let me revise my comment: why not CoordinatorImpl and ChildThreadImpl,
> when
> it's
> > not running in the browser process, each own an instance of
> > ProcessLocalDumpManagerImpl?
>
> That is definitely a possibility code-wise, but feels to me more a warkound
> around something that shouldn't happen in the first place.
> I just really don't like to live in the state where every time I review a
> CL in
> the client library I have to think "ah, check that nobody is adding
> globals,
> because they will break component builds".
> Have to live with ODR violation is, AFAIK, unprecedented in our codebase.
> ODR violations can be really subtle. One day somebody might refactor base
> and
> introduce a static local in an inline function. We shouldn't just be in
> this
> state in the first place.
>

If we're going to require every client library to be a component out of
fear of misuse, things are going to get very ugly very quickly. The only
way I think that policy is sustainable is if we make all generated mojom
bindings libraries components as well. I'm open to doing that still, but
it's a little tricky - I suppose we'd want to generate export macro names
from target path (e.g. //foo/public/interfaces:cool_mojom generates a
component which defines FOO_PUBLIC_INTERFACES_COOL_MOJOM_IMPL, and exports
conditionally on FOO_PUBLIC_INTERFACES_COOL_MOJOM_EXPORT).

But why do we allow any non-component source targets at all if this is
going to be the default position?


> https://codereview.chromium.org/2819413002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chiniforooshan, 2017-04-25 15:22:38]

On 2017/04/25 15:01:36, Primiano Tucci wrote:
> On 2017/04/25 14:44:25, chiniforooshan wrote:
> > On 2017/04/25 14:40:09, chiniforooshan wrote:
> > > On 2017/04/25 03:23:56, ssid wrote:
> > > > Ken, ptal. I think I have made changes as you suggested and the bots
that
> > were
> > > > failing turned green.
> > > 
> > > Just a passerby comment; feel free to ignore: I don't understand why we
> should
> > > make the BUILD files so complicated. Looks unnecessary to me. Why don't we
> let
> > > ChildThreadImpl own ProcessLocalDumpManagerImpl? If I'm not missing
> something,
> > > it looks like that's the only place that CreateInstance is called.
> > 
> > Let me revise my comment: why not CoordinatorImpl and ChildThreadImpl, when
> it's
> > not running in the browser process, each own an instance of
> > ProcessLocalDumpManagerImpl?
> 
> That is definitely a possibility code-wise, but feels to me more a warkound
> around something that shouldn't happen in the first place.
> I just really don't like to live in the state where every time I review a CL
in
> the client library I have to think "ah, check that nobody is adding globals,
> because they will break component builds".

Fair enough :)

> Have to live with ODR violation is, AFAIK, unprecedented in our codebase.
> ODR violations can be really subtle. One day somebody might refactor base and
> introduce a static local in an inline function. We shouldn't just be in this
> state in the first place.

I don't know about this being unprecedented: in services only,
//services/service_manager, //services/preferences, //services/device, and
//services/data_decoder all have client libs with source_set targets that are
linked to by different components.

[ssid, 2017-04-25 18:28:08]

So, I agree with priminao about not having to worry about the statics, singleton
issues in the library and make the build changes for that sake. This would be
useful in future when we face similar issues. We use source_set() in chromium
because they make sense most of the times and singletons and static are
generally avoided. But, in this case the MDM is a singleton and it uses the
delegate. MDM must be a singleton because we expect any piece of code to be able
to request global dumps and register dump providers. So, better to have a
singleton here rather than a bound lifetime to child thread which can cause bugs
in future. And when such cases arise the general practice in chromium is to
change to component and we should be aware of the build configuration before
introducing statics.

[Ken Rockot(use gerrit already), 2017-04-25 18:30:42]

OK - LGTM, thanks for the discussion. I'm going to attempt to move forward with
making mojom targets components by default, and then we can get rid of the GN
ugliness here.

[Primiano Tucci (use gerrit), 2017-04-25 18:33:46]

> If we're going to require every client library to be a component out of
> fear of misuse, things are going to get very ugly very quickly. 
This is the part I don't agree ("fear of misuse"). From my viewpoint this is
just "using standard c++ things that we use anywhere else in the codebase"
A static global (or base::SIngleton, or LazyInstance) is just a C++ thing that
is allowed anywhere else. I don't see why client libraries should have different
code-style restrictions.
I understand that we ended up in this state by accident, I just thing we should
get out of this state now that we found out.

> But why do we allow any non-component source targets at all if this is
> going to be the default position?
Because an ODR does not happen if a source-set is linked by two linker units
that are independent at runtime, e.g.:
- two executables
- a .so part of chrome and an executable
- two .so that are used in different executables (say a .so for chrome_sandbox
and a .so for chrome).

I think that encoding this sort of restriction in GN check is just too hard and
nobody bothered since we don't ship component builds to the public (thankfully).

> I don't know about this being unprecedented: in services only,
> //services/service_manager, //services/preferences, //services/device, and
> //services/data_decoder all have client libs with source_set targets that are
> linked to by different components.
Well, if you notice the pattern they are all //services  :)
What I am trying to say here is that this will bite other developers in all
those libraries as soon as they, directly or indirectly, end up having some
global, or rely addresses of functions.
I won't be surprised if, regardless of globals, this would subtlety break, say,
base's Scheduler because the addresses of the Bind()-ed funtions is not unique.


On 2017/04/25 15:22:38, chiniforooshan wrote:
> On 2017/04/25 15:01:36, Primiano Tucci wrote:
> > On 2017/04/25 14:44:25, chiniforooshan wrote:
> > > On 2017/04/25 14:40:09, chiniforooshan wrote:
> > > > On 2017/04/25 03:23:56, ssid wrote:
> > > > > Ken, ptal. I think I have made changes as you suggested and the bots
> that
> > > were
> > > > > failing turned green.
> > > > 
> > > > Just a passerby comment; feel free to ignore: I don't understand why we
> > should
> > > > make the BUILD files so complicated. Looks unnecessary to me. Why don't
we
> > let
> > > > ChildThreadImpl own ProcessLocalDumpManagerImpl? If I'm not missing
> > something,
> > > > it looks like that's the only place that CreateInstance is called.
> > > 
> > > Let me revise my comment: why not CoordinatorImpl and ChildThreadImpl,
when
> > it's
> > > not running in the browser process, each own an instance of
> > > ProcessLocalDumpManagerImpl?
> > 
> > That is definitely a possibility code-wise, but feels to me more a warkound
> > around something that shouldn't happen in the first place.
> > I just really don't like to live in the state where every time I review a CL
> in
> > the client library I have to think "ah, check that nobody is adding globals,
> > because they will break component builds".
> 
> Fair enough :)
> 
> > Have to live with ODR violation is, AFAIK, unprecedented in our codebase.
> > ODR violations can be really subtle. One day somebody might refactor base
and
> > introduce a static local in an inline function. We shouldn't just be in this
> > state in the first place.
> 
> I don't know about this being unprecedented: in services only,
> //services/service_manager, //services/preferences, //services/device, and
> //services/data_decoder all have client libs with source_set targets that are
> linked to by different components.

[Primiano Tucci (use gerrit), 2017-04-25 18:37:31]

>  We use source_set() in chromium because they make sense most of the times and
singletons and static are generally avoided.
Well, not really. We use source_set (or static_library, in this case is
irrelevant) in chromium mostly to include a target into one linker unit only.
Any other multiple-inclusion is at author risk, which should be careful in not
creating ODRs in component builds.
Unfortunately we don't have any check in place to detect this.

[ssid, 2017-04-25 18:48:53]

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

[ssid, 2017-04-25 18:48:55]

+rsesek for the memory_instrumentation_struct_traits.h. Changes the mojo structs
to include export macros.

[Robert Sesek, 2017-04-25 19:02:20]

struct_traits lgtm

[ssid, 2017-04-25 19:02:55]

The CQ bit was checked by ssid@chromium.org

[ssid, 2017-04-25 19:02:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
chiniforooshan@chromium.org, primiano@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2819413002/#ps200001
(title: "mojom stuff in component.")

[commit-bot: I haz the power, 2017-04-25 19:03:49]

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-04-25 19:13:47]

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

[commit-bot: I haz the power, 2017-04-25 19:13:49]

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

[ssid, 2017-04-25 20:12:04]

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

[commit-bot: I haz the power, 2017-04-25 20:12:33]

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-04-25 20:23:52]

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

[commit-bot: I haz the power, 2017-04-25 20:23:53]

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

[ssid, 2017-04-25 21:33:19]

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

[commit-bot: I haz the power, 2017-04-25 21:34:13]

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-04-25 21:45:09]

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

[commit-bot: I haz the power, 2017-04-25 21:45:11]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ssid, 2017-04-25 22:41:42]

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

[ssid, 2017-04-25 22:41:54]

Patchset #6 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-04-25 22:42:26]

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-04-25 23:48:33]

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

[commit-bot: I haz the power, 2017-04-25 23:48:34]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-04-26 01:15:01]

The CQ bit was checked by ssid@chromium.org

[ssid, 2017-04-26 01:15:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, rockot@chromium.org, jochen@chromium.org,
chiniforooshan@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2819413002/#ps240001
(title: "rebase.")

[commit-bot: I haz the power, 2017-04-26 01:15:49]

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-04-26 01:24:51]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493169301379060,
"parent_rev": "8b25fd5c093cb78cf8e3bcd3d52d8d58bdf7bce5", "commit_rev":
"f91ad00d0d78fe00a5d1ca4fc50ace6cd63718a7"}

[commit-bot: I haz the power, 2017-04-26 01:25:38]

Description was changed from

==========
[memory-infra] Remove MemoryDumpManagerDelegate

This CL removes the unnecessary Delegate in MemoryDumpManager and just
uses a callback to request global dumps.

BUG=703184
==========

to

==========
[memory-infra] Remove MemoryDumpManagerDelegate

This CL removes the unnecessary Delegate in MemoryDumpManager and just
uses a callback to request global dumps.

BUG=703184

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

[commit-bot: I haz the power, 2017-04-26 01:25:40]

Committed patchset #6 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/f91ad00d0d78fe00a5d1ca4fc50a...