Refactor ownership model for FirstWebContentsProfiler.

Refactor ownership model for FirstWebContentsProfiler.

Previously ChromeBrowserMainExtraPartsMetrics was owning FirstWebContentsProfiler
which would have a reference back to it as a Delegate only to be able to tell
ChromeBrowserMainExtraPartsMetrics to destroy its FirstWebContentsProfiler self
when done (i.e. FirstWebContentsProfiler was essentially managing its own lifetime
but someone else was the explicit owner...).

The only thing that could have made sense was if that guaranteed that the FirstWebContentsProfiler
didn't somehow live beyond its observed WebContents and cause crashes on shutdown but
this already can't be the case as it self-destructs on WebContentsObserver::WebContentsDestroyed().

BUG=Unecessarily complex ownership model.

Committed: https://crrev.com/8335a8869b48e2c5837c12bca370bd44de058c3f
Cr-Commit-Position: refs/heads/master@{#359953}

[gab, 2015-10-20 19:55:59]

gab@chromium.org changed reviewers:
+ rkaplow@chromium.org

[gab, 2015-10-20 19:55:59]

Rob PTAL, thanks :-)!

[rkaplow, 2015-10-20 20:28:02]

Looks like this suggestion came up in the original review:
https://codereview.chromium.org/760763002/

Specifically:
On 2014/11/26 02:24:55, Ilya Sherman wrote:
> This feels pretty hacky.  I wonder if it would be cleaner to just have the
> FirstWebContentsProfiler own its own memory, i.e. call "delete this" when it
> currently calls FirstWebContentsProfilerWantsDestruction().

I don't like the style where objects own their own memory, as I find it prone to
memory leaks. The logic is the same, but I changed the code to use the
delegation pattern, which I assume will be okay by you in terms of not feeling
hacky.

I don't feel strongly about this and wouldn't mind reducing the complexity -
letting isherman@ and erikchen@ know in case they feel against this change

[rkaplow, 2015-10-20 20:28:34]

On 2015/10/20 20:28:02, rkaplow wrote:
> Looks like this suggestion came up in the original review:
> https://codereview.chromium.org/760763002/
> 
> Specifically:
> On 2014/11/26 02:24:55, Ilya Sherman wrote:
> > This feels pretty hacky.  I wonder if it would be cleaner to just have the
> > FirstWebContentsProfiler own its own memory, i.e. call "delete this" when it
> > currently calls FirstWebContentsProfilerWantsDestruction().
> 
> I don't like the style where objects own their own memory, as I find it prone
to
> memory leaks. The logic is the same, but I changed the code to use the
> delegation pattern, which I assume will be okay by you in terms of not feeling
> hacky.
> 
> I don't feel strongly about this and wouldn't mind reducing the complexity -
> letting isherman@ and erikchen@ know in case they feel against this change

To be clear the:
> I don't like the style where objects own their own memory, as I find it prone
to
> memory leaks. The logic is the same, but I changed the code to use the
> delegation pattern, which I assume will be okay by you in terms of not feeling
> hacky.
bit is a quote from the previous review.

[erikchen, 2015-10-20 20:56:09]

On 2015/10/20 20:28:34, rkaplow wrote:
> On 2015/10/20 20:28:02, rkaplow wrote:
> > Looks like this suggestion came up in the original review:
> > https://codereview.chromium.org/760763002/
> > 
> > Specifically:
> > On 2014/11/26 02:24:55, Ilya Sherman wrote:
> > > This feels pretty hacky.  I wonder if it would be cleaner to just have the
> > > FirstWebContentsProfiler own its own memory, i.e. call "delete this" when
it
> > > currently calls FirstWebContentsProfilerWantsDestruction().
> > 
> > I don't like the style where objects own their own memory, as I find it
prone
> to
> > memory leaks. The logic is the same, but I changed the code to use the
> > delegation pattern, which I assume will be okay by you in terms of not
feeling
> > hacky.
> > 
> > I don't feel strongly about this and wouldn't mind reducing the complexity -
> > letting isherman@ and erikchen@ know in case they feel against this change
> 
> To be clear the:
> > I don't like the style where objects own their own memory, as I find it
prone
> to
> > memory leaks. The logic is the same, but I changed the code to use the
> > delegation pattern, which I assume will be okay by you in terms of not
feeling
> > hacky.
> bit is a quote from the previous review.

My feelings about this haven't changed. You can change to a self-ownership
model, and the current logic will still work. My claim is that self-ownership is
more fragile, and that it is easier for future code to cause memory leaks.
Having a well defined ownership chain may result in more code, but also yields a
cleaner destruction code path. One question you may want to ask yourself is:
Does it make sense for a FirstWebContentsProfiler to outlive
ChromeBrowserMainExtraPartsMetric?

This mostly comes down to a preference between two design patterns. rkaplow is
an OWNER of chrome/browser/metrics, so feel free to choose whichever you like
better.

[Ilya Sherman, 2015-10-20 22:14:04]

On 2015/10/20 20:56:09, erikchen wrote:
> On 2015/10/20 20:28:34, rkaplow wrote:
> > On 2015/10/20 20:28:02, rkaplow wrote:
> > > Looks like this suggestion came up in the original review:
> > > https://codereview.chromium.org/760763002/
> > > 
> > > Specifically:
> > > On 2014/11/26 02:24:55, Ilya Sherman wrote:
> > > > This feels pretty hacky.  I wonder if it would be cleaner to just have
the
> > > > FirstWebContentsProfiler own its own memory, i.e. call "delete this"
when
> it
> > > > currently calls FirstWebContentsProfilerWantsDestruction().
> > > 
> > > I don't like the style where objects own their own memory, as I find it
> prone
> > to
> > > memory leaks. The logic is the same, but I changed the code to use the
> > > delegation pattern, which I assume will be okay by you in terms of not
> feeling
> > > hacky.
> > > 
> > > I don't feel strongly about this and wouldn't mind reducing the complexity
-
> > > letting isherman@ and erikchen@ know in case they feel against this change
> > 
> > To be clear the:
> > > I don't like the style where objects own their own memory, as I find it
> prone
> > to
> > > memory leaks. The logic is the same, but I changed the code to use the
> > > delegation pattern, which I assume will be okay by you in terms of not
> feeling
> > > hacky.
> > bit is a quote from the previous review.
> 
> My feelings about this haven't changed. You can change to a self-ownership
> model, and the current logic will still work. My claim is that self-ownership
is
> more fragile, and that it is easier for future code to cause memory leaks.
> Having a well defined ownership chain may result in more code, but also yields
a
> cleaner destruction code path. One question you may want to ask yourself is:
> Does it make sense for a FirstWebContentsProfiler to outlive
> ChromeBrowserMainExtraPartsMetric?
> 
> This mostly comes down to a preference between two design patterns. rkaplow is
> an OWNER of chrome/browser/metrics, so feel free to choose whichever you like
> better.

Not LGTM.  My feelings about this haven't changed either, and I agree with
everything that Erik wrote.

[rkaplow, 2015-10-21 14:29:06]

We'll keep the current implementation. Thanks for looking into it though gab.

[gab, 2015-10-21 15:15:03]

I strongly disagree with the current implementation being better.

The current model is essentially self-ownership except instead of calling
"delete this" the FirstWebContentsProfiler calls
Delegate::ProfilerFinishedCollectingMetrics() which in turn deletes it.

This is an extra layer adding overhead for no benefit.

As for whether it's okay for a FirstWebContentsProfiler to outlive
ChromeBrowserMainExtraPartsMetric, I don't see why not. The
FirstWebContentsProfiler is only reactive to its observed WebContents and can't
possibly run code without being notified by an existing WebContents.

I think this is sufficient in itself to get rid of the Delegate indirection, but
furthermore I think it's also wrong for ChromeBrowserMainExtraPartsMetric to be
the owner here as ChromeBrowserMainExtraPartsMetric should merely set up some
metrics, once FirstWebContentsProfiler is up it only depends on its observed
WebContents and shouldn't be managed by anybody else. By having it self-manage
and destroy itself upon destruction of its WebContents, we essentially have the
observed WebContents as the implicit owner which makes a lot more sense.

tl;dr; ChromeBrowserMainExtraPartsMetric should kick things off but doesn't
belong in the ownership chain (as highlighted today by the fact that it merely
reacts to the FirstWebContentsProfiler telling it to delete it).


PS: Self-ownership is not a hack, it is wildly used, and in this specific case
it's definitely no more dangerous to forget to "delete this" (or to do work
after delete this) than it is to forget (or do work after) calling
ProfilerFinishedCollectingMetrics(). In fact, to me it's a lot more obvious that
no work should be done after a "delete this" than after a call to a Delegate
which will immediately delete you from under yourself...

[gab, 2015-10-22 15:58:56]

ping (see previous reply).

In short, I believe no downside of self-ownership is avoided by the existing
approach, i.e.:
 - Having to explicitly make a call to delete yourself.
 - Having to not do follow-up work after making that call.

The main difference is that "delete this" makes those semantics much clearer to
a reader than a call to a Delegate.

So this CL achieves the exact same in +20 -58 lines (i.e. simpler/more
readable).

I just also made the destructor private to make it impossible for anyone but
itself to delete it.

I'd like to understand what you think the downsides of explicit self-ownership
are compared to the existing approach. I personally don't see any (and am happy
if I've convinced you as well and we can land this :-)).

Thanks,
Gab

[gab, 2015-10-27 15:01:25]

@Ilya: ping on previous messages. I appreciate your care to do a drive-by
notlgtm for code health, but would appreciate a follow-up on my arguments. I
really think this is the right thing for this code (happy to loop in an
impartial third reviewer if you think that's desirable).

Thanks,
Gab

[rkaplow, 2015-10-27 22:17:19]

rkaplow@chromium.org changed reviewers:
+ isherman@chromium.org

[rkaplow, 2015-10-27 22:17:20]

Ilya I'm going to defer to you for the final say on this one since you've done a
lot more chromium work and you are an owner as well

[erikchen, 2015-10-28 18:39:47]

On 2015/10/27 22:17:20, rkaplow wrote:
> Ilya I'm going to defer to you for the final say on this one since you've done
a
> lot more chromium work and you are an owner as well

I do not agree with the statement:
"In short, I believe no downside of self-ownership is avoided by the existing
approach"

Let me provide a concrete example. I am currently trying to write a browser test
that starts a browser, closes it, and waits for all resources to be released.
The goal of my test is to check that there are no leaked Mach ports. As far as I
can tell, I will not be able to write this test, because there are too many
resources whose lifetime is not tied to the existence of a Browser* object, or
any other object I can get a reference to. If FirstWebContentsProfiler is
self-owned, there is no way for any test to control the lifetime of the
resources that it uses. As long as it is owned by ChromeBrowserMainExtraParts
(which I think is the correct ownership model), its lifetime is bounded by the
lifetime of ChromeBrowserMainExtraParts.

So I think the tradeoff is:
Code complexity vs. better ownership model (control of lifetime of resources).

[Ilya Sherman, 2015-10-28 19:11:34]

On 2015/10/27 15:01:25, gab wrote:
> @Ilya: ping on previous messages. I appreciate your care to do a drive-by
> notlgtm for code health, but would appreciate a follow-up on my arguments. I
> really think this is the right thing for this code (happy to loop in an
> impartial third reviewer if you think that's desirable).
> 
> Thanks,
> Gab

Sorry, your emails somehow missed my inbox -- I only got Rob's ping yesterday. 
I'm still in agreement with Erik, though: an explicit ownership model is, in
general, easier to reason about; and has advantages like the one that Erik
mentioned for testing.  I agree with you that on the surface, it seems simpler
for this object to own itself... but this imposes a tax on anyone who tries to
reason about this code as they make changes to it in the future.

All that said, it sounds like your main argument is that this object should be
owned by the WebContents that it observes, rather than by its current owner.  I
could be convinced that that's true.  WDYT of making that ownership relationship
explicit, via WebContentsUserData?

[gab, 2015-10-28 19:29:03]

I don't think the concerns for testing are justified here as the implicit owner
is the WebContents (i.e. its destruction will synchronously cause the
FirstWebContentsProfiler to destroy itself).

I agree that better ownership model is more important than reducing code
complexity in general but what I'm saying is that here I only see more
complexity for the same implicit model.

Also, PS: although it's irrelevant for this discussion, I don't see the purpose
of a test that would make sure all resources are freed on browser shutdown.
There is a famous analogy for this which says that this is like "cleaning the
carpets before burning down the building", essentially the only point of
following a shutdown path is to make sure the things that need to be persisted
to permanent storage are, after that there is no point freeing every single
allocation (in fact on Windows we explicitly terminate process after persisting
a few things as once you own no visible windows, Windows doesn't guarantee your
lifetime in any way [1]).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...

[Ilya Sherman, 2015-10-28 19:31:23]

On 2015/10/28 19:29:03, gab wrote:
> I don't think the concerns for testing are justified here as the implicit
owner
> is the WebContents (i.e. its destruction will synchronously cause the
> FirstWebContentsProfiler to destroy itself).
> 
> I agree that better ownership model is more important than reducing code
> complexity in general but what I'm saying is that here I only see more
> complexity for the same implicit model.
> 
> Also, PS: although it's irrelevant for this discussion, I don't see the
purpose
> of a test that would make sure all resources are freed on browser shutdown.
> There is a famous analogy for this which says that this is like "cleaning the
> carpets before burning down the building", essentially the only point of
> following a shutdown path is to make sure the things that need to be persisted
> to permanent storage are, after that there is no point freeing every single
> allocation (in fact on Windows we explicitly terminate process after
persisting
> a few things as once you own no visible windows, Windows doesn't guarantee
your
> lifetime in any way [1]).
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...

You haven't addressed my question about moving from an implicit ownership model
to an explicit one.  If you'd really like to clean up the ownership model here,
that's the approach that I would encourage.

[gab, 2015-10-28 19:41:10]

On 2015/10/28 19:31:23, Ilya Sherman wrote:
> On 2015/10/28 19:29:03, gab wrote:
> > I don't think the concerns for testing are justified here as the implicit
> owner
> > is the WebContents (i.e. its destruction will synchronously cause the
> > FirstWebContentsProfiler to destroy itself).
> > 
> > I agree that better ownership model is more important than reducing code
> > complexity in general but what I'm saying is that here I only see more
> > complexity for the same implicit model.
> > 
> > Also, PS: although it's irrelevant for this discussion, I don't see the
> purpose
> > of a test that would make sure all resources are freed on browser shutdown.
> > There is a famous analogy for this which says that this is like "cleaning
the
> > carpets before burning down the building", essentially the only point of
> > following a shutdown path is to make sure the things that need to be
persisted
> > to permanent storage are, after that there is no point freeing every single
> > allocation (in fact on Windows we explicitly terminate process after
> persisting
> > a few things as once you own no visible windows, Windows doesn't guarantee
> your
> > lifetime in any way [1]).
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...
> 
> You haven't addressed my question about moving from an implicit ownership
model
> to an explicit one.  If you'd really like to clean up the ownership model
here,
> that's the approach that I would encourage.

In practice, the FirstWebContentsProfiler will most often either abandon or
complete profiling before the WebContents is destroyed.

Therefore, I don't think making the WebContents the explicit owner is correct
either as in most cases that's overkill, what we want is for the WebContents'
lifetime to be an upper-bound for the FirstWebContentsProfiler's lifetime.

I think self-ownership plus bounding lifetime to the WebContents such that it
never lives past it is the correct approach (i.e. delete self when done or when
scope (observed WebContents) expires).

This is already the case, but currently involves an unnecessary object in the
ownership model.

[Ilya Sherman, 2015-10-28 19:57:03]

On 2015/10/28 19:41:10, gab wrote:
> On 2015/10/28 19:31:23, Ilya Sherman wrote:
> > On 2015/10/28 19:29:03, gab wrote:
> > > I don't think the concerns for testing are justified here as the implicit
> > owner
> > > is the WebContents (i.e. its destruction will synchronously cause the
> > > FirstWebContentsProfiler to destroy itself).
> > > 
> > > I agree that better ownership model is more important than reducing code
> > > complexity in general but what I'm saying is that here I only see more
> > > complexity for the same implicit model.
> > > 
> > > Also, PS: although it's irrelevant for this discussion, I don't see the
> > purpose
> > > of a test that would make sure all resources are freed on browser
shutdown.
> > > There is a famous analogy for this which says that this is like "cleaning
> the
> > > carpets before burning down the building", essentially the only point of
> > > following a shutdown path is to make sure the things that need to be
> persisted
> > > to permanent storage are, after that there is no point freeing every
single
> > > allocation (in fact on Windows we explicitly terminate process after
> > persisting
> > > a few things as once you own no visible windows, Windows doesn't guarantee
> > your
> > > lifetime in any way [1]).
> > > 
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...
> > 
> > You haven't addressed my question about moving from an implicit ownership
> model
> > to an explicit one.  If you'd really like to clean up the ownership model
> here,
> > that's the approach that I would encourage.
> 
> In practice, the FirstWebContentsProfiler will most often either abandon or
> complete profiling before the WebContents is destroyed.
> 
> Therefore, I don't think making the WebContents the explicit owner is correct
> either as in most cases that's overkill, what we want is for the WebContents'
> lifetime to be an upper-bound for the FirstWebContentsProfiler's lifetime.
> 
> I think self-ownership plus bounding lifetime to the WebContents such that it
> never lives past it is the correct approach (i.e. delete self when done or
when
> scope (observed WebContents) expires).
> 
> This is already the case, but currently involves an unnecessary object in the
> ownership model.

Okay, it seems like we just fundamentally disagree.  Feel free to loop in an
additional reviewer if you'd like to.  IMO it might not be worth spending too
much time on this though -- I imagine it's not causing you or anyone else any
daily pain.

[gab, 2015-11-09 20:43:51]

On 2015/10/28 19:57:03, Ilya Sherman wrote:
> On 2015/10/28 19:41:10, gab wrote:
> > On 2015/10/28 19:31:23, Ilya Sherman wrote:
> > > On 2015/10/28 19:29:03, gab wrote:
> > > > I don't think the concerns for testing are justified here as the
implicit
> > > owner
> > > > is the WebContents (i.e. its destruction will synchronously cause the
> > > > FirstWebContentsProfiler to destroy itself).
> > > > 
> > > > I agree that better ownership model is more important than reducing code
> > > > complexity in general but what I'm saying is that here I only see more
> > > > complexity for the same implicit model.
> > > > 
> > > > Also, PS: although it's irrelevant for this discussion, I don't see the
> > > purpose
> > > > of a test that would make sure all resources are freed on browser
> shutdown.
> > > > There is a famous analogy for this which says that this is like
"cleaning
> > the
> > > > carpets before burning down the building", essentially the only point of
> > > > following a shutdown path is to make sure the things that need to be
> > persisted
> > > > to permanent storage are, after that there is no point freeing every
> single
> > > > allocation (in fact on Windows we explicitly terminate process after
> > > persisting
> > > > a few things as once you own no visible windows, Windows doesn't
guarantee
> > > your
> > > > lifetime in any way [1]).
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...
> > > 
> > > You haven't addressed my question about moving from an implicit ownership
> > model
> > > to an explicit one.  If you'd really like to clean up the ownership model
> > here,
> > > that's the approach that I would encourage.
> > 
> > In practice, the FirstWebContentsProfiler will most often either abandon or
> > complete profiling before the WebContents is destroyed.
> > 
> > Therefore, I don't think making the WebContents the explicit owner is
correct
> > either as in most cases that's overkill, what we want is for the
WebContents'
> > lifetime to be an upper-bound for the FirstWebContentsProfiler's lifetime.
> > 
> > I think self-ownership plus bounding lifetime to the WebContents such that
it
> > never lives past it is the correct approach (i.e. delete self when done or
> when
> > scope (observed WebContents) expires).
> > 
> > This is already the case, but currently involves an unnecessary object in
the
> > ownership model.
> 
> Okay, it seems like we just fundamentally disagree.  Feel free to loop in an
> additional reviewer if you'd like to.  IMO it might not be worth spending too
> much time on this though -- I imagine it's not causing you or anyone else any
> daily pain.

FYI, speaking of shutdown only leaks by design in Chrome, see
http://crbug.com/371974.

[gab, 2015-11-09 20:45:27]

On 2015/11/09 20:43:51, gab wrote:
> On 2015/10/28 19:57:03, Ilya Sherman wrote:
> > On 2015/10/28 19:41:10, gab wrote:
> > > On 2015/10/28 19:31:23, Ilya Sherman wrote:
> > > > On 2015/10/28 19:29:03, gab wrote:
> > > > > I don't think the concerns for testing are justified here as the
> implicit
> > > > owner
> > > > > is the WebContents (i.e. its destruction will synchronously cause the
> > > > > FirstWebContentsProfiler to destroy itself).
> > > > > 
> > > > > I agree that better ownership model is more important than reducing
code
> > > > > complexity in general but what I'm saying is that here I only see more
> > > > > complexity for the same implicit model.
> > > > > 
> > > > > Also, PS: although it's irrelevant for this discussion, I don't see
the
> > > > purpose
> > > > > of a test that would make sure all resources are freed on browser
> > shutdown.
> > > > > There is a famous analogy for this which says that this is like
> "cleaning
> > > the
> > > > > carpets before burning down the building", essentially the only point
of
> > > > > following a shutdown path is to make sure the things that need to be
> > > persisted
> > > > > to permanent storage are, after that there is no point freeing every
> > single
> > > > > allocation (in fact on Windows we explicitly terminate process after
> > > > persisting
> > > > > a few things as once you own no visible windows, Windows doesn't
> guarantee
> > > > your
> > > > > lifetime in any way [1]).
> > > > > 
> > > > > [1]
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...
> > > > 
> > > > You haven't addressed my question about moving from an implicit
ownership
> > > model
> > > > to an explicit one.  If you'd really like to clean up the ownership
model
> > > here,
> > > > that's the approach that I would encourage.
> > > 
> > > In practice, the FirstWebContentsProfiler will most often either abandon
or
> > > complete profiling before the WebContents is destroyed.
> > > 
> > > Therefore, I don't think making the WebContents the explicit owner is
> correct
> > > either as in most cases that's overkill, what we want is for the
> WebContents'
> > > lifetime to be an upper-bound for the FirstWebContentsProfiler's lifetime.
> > > 
> > > I think self-ownership plus bounding lifetime to the WebContents such that
> it
> > > never lives past it is the correct approach (i.e. delete self when done or
> > when
> > > scope (observed WebContents) expires).
> > > 
> > > This is already the case, but currently involves an unnecessary object in
> the
> > > ownership model.
> > 
> > Okay, it seems like we just fundamentally disagree.  Feel free to loop in an
> > additional reviewer if you'd like to.  IMO it might not be worth spending
too
> > much time on this though -- I imagine it's not causing you or anyone else
any
> > daily pain.
> 
> FYI, speaking of shutdown only leaks by design in Chrome, see
> http://crbug.com/371974.

tl;dr; PostTaskAndReplyWithResult leaks by design (only deletes its Callbacks
after they were both ran -- i.e. never if the queue is terminated)

[Ilya Sherman, 2015-11-09 22:51:47]

On 2015/11/09 20:45:27, gab wrote:
> On 2015/11/09 20:43:51, gab wrote:
> > On 2015/10/28 19:57:03, Ilya Sherman wrote:
> > > On 2015/10/28 19:41:10, gab wrote:
> > > > On 2015/10/28 19:31:23, Ilya Sherman wrote:
> > > > > On 2015/10/28 19:29:03, gab wrote:
> > > > > > I don't think the concerns for testing are justified here as the
> > implicit
> > > > > owner
> > > > > > is the WebContents (i.e. its destruction will synchronously cause
the
> > > > > > FirstWebContentsProfiler to destroy itself).
> > > > > > 
> > > > > > I agree that better ownership model is more important than reducing
> code
> > > > > > complexity in general but what I'm saying is that here I only see
more
> > > > > > complexity for the same implicit model.
> > > > > > 
> > > > > > Also, PS: although it's irrelevant for this discussion, I don't see
> the
> > > > > purpose
> > > > > > of a test that would make sure all resources are freed on browser
> > > shutdown.
> > > > > > There is a famous analogy for this which says that this is like
> > "cleaning
> > > > the
> > > > > > carpets before burning down the building", essentially the only
point
> of
> > > > > > following a shutdown path is to make sure the things that need to be
> > > > persisted
> > > > > > to permanent storage are, after that there is no point freeing every
> > > single
> > > > > > allocation (in fact on Windows we explicitly terminate process after
> > > > > persisting
> > > > > > a few things as once you own no visible windows, Windows doesn't
> > guarantee
> > > > > your
> > > > > > lifetime in any way [1]).
> > > > > > 
> > > > > > [1]
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...
> > > > > 
> > > > > You haven't addressed my question about moving from an implicit
> ownership
> > > > model
> > > > > to an explicit one.  If you'd really like to clean up the ownership
> model
> > > > here,
> > > > > that's the approach that I would encourage.
> > > > 
> > > > In practice, the FirstWebContentsProfiler will most often either abandon
> or
> > > > complete profiling before the WebContents is destroyed.
> > > > 
> > > > Therefore, I don't think making the WebContents the explicit owner is
> > correct
> > > > either as in most cases that's overkill, what we want is for the
> > WebContents'
> > > > lifetime to be an upper-bound for the FirstWebContentsProfiler's
lifetime.
> > > > 
> > > > I think self-ownership plus bounding lifetime to the WebContents such
that
> > it
> > > > never lives past it is the correct approach (i.e. delete self when done
or
> > > when
> > > > scope (observed WebContents) expires).
> > > > 
> > > > This is already the case, but currently involves an unnecessary object
in
> > the
> > > > ownership model.
> > > 
> > > Okay, it seems like we just fundamentally disagree.  Feel free to loop in
an
> > > additional reviewer if you'd like to.  IMO it might not be worth spending
> too
> > > much time on this though -- I imagine it's not causing you or anyone else
> any
> > > daily pain.
> > 
> > FYI, speaking of shutdown only leaks by design in Chrome, see
> > http://crbug.com/371974.
> 
> tl;dr; PostTaskAndReplyWithResult leaks by design (only deletes its Callbacks
> after they were both ran -- i.e. never if the queue is terminated)

I agree that we often do (and should) use shutdown leaks by design in Chromium,
but that's not really what's motiving my objection to the self-ownership design.

[gab, 2015-11-10 01:33:26]

On 2015/11/09 22:51:47, Ilya Sherman wrote:
> On 2015/11/09 20:45:27, gab wrote:
> > On 2015/11/09 20:43:51, gab wrote:
> > > On 2015/10/28 19:57:03, Ilya Sherman wrote:
> > > > On 2015/10/28 19:41:10, gab wrote:
> > > > > On 2015/10/28 19:31:23, Ilya Sherman wrote:
> > > > > > On 2015/10/28 19:29:03, gab wrote:
> > > > > > > I don't think the concerns for testing are justified here as the
> > > implicit
> > > > > > owner
> > > > > > > is the WebContents (i.e. its destruction will synchronously cause
> the
> > > > > > > FirstWebContentsProfiler to destroy itself).
> > > > > > > 
> > > > > > > I agree that better ownership model is more important than
reducing
> > code
> > > > > > > complexity in general but what I'm saying is that here I only see
> more
> > > > > > > complexity for the same implicit model.
> > > > > > > 
> > > > > > > Also, PS: although it's irrelevant for this discussion, I don't
see
> > the
> > > > > > purpose
> > > > > > > of a test that would make sure all resources are freed on browser
> > > > shutdown.
> > > > > > > There is a famous analogy for this which says that this is like
> > > "cleaning
> > > > > the
> > > > > > > carpets before burning down the building", essentially the only
> point
> > of
> > > > > > > following a shutdown path is to make sure the things that need to
be
> > > > > persisted
> > > > > > > to permanent storage are, after that there is no point freeing
every
> > > > single
> > > > > > > allocation (in fact on Windows we explicitly terminate process
after
> > > > > > persisting
> > > > > > > a few things as once you own no visible windows, Windows doesn't
> > > guarantee
> > > > > > your
> > > > > > > lifetime in any way [1]).
> > > > > > > 
> > > > > > > [1]
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...
> > > > > > 
> > > > > > You haven't addressed my question about moving from an implicit
> > ownership
> > > > > model
> > > > > > to an explicit one.  If you'd really like to clean up the ownership
> > model
> > > > > here,
> > > > > > that's the approach that I would encourage.
> > > > > 
> > > > > In practice, the FirstWebContentsProfiler will most often either
abandon
> > or
> > > > > complete profiling before the WebContents is destroyed.
> > > > > 
> > > > > Therefore, I don't think making the WebContents the explicit owner is
> > > correct
> > > > > either as in most cases that's overkill, what we want is for the
> > > WebContents'
> > > > > lifetime to be an upper-bound for the FirstWebContentsProfiler's
> lifetime.
> > > > > 
> > > > > I think self-ownership plus bounding lifetime to the WebContents such
> that
> > > it
> > > > > never lives past it is the correct approach (i.e. delete self when
done
> or
> > > > when
> > > > > scope (observed WebContents) expires).
> > > > > 
> > > > > This is already the case, but currently involves an unnecessary object
> in
> > > the
> > > > > ownership model.
> > > > 
> > > > Okay, it seems like we just fundamentally disagree.  Feel free to loop
in
> an
> > > > additional reviewer if you'd like to.  IMO it might not be worth
spending
> > too
> > > > much time on this though -- I imagine it's not causing you or anyone
else
> > any
> > > > daily pain.
> > > 
> > > FYI, speaking of shutdown only leaks by design in Chrome, see
> > > http://crbug.com/371974.
> > 
> > tl;dr; PostTaskAndReplyWithResult leaks by design (only deletes its
Callbacks
> > after they were both ran -- i.e. never if the queue is terminated)
> 
> I agree that we often do (and should) use shutdown leaks by design in
Chromium,
> but that's not really what's motiving my objection to the self-ownership
design.

Hmmm okay, from the discussion I'd gathered that the main point of contention
was Erik's mention of the inability to test for no leaks when opening and
closing the browser in a browser test.
Besides that the current model is exactly self-ownership but disguised IMO (i.e.
same 2 downsides: (1) need to remember to call "deleter" and (2) must not do
work after calling "deleter" -- except that the current model doesn't make that
obvious to a reader like a direct "delete this" would).

[Alexei Svitkine (slow), 2015-11-13 17:28:59]

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

[Alexei Svitkine (slow), 2015-11-13 17:29:00]

I was asked to provide a second opinion here.

Looking at the diff and the ownership model, I actually prefer this change over
the original. It makes the code much cleaner and concise - and also easier to
reason about - because the ownership logic is self-contained in
FirstWebContentsProfiler.

Given that the object listens for WebContentsDestroyed() to destroy itself,
there's also no lifetime issues.

I can see the other approach being useful if this class depended on a bunch of
different things and used the ChromeBrowserMainExtraPartsMetrics' lifetime as a
proxy for "during that classes' lifetime all my dependencies are valid". But
that's not the case now - it only cares about a WebContents whose life time it
already monitors - and I don't think the extra complexity of the current code is
worth it.

So given the above, this LGTM from my point of view.

https://codereview.chromium.org/1415773004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1415773004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:107: new
FirstWebContentsProfiler(web_contents);
Add a comment about the lifetime here.

[gab, 2015-11-13 19:14:53]

Patchset #5 (id:80001) has been deleted

[gab, 2015-11-13 19:22:21]

Rebased on trunk and added requested comment.

@Ilya: ok?

Thanks,
Gab

https://codereview.chromium.org/1415773004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1415773004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:107: new
FirstWebContentsProfiler(web_contents);
On 2015/11/13 17:29:00, Alexei Svitkine (slow) wrote:
> Add a comment about the lifetime here.

Good point, done.

[Alexei Svitkine (slow), 2015-11-13 19:24:33]

lgtm

https://codereview.chromium.org/1415773004/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/first_web_contents_profiler.h (right):

https://codereview.chromium.org/1415773004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/first_web_contents_profiler.h:63: // // Logs
|finish_reason| to UMA and deletes this FirstWebContentsProfiler.
Nit: Extra "// "

[gab, 2015-11-13 19:29:05]

https://codereview.chromium.org/1415773004/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/first_web_contents_profiler.h (right):

https://codereview.chromium.org/1415773004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/first_web_contents_profiler.h:63: // // Logs
|finish_reason| to UMA and deletes this FirstWebContentsProfiler.
On 2015/11/13 19:24:33, Alexei Svitkine (slow) wrote:
> Nit: Extra "// "

Oops, good catch, manual merge mistake.. Done.

[gab, 2015-11-13 19:29:48]

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

[commit-bot: I haz the power, 2015-11-13 19:30:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415773004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415773004/140001

[Ilya Sherman, 2015-11-13 20:13:34]

On 2015/11/13 19:22:21, gab wrote:
> Rebased on trunk and added requested comment.
> 
> @Ilya: ok?

I still disagree with this approach, but I don't feel strongly enough to keep
the discussion/debate going.  LGTM.

[gab, 2015-11-13 20:56:55]

The CQ bit was unchecked by gab@chromium.org

[gab, 2015-11-13 20:57:07]

On 2015/11/13 20:13:34, Ilya Sherman wrote:
> On 2015/11/13 19:22:21, gab wrote:
> > Rebased on trunk and added requested comment.
> > 
> > @Ilya: ok?
> 
> I still disagree with this approach, but I don't feel strongly enough to keep
> the discussion/debate going.  LGTM.

Thank you.

[gab, 2015-11-13 20:57:16]

The CQ bit was checked by gab@chromium.org

[gab, 2015-11-13 20:57:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1415773004/#ps140001
(title: "- //")

[commit-bot: I haz the power, 2015-11-13 20:58:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415773004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415773004/140001

[commit-bot: I haz the power, 2015-11-13 21:43:43]

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

[commit-bot: I haz the power, 2015-11-13 21:43:44]

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

[gab, 2015-11-16 16:37:57]

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

[commit-bot: I haz the power, 2015-11-16 16:38:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415773004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415773004/160001

[gab, 2015-11-16 17:22:06]

On 2015/11/13 21:43:44, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Cool, this new approach caught an existing "leak" (well except that it was
eventually deleted by its scoped_ptr ownership on shutdown and thus uncaught in
previous approach) :-)!

http://crbug.com/556564 and fixed in https://codereview.chromium.org/1449933002/
:-).

Will land this when that precursor fix lands (fix to be merged while this is
not).

[commit-bot: I haz the power, 2015-11-16 17:26:03]

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

[commit-bot: I haz the power, 2015-11-16 17:26:04]

Dry run: This issue passed the CQ dry run.

[gab, 2015-11-16 23:33:48]

The CQ bit was checked by gab@chromium.org

[gab, 2015-11-16 23:33:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1415773004/#ps160001
(title: "rebase on leak fix (https://codereview.chromium.org/1449933002)")

[commit-bot: I haz the power, 2015-11-16 23:34:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415773004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415773004/160001

[commit-bot: I haz the power, 2015-11-16 23:42:47]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-11-16 23:43:47]

Patchset 8 (id:??) landed as
https://crrev.com/8335a8869b48e2c5837c12bca370bd44de058c3f
Cr-Commit-Position: refs/heads/master@{#359953}