BackgroundSyncManager tracks client registrations

For the upcoming "registration.done" feature the BackgroundSyncManager's clients will want to know about the state of registrations even after the BackgroundSyncManager has finished firing them. To prepare for that, this CL does the following:

1) BackgroundSyncManager now internally reference counts each registration it exposes to the client via RefCountedRegistration so that the client can reference a completed registration

2) BackgroundSyncManager returns BackgroundSyncRegistrationHandle to clients, which call BackgroundSyncManager::ReleaseRegistrationHandle when deleted (ensuring that all client-exposed registrations are released).

3) BackgroundSyncServiceImpl holds onto Client registrations until the renderer has signaled that it is done with them.

4) Moved Unregister to the new BackgroundSyncRegistrationHandle class (a la the javascript API). This makes it clear when it's safe for the client to delete its ClientBackgroundSyncRegisration (when it no longer needs to call unregister).

5) Added a few tests, and removed redundant sync browser tests from the service_workerbrowser_tests that are now covered in background_sync_browsertests.cc

Patch 1 [Blink]: https://codereview.chromium.org/1279323002/
Patch 2 [Browser]: this
Patch 3 [Blink cleanup]: https://codereview.chromium.org/1285653002/

BUG=502214
Committed: https://crrev.com/14977394017e56a86c5516e3c4b3f8988a2031f5
Cr-Commit-Position: refs/heads/master@{#348303}

[jkarlin, 2015-08-13 12:01:00]

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

[jkarlin, 2015-08-13 12:01:01]

iclelland: PTAL, sorry about the large CL!

[jkarlin, 2015-08-14 11:47:19]

PTAL, many thanks!

[iclelland, 2015-08-14 14:44:19]

That was a big one ;)

My only real concern is the tight coordination required between the browser and
the renderer to track the outstanding registrations, and the combination of
enforcing it with DCHECKs in some places, and just Log-and-ignoring in others.

I feel like there might be situations where the renderer crashes and doesn't
release its registration references, and the browser isn't able to recover from
that. I'll give it another look over to see if that really is an issue, though.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (left):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:102: void
BackgroundSyncManager::Unregister(
This method was made private in the header file; does it need to be moved
elsewhere in the .cc file to match?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:59:
DCHECK(client_registration_ids_.IsEmpty());
What happens if a renderer crashed (for unrelated reasons), while still holding
on to a reference to a ClientBackgroundSyncRegistration, at some time before the
manager is deleted? Will this code escalate that to a full browser crash?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:102:
scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
This change is repeated so many times in this file -- would it makes sense to
have a function to encapsulate the entire PostTask call for storage errors?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:184: //
TODO(jkarlin): Abort the render process.
What happens right now (by not aborting?) Is anything left in an inconsistent
state, or will the renderer clean itself up once it believes that the
registration has been released?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:658: // is called,
the manager might will hold the reference until it is destroyed.
"might will"

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:701:
RefCountedRegistration* existing_registration =
const removed?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:767:
RefCountedRegistration* registration =
const removed?

Actually there are a lot of these here -- is there an issue with RefCounted
objects not being const'able? You've defined an explicit const value() method,
so I didn't think so, but it's been pretty consistently removed in this file.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1138: void
BackgroundSyncManager::CompleteStatusAndRegistrationCallback(
Why are these two functions specified? I don't see why they're not just
instantiations of the CompleteOperationCallback anymore

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:168: // Removes the
background sync with tag |sync_registration_tag|, periodicity
|sync_registration_tag| isn't a parameter here anymore.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc (left):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:547:
TEST_F(BackgroundSyncManagerTest, RegisterExistingKeepsId) {
Is this test no longer valid? Do we not care that the registration doesn't
change, or does that not happen anymore?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:685:
TEST_F(BackgroundSyncManagerTest, UnregisterNonExisting) {
There's still code in UnregisterImpl that does this -- why is the test gone?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:21: static const
RegistrationId kInvalidRegistrationId;
Is this just for consistency? It doesn't look like you've changed where or how
it is accessed at all

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:44:
DISALLOW_COPY_AND_ASSIGN(BackgroundSyncRegistration);
Should there be an include for this, or is it enough that it's included
somewhere else transitively?
(Obviously, this is sufficient for the compiler right now)

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:94:
DCHECK(background_sync_manager);
What's the purpose of this check on destruction?

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:225: //
TODO(jkarlin): Abort client.
As in BackgroundSyncManager -- what happens if we don't abort? Is anything
inconsistent?

[jkarlin, 2015-08-17 17:05:15]

Patchset #12 (id:210001) has been deleted

[jkarlin, 2015-08-17 17:11:06]

Patchset #12 (id:230001) has been deleted

[jkarlin, 2015-08-17 17:14:50]

Thanks Ian, PTAL.

-- snip --
My only real concern is the tight coordination required between the browser and
the renderer to track the outstanding registrations, and the combination of
enforcing it with DCHECKs in some places, and just Log-and-ignoring in others.
-- snip --

I've made this saner. The BackgroundSyncManager now DCHECKS everywhere, as it
expects its clients to do the right thing. The BackgroundSyncServiceImpl checks
renderer values (which are untrusted) and kills when the renderer is misbehaving
(although the actual kill is a todo).

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:59:
DCHECK(client_registration_ids_.IsEmpty());
On 2015/08/14 14:44:18, iclelland wrote:
> What happens if a renderer crashed (for unrelated reasons), while still
holding
> on to a reference to a ClientBackgroundSyncRegistration, at some time before
the
> manager is deleted? Will this code escalate that to a full browser crash?

The BackgroundSyncServiceImpl tracks all of the references that the renderer
has. If the renderer crashes its mojo pipe will die and the
BackgroundSyncServiceImpl will die, releasing the references.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:102:
scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
On 2015/08/14 14:44:18, iclelland wrote:
> This change is repeated so many times in this file -- would it makes sense to
> have a function to encapsulate the entire PostTask call for storage errors?

Good idea. Done.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:184: //
TODO(jkarlin): Abort the render process.
On 2015/08/14 14:44:18, iclelland wrote:
> What happens right now (by not aborting?) Is anything left in an inconsistent
> state, or will the renderer clean itself up once it believes that the
> registration has been released?

I made some changes to BackgroundSyncServiceImpl::TrackRegistration so that this
can be a DCHECK instead. Done.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:658: // is called,
the manager might will hold the reference until it is destroyed.
On 2015/08/14 14:44:18, iclelland wrote:
> "might will"

Done.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:701:
RefCountedRegistration* existing_registration =
On 2015/08/14 14:44:18, iclelland wrote:
> const removed?

Done.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:767:
RefCountedRegistration* registration =
On 2015/08/14 14:44:18, iclelland wrote:
> const removed?
> 
> Actually there are a lot of these here -- is there an issue with RefCounted
> objects not being const'able? You've defined an explicit const value() method,
> so I didn't think so, but it's been pretty consistently removed in this file.

Thanks, I've added const where possible.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1138: void
BackgroundSyncManager::CompleteStatusAndRegistrationCallback(
On 2015/08/14 14:44:18, iclelland wrote:
> Why are these two functions specified? I don't see why they're not just
> instantiations of the CompleteOperationCallback anymore

Because they're passing scoped_ptrs that need to be passed. There is some magic
template way to do it (that's how bind works with base::Passed etc.) but I don't
know what it is. Added TODO though.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:168: // Removes the
background sync with tag |sync_registration_tag|, periodicity
On 2015/08/14 14:44:18, iclelland wrote:
> |sync_registration_tag| isn't a parameter here anymore.

Done.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc (left):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:547:
TEST_F(BackgroundSyncManagerTest, RegisterExistingKeepsId) {
On 2015/08/14 14:44:18, iclelland wrote:
> Is this test no longer valid? Do we not care that the registration doesn't
> change, or does that not happen anymore?

No longer valid. It used to be the same regitration returned the same id, but
with this change each ClientBackgroundSyncRegistration has as unique id.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:685:
TEST_F(BackgroundSyncManagerTest, UnregisterNonExisting) {
On 2015/08/14 14:44:18, iclelland wrote:
> There's still code in UnregisterImpl that does this -- why is the test gone?

Thanks for pointing this out. The BackgroundSyncManager now (as of this latest
patch) DCHECKS for the registration so the test needs to go.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:21: static const
RegistrationId kInvalidRegistrationId;
On 2015/08/14 14:44:18, iclelland wrote:
> Is this just for consistency? It doesn't look like you've changed where or how
> it is accessed at all

Needed it but then didn't, forgot to clean up. Thanks. Done.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:44:
DISALLOW_COPY_AND_ASSIGN(BackgroundSyncRegistration);
On 2015/08/14 14:44:18, iclelland wrote:
> Should there be an include for this, or is it enough that it's included
> somewhere else transitively?
> (Obviously, this is sufficient for the compiler right now)

Done.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:94:
DCHECK(background_sync_manager);
On 2015/08/14 14:44:19, iclelland wrote:
> What's the purpose of this check on destruction?

Just to verify that services delete before the manager.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:225: //
TODO(jkarlin): Abort client.
On 2015/08/14 14:44:18, iclelland wrote:
> As in BackgroundSyncManager -- what happens if we don't abort? Is anything
> inconsistent?

We should abort long term :) In the meanwhile, a client has a registration that
can't be used.

[jkarlin, 2015-08-17 17:18:15]

Forgot to check this one off.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (left):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:102: void
BackgroundSyncManager::Unregister(
On 2015/08/14 14:44:18, iclelland wrote:
> This method was made private in the header file; does it need to be moved
> elsewhere in the .cc file to match?

Done.

[jkarlin, 2015-08-17 17:18:23]

Patchset #12 (id:250001) has been deleted

[jkarlin, 2015-08-17 17:19:06]

Patchset #12 (id:270001) has been deleted

[iclelland, 2015-08-17 19:22:02]

Awesome! LGTM, with just a couple of nits in comments.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:59:
DCHECK(client_registration_ids_.IsEmpty());
On 2015/08/17 17:14:49, jkarlin wrote:
> On 2015/08/14 14:44:18, iclelland wrote:
> > What happens if a renderer crashed (for unrelated reasons), while still
> holding
> > on to a reference to a ClientBackgroundSyncRegistration, at some time before
> the
> > manager is deleted? Will this code escalate that to a full browser crash?
> 
> The BackgroundSyncServiceImpl tracks all of the references that the renderer
> has. If the renderer crashes its mojo pipe will die and the
> BackgroundSyncServiceImpl will die, releasing the references.

That seems sound, thanks.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1138: void
BackgroundSyncManager::CompleteStatusAndRegistrationCallback(
On 2015/08/17 17:14:49, jkarlin wrote:
> On 2015/08/14 14:44:18, iclelland wrote:
> > Why are these two functions specified? I don't see why they're not just
> > instantiations of the CompleteOperationCallback anymore
> 
> Because they're passing scoped_ptrs that need to be passed. There is some
magic
> template way to do it (that's how bind works with base::Passed etc.) but I
don't
> know what it is. Added TODO though.

That makes sense -- thanks for the explanation. I'm sure you're right, that
there is some template magic that would fix it, and that it's probably fine to
not spend any time on it right now.

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/170001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:225: //
TODO(jkarlin): Abort client.
On 2015/08/17 17:14:49, jkarlin wrote:
> On 2015/08/14 14:44:18, iclelland wrote:
> > As in BackgroundSyncManager -- what happens if we don't abort? Is anything
> > inconsistent?
> 
> We should abort long term :) In the meanwhile, a client has a registration
that
> can't be used.

As long as it's not a browser-destablising situation, then I'm okay with it :)

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:170: // with id
|sync_id|.
Random line break

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
File content/browser/background_sync/client_background_sync_registration.h
(right):

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:31: //
with BACKGROUND_SYNC_STATUS_OK on success.
And never, ever fails ;)

(I don't know if we need to specify what happens on failure, but it feels wrong
that we're missing any mention of it)

[jkarlin, 2015-08-18 15:12:38]

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

[jkarlin, 2015-08-18 15:12:38]

michaeln: PTAL at everything for committer approval. Thank you!

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:170: // with id
|sync_id|.
On 2015/08/17 19:22:02, iclelland wrote:
> Random line break

Done.

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
File content/browser/background_sync/client_background_sync_registration.h
(right):

https://codereview.chromium.org/1282013004/diff/290001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:31: //
with BACKGROUND_SYNC_STATUS_OK on success.
On 2015/08/17 19:22:02, iclelland wrote:
> And never, ever fails ;)
> 
> (I don't know if we need to specify what happens on failure, but it feels
wrong
> that we're missing any mention of it)

:) Changed to "if it succeeds".

[michaeln, 2015-08-19 01:56:01]

I'm not sure the lifetime management stuff really works? I'm also not sure any
of it is needed, why do those PODs structs need to be kept in memory?

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:631:
sync_registration->id = client_registration_ids_.Add(ptr);
Does this leak (for the lifetime of the profile) if the logic inside blink
(running on the renderer's worker thread) doesn't call
WebSomething->trackRegistration?

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1085:
out_registration->set_sync_state(registration.value()->sync_state());
Why is it necessary to copy the values? The refcounting logic works to keep a
particular instance alive.

Or maybe the better question is, why is it important to keep that particular
refcounted instance alive? It looks like POD data structure that could be passed
by value everywhere.

I'm not following exactaly, looks like your making copies of the POD as needed
and doing refcounting bookeeping?

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1089:
out_registration->set_id(client_registration_ids_.Add(ptr));
This confused me quite a bit.

(I think) in some cases 'id' refers to a persistent identifer for a stored
registration, i see the value being pulled out of a protobug. And in others
cases 'id' is a very transient valued. Multiple callers can GetRegisteration and
be returned something that corresponds to that same persistent registration,
each with a different 'id'.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:107: virtual
~RefCountedRegistration() {}
no semantics around to deletion

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.cc:83:
BackgroundSyncRegistration::~BackgroundSyncRegistration() {}
deletion has no semantic meaning

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:273:
hosted_registrations_.AddWithID(registration, registration->id());
Does this leak (for the lifetime of the renderer process) if script logic
running on a renderer's worker thread does call
WebSomething->releaseRegistration?

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/client_background_sync_registration.h
(right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:22: :
public BackgroundSyncRegistration {
This derivation seems a little odd to me.

(I think) the 'client' class acts as a unique handle to an underlying
registration. There can be many handles to the same underlyaing registration.
Each handle has a unique 'id' which is not at all related to the 'id' of the
underlying registration. It's not a kind of registration, it's a layer of
indirection between a client and the registartion.

I asked this elsewhere too, but why does this class get a copy of the underlying
registrations values instead of referring to the RefCounted instance for the
values?

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:32: void
Unregister(int64 service_worker_id, const StatusCallback& callback);
ah, now i see there unregister has gone

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:38: // is
deleted.
It might be worth using weakptrs here since these objects are given to whomever
is calling the public GetRegistration() and to not assume all caller have
deleted them prior to ~BSM.

https://codereview.chromium.org/1282013004/diff/310001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:43:
blink::WebSyncProvider* webSyncProvider =
style nit: !camel_case

[jkarlin, 2015-08-19 12:47:06]

Thanks Michael, PTAL at my responses.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:631:
sync_registration->id = client_registration_ids_.Add(ptr);
On 2015/08/19 01:56:01, michaeln wrote:
> Does this leak (for the lifetime of the profile) if the logic inside blink
> (running on the renderer's worker thread) doesn't call
> WebSomething->trackRegistration?

Yes. But we'd at least catch that it's happening (at least in tests) with the
destructor's DCHECK.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1085:
out_registration->set_sync_state(registration.value()->sync_state());
On 2015/08/19 01:56:01, michaeln wrote:
> Why is it necessary to copy the values? The refcounting logic works to keep a
> particular instance alive.
> 
> Or maybe the better question is, why is it important to keep that particular
> refcounted instance alive? It looks like POD data structure that could be
passed
> by value everywhere.

Sorry, it's not obvious in this CL why all of this is happening. The description
describes it some. This particular refcounted instance needs to be kept alive
because in an upcoming CL clients will be able to ask about the status of
registrations that have completed and are no longer in the
sw_to_registrations_map_. The refcounted instance will have the final status.

> 
> I'm not following exactaly, looks like your making copies of the POD as needed
> and doing refcounting bookeeping?

The BackgroundSyncManager keeps a single refcounted instance for itself, but
gives the clients copies. Each copy has a unique id which maps to a
scoped_refptr to the BackgroundSyncManager's refcounted instance. The purpose of
this is to keep the refcounting within the BackgroundSyncManager. This way we
guarantee that when the BackgroundSyncManager is deleted all of its
registrations are deleted with it.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1089:
out_registration->set_id(client_registration_ids_.Add(ptr));
On 2015/08/19 01:56:01, michaeln wrote:
> This confused me quite a bit.
> 
> (I think) in some cases 'id' refers to a persistent identifer for a stored
> registration, i see the value being pulled out of a protobug. And in others
> cases 'id' is a very transient valued. Multiple callers can GetRegisteration
and
> be returned something that corresponds to that same persistent registration,
> each with a different 'id'.

Yes, each client registration has a unique id which the BackgroundSyncManager
maps to the real (protobuf) id. Added a comment.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:107: virtual
~RefCountedRegistration() {}
On 2015/08/19 01:56:01, michaeln wrote:
> no semantics around to deletion

Made default.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.cc:83:
BackgroundSyncRegistration::~BackgroundSyncRegistration() {}
On 2015/08/19 01:56:01, michaeln wrote:
> deletion has no semantic meaning

Made default.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:273:
hosted_registrations_.AddWithID(registration, registration->id());
On 2015/08/19 01:56:01, michaeln wrote:
> Does this leak (for the lifetime of the renderer process) if script logic
> running on a renderer's worker thread does call
> WebSomething->releaseRegistration?

Yes. See https://codereview.chromium.org/1299903002/ for how the render process
calls releaseRegistration when SyncRegistration and PeriodicSyncRegistration
objects are collected.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/client_background_sync_registration.h
(right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:22: :
public BackgroundSyncRegistration {
On 2015/08/19 01:56:01, michaeln wrote:
> This derivation seems a little odd to me.
> 
> (I think) the 'client' class acts as a unique handle to an underlying
> registration. There can be many handles to the same underlyaing registration.
> Each handle has a unique 'id' which is not at all related to the 'id' of the
> underlying registration. It's not a kind of registration, it's a layer of
> indirection between a client and the registartion.

That's fair. Perhaps it shouldn't be a sub-class of BackgroundSyncRegistration
and should wrap a BackgroundSyncRegistration instead. What do you think?

> 
> I asked this elsewhere too, but why does this class get a copy of the
underlying
> registrations values instead of referring to the RefCounted instance for the
> values?

To prevent ref-counted objects from escaping the BackgroundSyncManager. This way
the BSM can DCHECK at destruction to verify that all of the registrations are
destroyed.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:32: void
Unregister(int64 service_worker_id, const StatusCallback& callback);
On 2015/08/19 01:56:01, michaeln wrote:
> ah, now i see there unregister has gone

Acknowledged.

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:38: // is
deleted.
On 2015/08/19 01:56:01, michaeln wrote:
> It might be worth using weakptrs here since these objects are given to
whomever
> is calling the public GetRegistration() and to not assume all caller have
> deleted them prior to ~BSM.

There is a DCHECK to verify that clients call it prior to ~BSM. If they call it
later it's a bug.

https://codereview.chromium.org/1282013004/diff/310001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:43:
blink::WebSyncProvider* webSyncProvider =
On 2015/08/19 01:56:01, michaeln wrote:
> style nit: !camel_case

Done.

[jkarlin, 2015-08-19 17:24:22]

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/client_background_sync_registration.h
(right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/client_background_sync_registration.h:22: :
public BackgroundSyncRegistration {
On 2015/08/19 12:47:06, jkarlin wrote:
> On 2015/08/19 01:56:01, michaeln wrote:
> > This derivation seems a little odd to me.
> > 
> > (I think) the 'client' class acts as a unique handle to an underlying
> > registration. There can be many handles to the same underlyaing
registration.
> > Each handle has a unique 'id' which is not at all related to the 'id' of the
> > underlying registration. It's not a kind of registration, it's a layer of
> > indirection between a client and the registartion.
> 
> That's fair. Perhaps it shouldn't be a sub-class of BackgroundSyncRegistration
> and should wrap a BackgroundSyncRegistration instead. What do you think?
> 
> > 
> > I asked this elsewhere too, but why does this class get a copy of the
> underlying
> > registrations values instead of referring to the RefCounted instance for the
> > values?
> 
> To prevent ref-counted objects from escaping the BackgroundSyncManager. This
way
> the BSM can DCHECK at destruction to verify that all of the registrations are
> destroyed.
> 

Ah, I think perhaps you're getting at something like this instead? This does
seem better:

class ClientBackgroundSyncHandle {
  BackgroundSyncRegistrationOptions* options() { return
background_sync_manager_->GetRegistrationOptionsById(reference_id_); }

  ~ClientBackgroundSyncHandle() {
background_sync_manager_->ReleaseRegistration(reference_id_); }

  Unregister() { background_sync_manager_->Unregister(...); }

private:
  int64 reference_id_;
  BackgroundSyncManager* background_sync_manager_;
};

[michaeln, 2015-08-19 18:59:35]

On 2015/08/19 17:24:22, jkarlin wrote:
>
https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
> File content/browser/background_sync/client_background_sync_registration.h
> (right):
> 
>
https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
> content/browser/background_sync/client_background_sync_registration.h:22: :
> public BackgroundSyncRegistration {
> On 2015/08/19 12:47:06, jkarlin wrote:
> > On 2015/08/19 01:56:01, michaeln wrote:
> > > This derivation seems a little odd to me.
> > > 
> > > (I think) the 'client' class acts as a unique handle to an underlying
> > > registration. There can be many handles to the same underlyaing
> registration.
> > > Each handle has a unique 'id' which is not at all related to the 'id' of
the
> > > underlying registration. It's not a kind of registration, it's a layer of
> > > indirection between a client and the registartion.
> > 
> > That's fair. Perhaps it shouldn't be a sub-class of
BackgroundSyncRegistration
> > and should wrap a BackgroundSyncRegistration instead. What do you think?

sgtm!

> > > 
> > > I asked this elsewhere too, but why does this class get a copy of the
> > underlying
> > > registrations values instead of referring to the RefCounted instance for
the
> > > values?
> > 
> > To prevent ref-counted objects from escaping the BackgroundSyncManager. This
> way
> > the BSM can DCHECK at destruction to verify that all of the registrations
are
> > destroyed.
> > 
> 
> Ah, I think perhaps you're getting at something like this instead? This does
> seem better:
> 
> class ClientBackgroundSyncHandle {
>   BackgroundSyncRegistrationOptions* options() { return
> background_sync_manager_->GetRegistrationOptionsById(reference_id_); }
> 
>   ~ClientBackgroundSyncHandle() {
> background_sync_manager_->ReleaseRegistration(reference_id_); }
> 
>   Unregister() { background_sync_manager_->Unregister(...); }
> 
> private:
>   int64 reference_id_;
>   BackgroundSyncManager* background_sync_manager_;
> };

Yup, "handle" is a commonly used term for this kind of indirection and
delegating to the real thing to access the underlying data makes it more clear.

[jkarlin, 2015-08-20 17:08:18]

On 2015/08/19 18:59:35, michaeln wrote:
> On 2015/08/19 17:24:22, jkarlin wrote:
> >
>
https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
> > File content/browser/background_sync/client_background_sync_registration.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
> > content/browser/background_sync/client_background_sync_registration.h:22: :
> > public BackgroundSyncRegistration {
> > On 2015/08/19 12:47:06, jkarlin wrote:
> > > On 2015/08/19 01:56:01, michaeln wrote:
> > > > This derivation seems a little odd to me.
> > > > 
> > > > (I think) the 'client' class acts as a unique handle to an underlying
> > > > registration. There can be many handles to the same underlyaing
> > registration.
> > > > Each handle has a unique 'id' which is not at all related to the 'id' of
> the
> > > > underlying registration. It's not a kind of registration, it's a layer
of
> > > > indirection between a client and the registartion.
> > > 
> > > That's fair. Perhaps it shouldn't be a sub-class of
> BackgroundSyncRegistration
> > > and should wrap a BackgroundSyncRegistration instead. What do you think?
> 
> sgtm!
> 
> > > > 
> > > > I asked this elsewhere too, but why does this class get a copy of the
> > > underlying
> > > > registrations values instead of referring to the RefCounted instance for
> the
> > > > values?
> > > 
> > > To prevent ref-counted objects from escaping the BackgroundSyncManager.
This
> > way
> > > the BSM can DCHECK at destruction to verify that all of the registrations
> are
> > > destroyed.
> > > 
> > 
> > Ah, I think perhaps you're getting at something like this instead? This does
> > seem better:
> > 
> > class ClientBackgroundSyncHandle {
> >   BackgroundSyncRegistrationOptions* options() { return
> > background_sync_manager_->GetRegistrationOptionsById(reference_id_); }
> > 
> >   ~ClientBackgroundSyncHandle() {
> > background_sync_manager_->ReleaseRegistration(reference_id_); }
> > 
> >   Unregister() { background_sync_manager_->Unregister(...); }
> > 
> > private:
> >   int64 reference_id_;
> >   BackgroundSyncManager* background_sync_manager_;
> > };
> 
> Yup, "handle" is a commonly used term for this kind of indirection and
> delegating to the real thing to access the underlying data makes it more
clear.

Done, great suggestion. PTAL

[michaeln, 2015-08-21 02:39:25]

cc'ing ken for thoughts about relative msg ordering (see the "Bah... also, I'm
noticing" comment)

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/310001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:631:
sync_registration->id = client_registration_ids_.Add(ptr);
On 2015/08/19 12:47:05, jkarlin wrote:
> On 2015/08/19 01:56:01, michaeln wrote:
> > Does this leak (for the lifetime of the profile) if the logic inside blink
> > (running on the renderer's worker thread) doesn't call
> > WebSomething->trackRegistration?
> 
> Yes. But we'd at least catch that it's happening (at least in tests) with the
> destructor's DCHECK.

I'd vote to improve upon this. There are a many things that can prevent the
logic on the worker thread from ever getting a chance to call the balancing
'track' method. See comments inline for some ideas on that.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:383:
scoped_refptr<RefCountedRegistration> new_ref_registration(
Is it possible for there to be handle to a registration matching these criteria
in the  registration_handle_ids_ map? And if so should its underlying
BackgroundSyncRegistration be updated and reinserted with AddRegistrationToMap?

Something that might help is to have an authoritative collection of 'live'
RefCountedRegistration instances, keyed by persistent id i guess, rawptrs to not
influence lifetimes? Their may be refs to these objects from the
registration_handle_ids_ and the sw_to_registrations_map_, and those refs
determine the object lifetimes.

You can see something like that at work in the sw code base. The
constructors/destructors add/rmv ptrs from the map. See AddLiveVersion() and
RemoveLiveVersion() for example.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:671:
base::Bind(&BackgroundSyncManager::UnregisterImpl,
What prevents ReleaseHandleId(xxx) from happening prior to the scheduled op
running? You could create a new handle and bind that instance into this closure,
Pass() it as a param in place of 'handle_id'. Passing ids is good for crossing
process boundaries, but once your inside a process, use the smart scopers to
manage lifetimes.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:966:
base::Bind(&BackgroundSyncManager::EventComplete,
I think we might be able to improve  upon the lifetime management. As coded,
this introduces a leak that isn't all that unlikely (if the worker thread
doesn't actually process the event... leak).

What if...

This method creates a handle and binds it to the EventComplete callback so the
handle will be valid during event dispatch to the renderer. During event
dispatch the renderer is expected to invoke the
track|adpot|acquire|copy|duplicate|take (or whatever name is settled upon)
method.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:166: using
SWIdToRegistrationsMap = std::map<int64, BackgroundSyncRegistrations>;
hmmm... what if...


// Used to dispatch the sync event, bound to the event completion callback.
// At dtor time, will release the underlying reg if it hasn't been adpoted.
// Or if it's been adopted, dtor does nothing.
scoped_ptr<AdoptableBgSyncRegHandle> CreateAdoptableRegistrationHandle(reg);

// Called by the client-side when processing the event. Returns a normal
// handle having the same id, or nullptr is there is no adpotable handle
// with that id.
scoped_ptr<BgSyncRegHandle> AdoptRegistrationHandle(handle_id);

// Just like you have now, used for responding to mojo intf method calls.
scoped_ptr<BgSyncRegHandle> CreatedRegistrationHandle(reg);

// Just like you have now, only mucks with the nonadoptable handles.
// Called by ~BgSyncRegHandle
void ReleaesRegistrationHandle(handle_id)

// Called by ~AdoptableBgSyncRegHandle
void ReleaesAdoptableRegistrationHandle(handle_id)


IDMap<scoped_refptr<const RefCountedRegistration>, IDMapOwnPointer>
    registration_handle_ids_;

IDMap<scoped_refptr<const RefCountedRegistration>, IDMapOwnPointer>
    adpotable_registration_handle_ids_;

SWIdToRegistrationsMap sw_to_registrations_map_;


or something like that?

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:24: virtual
~BackgroundSyncRegistration();
This class doesn't have any derivatives anymore, so nix the virtual dtor?

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:24: using
HandleId = int32_t;
is 'int' good enough for this

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:30:
virtual ~BackgroundSyncRegistrationHandle();
does this need to be virtual?

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:36: const
BackgroundSyncRegistration* RegistrationForTests() const {
maybe put this at the end of the public section inside of an #if
defined(UNIT_TEST)

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:42: void
Unregister(int64 service_worker_id, const StatusCallback& callback);
int64_t?

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:95:
DCHECK(background_sync_manager);
You could put this larger expression inside the dcheck so the dtor is obviously
and completely empty in release builds.

DCHECK(background_sync_context_->background_sync_manager());

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:209: if
(hosted_registrations_.Lookup(sync_registration->id)) {
This is actually using a 'handle_id' to lookup a 'handle' but its hard to tell
that just by reading this one line of code.

if (handles_in_use_.Lookup(xxx->handle_id))
  // ohh nooo mr bill, trying to adopt a handle that's already in use!!

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:215:
scoped_ptr<BackgroundSyncRegistrationHandle> registration(
nit: put 'handle' somewhere in the local variable name, maybe just call it
'handle'?

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:64: void
OnConnectionError();
I had a quick chat with rockot about how to manage handle ownership such that
they're never leaked and are reliably present when they should be. 
OnConnectionError() really helps. If a message fails to be delivered to the
remote-side, OnConnectionError() will fire. For example, when a worker thread
terminates and closes its side of the intf. I think that makes it easy to manage
the lifetime of handles attributed to this client-side of this intf ptr
(provided the handles aren't supposed to outlive the intfptr?)

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:73:
hosted_registrations_;
The terms 'host' and 'registration' are both somewhat overloaded in our code
base. Now that you're using the 'handle' term, you can proably rename some
methods and members to better distinguish between where a handle is being used
vs an actual registration.

// Collection of handles owned by this instance. For each entry in the map,
// the handle id is either in transit destined for the client-side, or held in
// an object on the client-side. When this instance dies, these handles are
// freed so we don't leak references to sync registrations.
Map handles_in_use_;

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:144:
scoped_ptr<blink::WebSyncRegistration> scoped_registration(registration);
is WebSyncRegistration thread safe?

no, it has a WebString data member :(

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider.h:55: void
releaseRegistration(int64_t sync_id) override;
maybe use handle_id as the param name for clarity

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:179: void
BackgroundSyncProviderThreadProxy::trackRegistration(
adoptRegistrationHandle(handle_id)

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:183:
base::Unretained(sync_provider_), registration));
it'd be good to indicate xfer of ownership of |registration| with a
make_scoped_ptr here... but WebSyncRegistration aren't threadsafe so you'll have
to do something else here.


Bah... also, I'm noticing that the messaging is all being routed to thru the
main thread. That will wreak havok on the lifetime management scheme i described
because that scheme depends on the relative ordering of trackRegistrationMojoMsg
and didHandleSyncEventMojoMsg. If they come out of order it doesn't work and
routing the the track traffic thru a mojo intf on main thread vs the
handledEvent traffic thru an intf directly on the worker thread pretty much
guarantees they'll show up in the wrong order :(

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:186: void
BackgroundSyncProviderThreadProxy::releaseRegistration(int64_t sync_id) {
releaseRegistrationHandle(handle_id)

https://codereview.chromium.org/1282013004/diff/390001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:48:
web_sync_provider->trackRegistration(web_registration.release());
track is an odd verb? take adopt acquire grab?

[jkarlin, 2015-08-25 17:32:27]

Patchset #18 (id:410001) has been deleted

[jkarlin, 2015-08-25 17:32:59]

*phew*. PTAL Michael. Thanks.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:383:
scoped_refptr<RefCountedRegistration> new_ref_registration(
On 2015/08/21 02:39:24, michaeln wrote:
> Is it possible for there to be handle to a registration matching these
criteria
> in the  registration_handle_ids_ map? And if so should its underlying
> BackgroundSyncRegistration be updated and reinserted with
AddRegistrationToMap?

Yes, but it will have a different registration id. No, the handle's registration
shouldn't be updated, it should keep pointing at the out-of-date registration.

> 
> Something that might help is to have an authoritative collection of 'live'
> RefCountedRegistration instances, keyed by persistent id i guess, rawptrs to
not
> influence lifetimes? Their may be refs to these objects from the
> registration_handle_ids_ and the sw_to_registrations_map_, and those refs
> determine the object lifetimes.

sw_to_registrations_map_ is the collection of live registrations. Once it leaves
that map it may still be referenced by clients but it's no longer live (won't be
run).


> 
> You can see something like that at work in the sw code base. The
> constructors/destructors add/rmv ptrs from the map. See AddLiveVersion() and
> RemoveLiveVersion() for example.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:671:
base::Bind(&BackgroundSyncManager::UnregisterImpl,
On 2015/08/21 02:39:24, michaeln wrote:
> What prevents ReleaseHandleId(xxx) from happening prior to the scheduled op
> running? You could create a new handle and bind that instance into this
closure,
> Pass() it as a param in place of 'handle_id'. Passing ids is good for crossing
> process boundaries, but once your inside a process, use the smart scopers to
> manage lifetimes.

Thanks for that catch. Resolved by reverting UnregisterImpl.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:966:
base::Bind(&BackgroundSyncManager::EventComplete,
On 2015/08/21 02:39:24, michaeln wrote:
> I think we might be able to improve  upon the lifetime management. As coded,
> this introduces a leak that isn't all that unlikely (if the worker thread
> doesn't actually process the event... leak).
> 
> What if...
> 
> This method creates a handle and binds it to the EventComplete callback so the
> handle will be valid during event dispatch to the renderer. During event
> dispatch the renderer is expected to invoke the
> track|adpot|acquire|copy|duplicate|take (or whatever name is settled upon)
> method.

This is a good idea. In my implementation the renderer creates a duplicate
handle (with a unique handle id) and doesn't fire the event until the handle is
made. There shouldn't be any race issues.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:166: using
SWIdToRegistrationsMap = std::map<int64, BackgroundSyncRegistrations>;
On 2015/08/21 02:39:24, michaeln wrote:
> hmmm... what if...
> 
> 
> // Used to dispatch the sync event, bound to the event completion callback.
> // At dtor time, will release the underlying reg if it hasn't been adpoted.
> // Or if it's been adopted, dtor does nothing.
> scoped_ptr<AdoptableBgSyncRegHandle> CreateAdoptableRegistrationHandle(reg);
> 
> // Called by the client-side when processing the event. Returns a normal
> // handle having the same id, or nullptr is there is no adpotable handle
> // with that id.
> scoped_ptr<BgSyncRegHandle> AdoptRegistrationHandle(handle_id);
> 
> // Just like you have now, used for responding to mojo intf method calls.
> scoped_ptr<BgSyncRegHandle> CreatedRegistrationHandle(reg);
> 
> // Just like you have now, only mucks with the nonadoptable handles.
> // Called by ~BgSyncRegHandle
> void ReleaesRegistrationHandle(handle_id)
> 
> // Called by ~AdoptableBgSyncRegHandle
> void ReleaesAdoptableRegistrationHandle(handle_id)
> 
> 
> IDMap<scoped_refptr<const RefCountedRegistration>, IDMapOwnPointer>
>     registration_handle_ids_;
> 
> IDMap<scoped_refptr<const RefCountedRegistration>, IDMapOwnPointer>
>     adpotable_registration_handle_ids_;
> 
> SWIdToRegistrationsMap sw_to_registrations_map_;
> 
> 
> or something like that?

I solved the problem without the need for adoptable handles. I used two handles
and a duplicate method.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:24: virtual
~BackgroundSyncRegistration();
On 2015/08/21 02:39:24, michaeln wrote:
> This class doesn't have any derivatives anymore, so nix the virtual dtor?

Done.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:24: using
HandleId = int32_t;
On 2015/08/21 02:39:24, michaeln wrote:
> is 'int' good enough for this

Done.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:30:
virtual ~BackgroundSyncRegistrationHandle();
On 2015/08/21 02:39:24, michaeln wrote:
> does this need to be virtual?

Done.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:36: const
BackgroundSyncRegistration* RegistrationForTests() const {
On 2015/08/21 02:39:24, michaeln wrote:
> maybe put this at the end of the public section inside of an #if
> defined(UNIT_TEST) 

Removed.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:42: void
Unregister(int64 service_worker_id, const StatusCallback& callback);
On 2015/08/21 02:39:24, michaeln wrote:
> int64_t?

Done.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:95:
DCHECK(background_sync_manager);
On 2015/08/21 02:39:24, michaeln wrote:
> You could put this larger expression inside the dcheck so the dtor is
obviously
> and completely empty in release builds.
> 
> DCHECK(background_sync_context_->background_sync_manager());

Done.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:209: if
(hosted_registrations_.Lookup(sync_registration->id)) {
On 2015/08/21 02:39:24, michaeln wrote:
> This is actually using a 'handle_id' to lookup a 'handle' but its hard to tell
> that just by reading this one line of code.
> 
> if (handles_in_use_.Lookup(xxx->handle_id))
>   // ohh nooo mr bill, trying to adopt a handle that's already in use!!

This method is gone now.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:215:
scoped_ptr<BackgroundSyncRegistrationHandle> registration(
On 2015/08/21 02:39:24, michaeln wrote:
> nit: put 'handle' somewhere in the local variable name, maybe just call it
> 'handle'?

This method is gone now.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:64: void
OnConnectionError();
On 2015/08/21 02:39:24, michaeln wrote:
> I had a quick chat with rockot about how to manage handle ownership such that
> they're never leaked and are reliably present when they should be. 
> OnConnectionError() really helps. If a message fails to be delivered to the
> remote-side, OnConnectionError() will fire. For example, when a worker thread
> terminates and closes its side of the intf. I think that makes it easy to
manage
> the lifetime of handles attributed to this client-side of this intf ptr
> (provided the handles aren't supposed to outlive the intfptr?)

Right. If the renderer dies this gets called (which leads to this object's
deletion) and all of the renderer's handles are released.

https://codereview.chromium.org/1282013004/diff/390001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:73:
hosted_registrations_;
On 2015/08/21 02:39:24, michaeln wrote:
> The terms 'host' and 'registration' are both somewhat overloaded in our code
> base. Now that you're using the 'handle' term, you can proably rename some
> methods and members to better distinguish between where a handle is being used
> vs an actual registration.
> 
> // Collection of handles owned by this instance. For each entry in the map,
> // the handle id is either in transit destined for the client-side, or held in
> // an object on the client-side. When this instance dies, these handles are
> // freed so we don't leak references to sync registrations.
> Map handles_in_use_;

Refactored to active_handles_.

Not sure I want to change the mojo function names from registration to handle
however, as the type that's returned is a mojo registration.

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:144:
scoped_ptr<blink::WebSyncRegistration> scoped_registration(registration);
On 2015/08/21 02:39:24, michaeln wrote:
> is WebSyncRegistration thread safe?
> 
> no, it has a WebString data member :(

Good catch. This function is gone now but this is an existing problem with the
file, tracking in https://crbug.com/524017

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider.h:55: void
releaseRegistration(int64_t sync_id) override;
On 2015/08/21 02:39:24, michaeln wrote:
> maybe use handle_id as the param name for clarity

Done.

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:179: void
BackgroundSyncProviderThreadProxy::trackRegistration(
On 2015/08/21 02:39:25, michaeln wrote:
> adoptRegistrationHandle(handle_id)

Changed to "GetRegistrationForHandleId" as it now asks the service to create a
new handle and return the corresponding mojo registration.

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:183:
base::Unretained(sync_provider_), registration));
On 2015/08/21 02:39:25, michaeln wrote:
> it'd be good to indicate xfer of ownership of |registration| with a
> make_scoped_ptr here... but WebSyncRegistration aren't threadsafe so you'll
have
> to do something else here.

OBE. The new method takes a HandleId.

> 
> 
> Bah... also, I'm noticing that the messaging is all being routed to thru the
> main thread. That will wreak havok on the lifetime management scheme i
described
> because that scheme depends on the relative ordering of
trackRegistrationMojoMsg
> and didHandleSyncEventMojoMsg. If they come out of order it doesn't work and
> routing the the track traffic thru a mojo intf on main thread vs the
> handledEvent traffic thru an intf directly on the worker thread pretty much
> guarantees they'll show up in the wrong order :(

With my current implementation the manager has handle that is kept until the
event finishes. While the event is firing the client requests a duplicate handle
for the same id. didHandleSyncEventMojoMsg won't come before the request for the
duplicate handle as the event won't be completed until the duplicate handle has
been returned.

https://codereview.chromium.org/1282013004/diff/390001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:186: void
BackgroundSyncProviderThreadProxy::releaseRegistration(int64_t sync_id) {
On 2015/08/21 02:39:25, michaeln wrote:
> releaseRegistrationHandle(handle_id)

Changed sync_id to handle_id. Added a TODO to update the function names later
(it requires a 3-legged patch due to a change to WebSyncProvider.h)

https://codereview.chromium.org/1282013004/diff/390001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/390001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:48:
web_sync_provider->trackRegistration(web_registration.release());
On 2015/08/21 02:39:25, michaeln wrote:
> track is an odd verb? take adopt acquire grab?

went with DuplicateRegistrationHandle

[jkarlin, 2015-08-25 21:31:46]

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:980:
registration_handle->handle_id(),
registration_handle is passed later in this call, need to use a raw pointer or
get the id earlier.

[jkarlin, 2015-08-26 14:22:51]

PTAL michaeln, sorry that it has become so monstrous :\

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:980:
registration_handle->handle_id(),
On 2015/08/25 21:31:46, jkarlin wrote:
> registration_handle is passed later in this call, need to use a raw pointer or
> get the id earlier.

Done.

[jkarlin, 2015-08-27 17:39:06]

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

[jkarlin, 2015-08-27 17:39:07]

tsepez@chromium.org: Please review changes in the mojom file. Thanks!

[michaeln, 2015-08-28 02:53:12]

Sorry for being slow to reply. I'm still looking for how to best utilize mojo so
we can establish pattern worth replicating.

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:173: int64
sw_registration_id,
|sw_registration_id| is not used, did you mean to use it for something or can it
be removed?

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.cc
(right):

https://codereview.chromium.org/1282013004/diff/430001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.cc:41:
return registration_ != nullptr;
maybe test background_sync_mgr_ && registration_

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:35: virtual
~RefCountedRegistration() = default;
does this need to be virtual? if not, nix the vtable

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:173: int64
sw_registration_id,
|sw_registration_id| is not used, nix it?

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:640:
RefCountedRegistration* registration) {
i think const scoped_refptr<T>& registration for this parameter instead of the
raw ptr is our current convention for passing around refcounted objs

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:672: int64
sw_registration_id,
I was wondering if sw_registration_id is needed? I see it being passed in
parallel with the handle_id often and now i see why. The
BackgroundSyncRegistration struct doesn't have a data member for the sw_reg_id
from whence it came. The value actually is needed here to remove it form the
active collection and to update storage properly. It might be nice to add that
value to the struct so |handle_id| is all you need to get to everything there is
to know about a particular registration.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:204: void
AddActiveRegistration(
thnx for making the subtle name changes

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:28:
BackgroundSyncRegistrationHandle(
can this constructor be put in the private section? does anything other than the
mgr (a friend) have any reason to construct one of these?

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:34:
DCHECK(background_sync_manager_);
#include "base/logging.h" for dcheck

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:197: int64_t
service_worker_registration_id,
if service_worker_registration_id needed? if not, nix it?

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.h (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:36: int64_t id,
int handle_id?

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:37: int64_t
sw_registration_id,
is sw_registration_id needed, is handle_id not sufficient?

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:51: int64_t
handle_id,
int to match BackgroundSyncRegistrationHandle::HandleId

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:52: int64_t
service_worker_registration_id,
is service_worker_registration_id needed?

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:64: int64_t id,
this is an int handle_id? is that right?

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:142: void
BackgroundSyncProvider::releaseRegistration(int64_t handle_id) {
int?

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:147: int64_t
handle_id,
int?

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:28: class
BackgroundSyncProviderThreadProxy : public blink::WebSyncProvider,
I don't think we're taking full advantage of mojo with the current design
pattern.

Mojo ipc gives us lightweight interface ptrs which can be bound to any thread so
we can avoid doing heavyweight manual thread hopping. The lifetimes of the
objects on either side can be scoped to one another which can be help with
resource reclamation when things unwind.

What if instead of a 'proxy' each thread had its own BackgroundSyncProvider
instance with its own BackgroundSyncServicePtr interface ptr?

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:42:
int64_t id,
is this an int handle_id and are the other inputs needed?

Might be out of scope but would it make sense to pass a WebSyncRegistration*
here instead of passing an id value. If i understand the api properly, the
caller receives that data type in response to the regiseter and get methods. I
think it contains the handle_id value of interest.
(similar to how WebServiceWorkerRegistration is not referred to by id in the
WebKit api)

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:45:
provider->DuplicateRegistrationHandle(
Eeek, the thread and msg round trips are kinda ugly. This is akin to putting a
sync ipc in the loop since msg queues have to flush in both directions prior to
proceeding with the event handleing.

I think can leak bgsync handle references for the lifetime of the renderer
process. Suppose...

* DuplicateRegistrationHandle is called.
* Browser-side does its work stashes the handleptr in
BackgroundSyncServiceImpl.active_handles_ and puts the goods in transit.
* Worker thread terminates.
* Main thread receives the goods and calls
DuplicateRegistrationHandleCallbackOnMainThread
* But... DuplicateRegistrationHandleCallbackOnWorkerThread is never called
* And... releaseRegistration(handle_id) is never called
* The BackgroundSyncServiceImpl eventually gets deleted when the renderer exits
and that does clean up the handle.

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:358:
provider_context_->GetRegistrationInfoAndVersionAttributes(&registration_info,
there's an early return in GetRegistrationInfoAndVersionAttributes where it can
return false but for a worker context it never should go down that route, can
you add a...

DCHECK_NE(registration_info.registration_id,
kInvalidServiceWorkerRegistrationId)

[iclelland, 2015-08-28 12:55:22]

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:28: class
BackgroundSyncProviderThreadProxy : public blink::WebSyncProvider,
On 2015/08/28 02:53:11, michaeln wrote:
> I don't think we're taking full advantage of mojo with the current design
> pattern.
> 
> Mojo ipc gives us lightweight interface ptrs which can be bound to any thread
so
> we can avoid doing heavyweight manual thread hopping. The lifetimes of the
> objects on either side can be scoped to one another which can be help with
> resource reclamation when things unwind.
> 
> What if instead of a 'proxy' each thread had its own BackgroundSyncProvider
> instance with its own BackgroundSyncServicePtr interface ptr?

That's actually a good idea -- I wrote the original thread proxy, with the
intention of having just one mojo channel from each renderer to the browser.
Since the mojo interfaces are so light, though, and since mojo is going to do a
thread hop to its own UI thread regardless of where we call it from (as I
understand it,) then it makes sense to try to avoid doing another one here, at
the expense of a mojo connection per thread.

Can that refactoring be in a separate CL, or would you want to see it in this
one?

[Tom Sepez, 2015-08-31 18:30:56]

mojom LGTM

[Tom Sepez, 2015-08-31 18:30:56]

mojom LGTM

[jkarlin, 2015-09-02 23:51:13]

Patchset #23 (id:530001) has been deleted

[jkarlin, 2015-09-02 23:51:41]

Back from vacation. Thanks for your comments Michael. PTAL!

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:35: virtual
~RefCountedRegistration() = default;
On 2015/08/28 02:53:11, michaeln wrote:
> does this need to be virtual? if not, nix the vtable

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:173: int64
sw_registration_id,
On 2015/08/28 02:53:11, michaeln wrote:
> |sw_registration_id| is not used, nix it?

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:640:
RefCountedRegistration* registration) {
On 2015/08/28 02:53:11, michaeln wrote:
> i think const scoped_refptr<T>& registration for this parameter instead of the
> raw ptr is our current convention for passing around refcounted objs

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:672: int64
sw_registration_id,
On 2015/08/28 02:53:11, michaeln wrote:
> I was wondering if sw_registration_id is needed? I see it being passed in
> parallel with the handle_id often and now i see why. The
> BackgroundSyncRegistration struct doesn't have a data member for the sw_reg_id
> from whence it came. The value actually is needed here to remove it form the
> active collection and to update storage properly. It might be nice to add that
> value to the struct so |handle_id| is all you need to get to everything there
is
> to know about a particular registration.

Acknowledged.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:204: void
AddActiveRegistration(
On 2015/08/28 02:53:11, michaeln wrote:
> thnx for making the subtle name changes

Acknowledged.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:28:
BackgroundSyncRegistrationHandle(
On 2015/08/28 02:53:11, michaeln wrote:
> can this constructor be put in the private section? does anything other than
the
> mgr (a friend) have any reason to construct one of these?

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:34:
DCHECK(background_sync_manager_);
On 2015/08/28 02:53:11, michaeln wrote:
> #include "base/logging.h" for dcheck

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:197: int64_t
service_worker_registration_id,
On 2015/08/28 02:53:11, michaeln wrote:
> if service_worker_registration_id needed? if not, nix it?

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.h (right):

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:36: int64_t id,
On 2015/08/28 02:53:11, michaeln wrote:
> int handle_id?

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:37: int64_t
sw_registration_id,
On 2015/08/28 02:53:11, michaeln wrote:
> is sw_registration_id needed, is handle_id not sufficient?

Acknowledged.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:51: int64_t
handle_id,
On 2015/08/28 02:53:11, michaeln wrote:
> int to match BackgroundSyncRegistrationHandle::HandleId

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.h:52: int64_t
service_worker_registration_id,
On 2015/08/28 02:53:11, michaeln wrote:
> is service_worker_registration_id needed?

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:64: int64_t id,
On 2015/08/28 02:53:11, michaeln wrote:
> this is an int handle_id? is that right?

Yes, conversion to int and the removal of tag will have to be done in follow-up
cls due to the three-legged Web* change.

TODO added.

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:142: void
BackgroundSyncProvider::releaseRegistration(int64_t handle_id) {
On 2015/08/28 02:53:11, michaeln wrote:
> int?

Yes, but for a future CL as it requires a WebSyncProvider change.

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:147: int64_t
handle_id,
On 2015/08/28 02:53:11, michaeln wrote:
> int?

Done.

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:28: class
BackgroundSyncProviderThreadProxy : public blink::WebSyncProvider,
On 2015/08/28 12:55:21, iclelland wrote:
> On 2015/08/28 02:53:11, michaeln wrote:
> > I don't think we're taking full advantage of mojo with the current design
> > pattern.
> > 
> > Mojo ipc gives us lightweight interface ptrs which can be bound to any
thread
> so
> > we can avoid doing heavyweight manual thread hopping. The lifetimes of the
> > objects on either side can be scoped to one another which can be help with
> > resource reclamation when things unwind.
> > 
> > What if instead of a 'proxy' each thread had its own BackgroundSyncProvider
> > instance with its own BackgroundSyncServicePtr interface ptr?
> 
> That's actually a good idea -- I wrote the original thread proxy, with the
> intention of having just one mojo channel from each renderer to the browser.
> Since the mojo interfaces are so light, though, and since mojo is going to do
a
> thread hop to its own UI thread regardless of where we call it from (as I
> understand it,) then it makes sense to try to avoid doing another one here, at
> the expense of a mojo connection per thread.
> 
> Can that refactoring be in a separate CL, or would you want to see it in this
> one? 

Agree that this should be done. Also agree it's for another CL.  Created bug
https://code.google.com/p/chromium/issues/detail?id=527601

https://codereview.chromium.org/1282013004/diff/490001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:42:
int64_t id,
On 2015/08/28 02:53:11, michaeln wrote:
> is this an int handle_id and are the other inputs needed?

Needs to be done in a follow-up cl. TODO added.


> 
> Might be out of scope but would it make sense to pass a WebSyncRegistration*
> here instead of passing an id value. If i understand the api properly, the
> caller receives that data type in response to the regiseter and get methods. I
> think it contains the handle_id value of interest.
> (similar to how WebServiceWorkerRegistration is not referred to by id in the
> WebKit api)

Ack. I'm torn on this, since the only thing that's needed is the id and passing
less is often better.

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:45:
provider->DuplicateRegistrationHandle(
On 2015/08/28 02:53:11, michaeln wrote:
> Eeek, the thread and msg round trips are kinda ugly. This is akin to putting a
> sync ipc in the loop since msg queues have to flush in both directions prior
to
> proceeding with the event handleing.

Agreed. On the plus side, this isn't a latency-sensitive event. In fact, it's
whole purpose is to fire when convenient for the UA. That said, I'd rather avoid
the round-trip if possible. TODO added.

> 
> I think can leak bgsync handle references for the lifetime of the renderer
> process. Suppose...
> 
> * DuplicateRegistrationHandle is called.
> * Browser-side does its work stashes the handleptr in
> BackgroundSyncServiceImpl.active_handles_ and puts the goods in transit.
> * Worker thread terminates.
> * Main thread receives the goods and calls
> DuplicateRegistrationHandleCallbackOnMainThread
> * But... DuplicateRegistrationHandleCallbackOnWorkerThread is never called
> * And... releaseRegistration(handle_id) is never called
> * The BackgroundSyncServiceImpl eventually gets deleted when the renderer
exits
> and that does clean up the handle.
> 
> 

This can also happen to register, getRegistration, and getRegistrations. This
leak will be taken care of once we move to the per-service-worker mojo channel
instead of the per-renderer registration. I've created bug
https://code.google.com/p/chromium/issues/detail?id=527601 to do the mojo work.

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:358:
provider_context_->GetRegistrationInfoAndVersionAttributes(&registration_info,
On 2015/08/28 02:53:11, michaeln wrote:
> there's an early return in GetRegistrationInfoAndVersionAttributes where it
can
> return false but for a worker context it never should go down that route, can
> you add a...
> 
> DCHECK_NE(registration_info.registration_id,
> kInvalidServiceWorkerRegistrationId)

Done.

[jkarlin, 2015-09-03 11:21:53]

Patchset #23 (id:550001) has been deleted

[jkarlin, 2015-09-03 11:22:10]

Patchset #23 (id:550001) has been deleted

[jkarlin, 2015-09-03 14:21:02]

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1282013004/diff/490001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:358:
provider_context_->GetRegistrationInfoAndVersionAttributes(&registration_info,
On 2015/09/02 23:51:41, jkarlin wrote:
> On 2015/08/28 02:53:11, michaeln wrote:
> > there's an early return in GetRegistrationInfoAndVersionAttributes where it
> can
> > return false but for a worker context it never should go down that route,
can
> > you add a...
> > 
> > DCHECK_NE(registration_info.registration_id,
> > kInvalidServiceWorkerRegistrationId)
> 
> Done.

Hmm, the DCHECK_NE is failing for
IN_PROC_BROWSER_TEST_F(EmbeddedWorkerBrowserTest, StartAndStop) because it
doesn't create a registration before starting the worker :(

[jkarlin, 2015-09-03 21:11:32]

michaeln, I think we're almost there, PTAL! Thank you.

[jkarlin, 2015-09-08 19:03:14]

Ping michaeln?

[jkarlin, 2015-09-08 20:57:49]

On 2015/09/08 19:03:14, jkarlin wrote:
> Ping michaeln?

Note that I still have to deal with the StartAndStop browser test failing due to
adding a dcheck for a valid registration id.

[jkarlin, 2015-09-08 21:06:13]

On 2015/09/08 20:57:49, jkarlin wrote:
> On 2015/09/08 19:03:14, jkarlin wrote:
> > Ping michaeln?
> 
> Note that I still have to deal with the StartAndStop browser test failing due
to
> adding a dcheck for a valid registration id.

Fixing that actually looks non-trivial as the test doesn't create a registration
at all. Is that test necessary? The one below it creates a registration and
tests the same path (I believe).

[michaeln, 2015-09-10 21:21:34]

lgtm, kinda, its a bunch of complexity that doesn't quite fit together right
yet?

I think taking care of the per-thread mojointf ptr and the sync round trips for
the handle ownsership xfer would be good improvements.

> Fixing that actually looks non-trivial as the test doesn't create a
registration
> at all. Is that test necessary? The one below it creates a registration and
> tests the same path (I believe).

I think you're right that the first test can be deleted since the second test is
so similar.

https://codereview.chromium.org/1282013004/diff/630001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/630001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:45: //
TODO(jkarlin): Find a way to claim the handle safely without requiring a
I guess the trouble is having to get a new id value back.

I'd be good to revisit this once the per-thread BackgroundSyncServicePtr is in
place.

[jkarlin, 2015-09-10 23:26:01]

https://codereview.chromium.org/1282013004/diff/630001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1282013004/diff/630001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:45: //
TODO(jkarlin): Find a way to claim the handle safely without requiring a
On 2015/09/10 21:21:33, michaeln wrote:
> I guess the trouble is having to get a new id value back.
> 
> I'd be good to revisit this once the per-thread BackgroundSyncServicePtr is in
> place.
> 

Acknowledged.

[jkarlin, 2015-09-10 23:31:14]

On 2015/09/10 21:21:34, michaeln wrote:
> lgtm, kinda, its a bunch of complexity that doesn't quite fit together right
> yet?
> 
> I think taking care of the per-thread mojointf ptr and the sync round trips
for
> the handle ownsership xfer would be good improvements.

chasej@ is already working on the per-thread mojo. The round-trip issue has a
bug: https://code.google.com/p/chromium/issues/detail?id=530372

> 
> > Fixing that actually looks non-trivial as the test doesn't create a
> registration
> > at all. Is that test necessary? The one below it creates a registration and
> > tests the same path (I believe).
> 
> I think you're right that the first test can be deleted since the second test
is
> so similar.

Done.

[jkarlin, 2015-09-11 00:05:57]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-09-11 00:05:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, tsepez@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1282013004/#ps670001
(title: "Removed test")

[commit-bot: I haz the power, 2015-09-11 00:06:19]

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

[commit-bot: I haz the power, 2015-09-11 00:52:10]

Committed patchset #28 (id:670001)

[commit-bot: I haz the power, 2015-09-11 00:52:59]

Patchset 28 (id:??) landed as
https://crrev.com/14977394017e56a86c5516e3c4b3f8988a2031f5
Cr-Commit-Position: refs/heads/master@{#348303}

[jkarlin, 2015-09-11 11:04:59]

On 2015/09/10 21:21:34, michaeln wrote:
> lgtm, kinda, its a bunch of complexity that doesn't quite fit together right
> yet?

Forgot to address this question earlier. Yes, the purpose of the complexity is
for a new feature (registration.done) in an upcoming CL. This is just the grunge
work to get there.

[commit-bot: I haz the power, 2015-09-23 12:17:12]

Patchset 28 (id:??) landed as
https://crrev.com/14977394017e56a86c5516e3c4b3f8988a2031f5
Cr-Commit-Position: refs/heads/master@{#348303}