Remove stl_util's deletion function use from media galleries.

Remove stl_util's deletion function use from media galleries.

BUG=555865, 659145
Committed: https://crrev.com/f925a4c006162020961ac047a98a5cef5ca13c23
Cr-Commit-Position: refs/heads/master@{#429446}

[Avi (use Gerrit), 2016-10-25 15:00:29]

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

[commit-bot: I haz the power, 2016-10-25 15:00:41]

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

[Avi (use Gerrit), 2016-10-25 15:02:51]

avi@chromium.org changed reviewers:
+ tommycli@chromium.org

[Avi (use Gerrit), 2016-10-25 15:02:52]

Tommy, can you advise here? There's some definite weirdness around the
RenderProcessHost use. If we're holding them in a container that we call
STLDeleteValues on, then they're owned, yet when we remove them we abandon them.
Who is discarding them? (Someone is, because if we discard them we get a
double-delete.) What can we do to make things clearer here?

[commit-bot: I haz the power, 2016-10-25 15:48:50]

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

[commit-bot: I haz the power, 2016-10-25 15:48:50]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-10-25 16:45:43]

On 2016/10/25 15:02:52, Avi wrote:
> Tommy, can you advise here? There's some definite weirdness around the
> RenderProcessHost use. If we're holding them in a container that we call
> STLDeleteValues on, then they're owned, yet when we remove them we abandon
them.
> Who is discarding them? (Someone is, because if we discard them we get a
> double-delete.) What can we do to make things clearer here?

Your changes look fine to me. Can you point me to where they are removed from
the map without deleting them?

[Avi (use Gerrit), 2016-10-25 17:14:18]

On 2016/10/25 16:45:43, tommycli wrote:
> On 2016/10/25 15:02:52, Avi wrote:
> > Tommy, can you advise here? There's some definite weirdness around the
> > RenderProcessHost use. If we're holding them in a container that we call
> > STLDeleteValues on, then they're owned, yet when we remove them we abandon
> them.
> > Who is discarding them? (Someone is, because if we discard them we get a
> > double-delete.) What can we do to make things clearer here?
> 
> Your changes look fine to me. Can you point me to where they are removed from
> the map without deleting them?

In the unit test file I turned ReleaseRPH into an extraction and returning the
unique_ptr, and on lines 455 and 461 I'm calling .release() on them.

What is the ownership here? Why do we take those RPHs out of an owned set and
set them free, yet have someone else delete them? The ownership here is
something that I don't understand.

[tommycli, 2016-10-25 17:56:49]

Okay I've dug into the code and here's my conclusions:

RPH is a self-deleting object. The RPH Factory keeps a map of Profile->RPH, and
when new WebContents are created, a new SiteInstance is created, which asks for
the RPH associated with the profile.

The SiteInstance doesn't own the RPH, but every time a listener detaches from
RPH, RPH calls its CleanUp method, which will delete itself if there are no
other listeners.

I'm a bit fuzzy on how the cleanup happens exactly, but if you add a stacktrace
to RPH::Cleanup, you can probably find out.

lgtm with the below nit:

https://codereview.chromium.org/2448183002/diff/1/chrome/browser/media_galler...
File chrome/browser/media_galleries/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/2448183002/diff/1/chrome/browser/media_galler...
chrome/browser/media_galleries/media_file_system_registry_unittest.cc:201:
std::unique_ptr<content::MockRenderProcessHost> ReleaseRPH(
returning a unique_ptr here may make the caller believe that it now owns the
RPH.

Instead, since the intent of the function is to set RPH free and allow it to
delete itself, I think it should return a raw pointer.

[Avi (use Gerrit), 2016-10-25 19:51:11]

On 2016/10/25 17:56:49, tommycli wrote:
> RPH is a self-deleting object. The RPH Factory keeps a map of Profile->RPH,
and
> when new WebContents are created, a new SiteInstance is created, which asks
for
> the RPH associated with the profile.
> 
> The SiteInstance doesn't own the RPH, but every time a listener detaches from
> RPH, RPH calls its CleanUp method, which will delete itself if there are no
> other listeners.

So should the MockProfileSharedRenderProcessHostFactory have been calling
STLDeleteValues on them? Should the rph_map_ be owning them?

[Avi (use Gerrit), 2016-11-02 22:10:12]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-11-02 22:10:12]

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

[commit-bot: I haz the power, 2016-11-02 22:10:50]

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

[tommycli, 2016-11-02 22:31:18]

On 2016/10/25 19:51:11, Avi wrote:
> On 2016/10/25 17:56:49, tommycli wrote:
> > RPH is a self-deleting object. The RPH Factory keeps a map of Profile->RPH,
> and
> > when new WebContents are created, a new SiteInstance is created, which asks
> for
> > the RPH associated with the profile.
> > 
> > The SiteInstance doesn't own the RPH, but every time a listener detaches
from
> > RPH, RPH calls its CleanUp method, which will delete itself if there are no
> > other listeners.
> 
> So should the MockProfileSharedRenderProcessHostFactory have been calling
> STLDeleteValues on them? Should the rph_map_ be owning them?

Sorry I didn't respond: Yeah... it's not really clear that rph_map_ should have
been owning them. I would say: If no WebContents were ever created, it would
have been important for rph_map_ to own them and delete them.

But since they are created -- it's not really necessary for rph_map_ to take
ownership. In either case - I'd vote to preserve the existing behavior, since
we're not really trying to refactor the unit test logic at this time.

Patch is still lgtm as is.

[commit-bot: I haz the power, 2016-11-02 23:21:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-02 23:24:27]

Description was changed from

==========
Remove stl_util's deletion function use from media galleries.

BUG=555865, 659145
==========

to

==========
Remove stl_util's deletion function use from media galleries.

BUG=555865, 659145

Committed: https://crrev.com/f925a4c006162020961ac047a98a5cef5ca13c23
Cr-Commit-Position: refs/heads/master@{#429446}
==========

[commit-bot: I haz the power, 2016-11-02 23:24:27]

Patchset 2 (id:??) landed as
https://crrev.com/f925a4c006162020961ac047a98a5cef5ca13c23
Cr-Commit-Position: refs/heads/master@{#429446}