Change ownership of BackgroundSyncServiceImpl to BackgroundSyncContextImpl

Change ownership of BackgroundSyncServiceImpl to BackgroundSyncContextImpl

Previously BackgroundSyncServiceImpl was deleted by mojo very late on
shutdown (on messageloop destruction). This is way too late. Instead, let the BackgroundSyncContextImpl own them.

BUG=517122
Committed: https://crrev.com/3440155cd4d8bb061f8261f6e01564804bb9f155
Cr-Commit-Position: refs/heads/master@{#343215}

[jkarlin, 2015-08-11 15:37:14]

Patchset #2 (id:20001) has been deleted

[jkarlin, 2015-08-11 15:43:25]

jkarlin@chromium.org changed reviewers:
+ iclelland@chromium.org

[jkarlin, 2015-08-11 15:43:25]

iclelland, PTAL, thanks!

[iclelland, 2015-08-12 04:01:09]

Just a couple of questions for you, but otherwise lgtm

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_context_impl.cc (right):

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_context_impl.cc:21:
DCHECK(!background_sync_manager_);
Is this really just asserting that ShutdownOnIO is always called?

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_context_impl.cc:59: void
BackgroundSyncContextImpl::CreateService(
Why a static method that takes an instance as the first parameter, rather than
an instance method?

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_context_impl.cc:73: if
(!background_sync_manager_)
Is this is going to fail silently, should we document when that would occur? Is
this code just for unit tests?

[jkarlin, 2015-08-12 11:50:11]

Thanks!

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_context_impl.cc (right):

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_context_impl.cc:21:
DCHECK(!background_sync_manager_);
On 2015/08/12 04:01:09, iclelland wrote:
> Is this really just asserting that ShutdownOnIO is always called?

Yes, rather than delete everything in either scenario we're enforcing that
shutdown is called first before deleting. I've added a comment in the header
stating that shutdown must be called.

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_context_impl.cc:59: void
BackgroundSyncContextImpl::CreateService(
On 2015/08/12 04:01:08, iclelland wrote:
> Why a static method that takes an instance as the first parameter, rather than
> an instance method?

Good point. Done.

https://codereview.chromium.org/1283703003/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_context_impl.cc:73: if
(!background_sync_manager_)
On 2015/08/12 04:01:08, iclelland wrote:
> Is this is going to fail silently, should we document when that would occur?
Is
> this code just for unit tests?

It can happen in the wild. Documented and log message added.

[jkarlin, 2015-08-12 11:54:04]

jkarlin@chromium.org changed reviewers:
+ davidben@chromium.org, michaeln@chromium.org

[jkarlin, 2015-08-12 11:54:04]

michaeln@ PTAL at everything for committer approval, thanks!

davidben@ PTAL at render_process_host_impl, thanks! (that particular change is
necessary because the render process host outlives the storage partition, at
least in browser tests)

[michaeln, 2015-08-12 20:14:55]

lgtm

[jkarlin, 2015-08-13 12:02:47]

davidben@, PTAL Thanks!

[davidben, 2015-08-13 13:36:30]

davidben@chromium.org changed reviewers:
+ creis@chromium.org

[davidben, 2015-08-13 13:36:31]

> davidben@ PTAL at render_process_host_impl, thanks! (that particular change is
> necessary because the render process host outlives the storage partition, at
> least in browser tests)

Eh? That seems wrong... the RenderProcessHost has a raw pointer to the
StoragePartition and nothing nulls it as far as I can tell. If the RPH outlives
it, that suggests there's already a problem.

+creis who hopefully knows more about this area than I do.

[jkarlin, 2015-08-13 13:55:25]

On 2015/08/13 13:36:31, David Benjamin wrote:
> > davidben@ PTAL at render_process_host_impl, thanks! (that particular change
is
> > necessary because the render process host outlives the storage partition, at
> > least in browser tests)
> 
> Eh? That seems wrong... the RenderProcessHost has a raw pointer to the
> StoragePartition and nothing nulls it as far as I can tell. If the RPH
outlives
> it, that suggests there's already a problem.
> 
> +creis who hopefully knows more about this area than I do.

Ah, excellent point. It appears to be a problem in my particular browser test,
so a separate bug. Removing the change to this file. Thanks David!

[jkarlin, 2015-08-13 13:57:46]

On 2015/08/13 13:55:25, jkarlin wrote:
> On 2015/08/13 13:36:31, David Benjamin wrote:
> > > davidben@ PTAL at render_process_host_impl, thanks! (that particular
change
> is
> > > necessary because the render process host outlives the storage partition,
at
> > > least in browser tests)
> > 
> > Eh? That seems wrong... the RenderProcessHost has a raw pointer to the
> > StoragePartition and nothing nulls it as far as I can tell. If the RPH
> outlives
> > it, that suggests there's already a problem.
> > 
> > +creis who hopefully knows more about this area than I do.
> 
> Ah, excellent point. It appears to be a problem in my particular browser test,
> so a separate bug. Removing the change to this file. Thanks David!

There is still a minor change to the file, so PTAL.

[davidben, 2015-08-13 14:54:30]

https://codereview.chromium.org/1283703003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_context_impl.cc (right):

https://codereview.chromium.org/1283703003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_context_impl.cc:59:
services_.erase(service);
I don't think it can actually make a difference here, but this really should
happen in the other order. Using a freed pointer is scary.

https://codereview.chromium.org/1283703003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_context_impl.cc:89: new
BackgroundSyncServiceImpl(make_scoped_refptr(this), request.Pass()));
Is this not a reference cycle?

[jkarlin, 2015-08-13 15:23:03]

Patchset #6 (id:120001) has been deleted

[jkarlin, 2015-08-13 15:24:27]

Thanks, PTAL david!

https://codereview.chromium.org/1283703003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_context_impl.cc (right):

https://codereview.chromium.org/1283703003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_context_impl.cc:59:
services_.erase(service);
On 2015/08/13 14:54:30, David Benjamin wrote:
> I don't think it can actually make a difference here, but this really should
> happen in the other order. Using a freed pointer is scary.

Done.

https://codereview.chromium.org/1283703003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_context_impl.cc:89: new
BackgroundSyncServiceImpl(make_scoped_refptr(this), request.Pass()));
On 2015/08/13 14:54:30, David Benjamin wrote:
> Is this not a reference cycle?

*hangs head in shame*. Fixed.

[davidben, 2015-08-13 15:49:18]

lgtm. I'm not really familiar with Mojo and assume the other two reviewers
covered that. (OWNERS-wise I'm just needed for the one file anyway.)

https://codereview.chromium.org/1283703003/diff/140001/content/browser/backgr...
File content/browser/background_sync/background_sync_context_impl.h (right):

https://codereview.chromium.org/1283703003/diff/140001/content/browser/backgr...
content/browser/background_sync/background_sync_context_impl.h:41: // Called by
BackgroundSyncServciceImpl objects so that they can
BackgroundSyncServiceImpl

[jkarlin, 2015-08-13 15:51:21]

Thanks!

https://codereview.chromium.org/1283703003/diff/140001/content/browser/backgr...
File content/browser/background_sync/background_sync_context_impl.h (right):

https://codereview.chromium.org/1283703003/diff/140001/content/browser/backgr...
content/browser/background_sync/background_sync_context_impl.h:41: // Called by
BackgroundSyncServciceImpl objects so that they can
On 2015/08/13 15:49:18, David Benjamin wrote:
> BackgroundSyncServiceImpl

Done.

[jkarlin, 2015-08-13 15:51:24]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-08-13 15:51:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, michaeln@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1283703003/#ps160001
(title: "Fix typo")

[commit-bot: I haz the power, 2015-08-13 15:51:56]

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

[commit-bot: I haz the power, 2015-08-13 16:34:37]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-08-13 16:35:24]

Patchset 7 (id:??) landed as
https://crrev.com/3440155cd4d8bb061f8261f6e01564804bb9f155
Cr-Commit-Position: refs/heads/master@{#343215}