sync: Consistently refcount ModelSafeWorkers

sync: Consistently refcount ModelSafeWorkers

Certain interfaces in sync had signatures that included vectors of raw
pointers to ModelSafeWorkers.  The use of raw pointers to refcounted
objects looks a lot like a bug waiting to happen.

This CL replaces any use of std::vector<ModelSafeWorker*> with
std::vector<scoped_refptr<ModelSafeWorker> >.  This is not expected to
alter sync behavior in any significant way.

BUG=332251
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244329

[rlarocque, 2014-01-09 22:47:54]

Here's the patch to fix the ModelSafeWorker issue pointed out in
https://codereview.chromium.org/93433006/.

Haitao: Please review.

[haitaol1, 2014-01-10 18:15:11]

On 2014/01/09 22:47:54, rlarocque wrote:
> Here's the patch to fix the ModelSafeWorker issue pointed out in
> https://codereview.chromium.org/93433006/.
> 
> Haitao: Please review.

I don't think this change is necessary. ModelSafeWorkers are owned by
SyncBackendRegistrar by the design of lock-free shutdown.
Making them ref-counted may introduce subtle bugs. I would urge caution if
there's no evidence indicating something is wrong.

[rlarocque, 2014-01-10 18:42:02]

On 2014/01/10 18:15:11, haitaol1 wrote:
> On 2014/01/09 22:47:54, rlarocque wrote:
> > Here's the patch to fix the ModelSafeWorker issue pointed out in
> > https://codereview.chromium.org/93433006/.
> > 
> > Haitao: Please review.
> 
> I don't think this change is necessary. ModelSafeWorkers are owned by
> SyncBackendRegistrar by the design of lock-free shutdown.
> Making them ref-counted may introduce subtle bugs. I would urge caution if
> there's no evidence indicating something is wrong.

I was hoping you would say something like that.  That was my first response,
too.  Now let's see if I can convince you otherwise.

I think the reason these weren't refcounted is due to history.  A long time ago,
the workers were owned by the UI thread but accessed (via shared memory and a
lock) from the sync thread.  Maybe we thought the lock and the design of the
SyncBackendRegistrar made the refcounting redundant?  Also, at the time we had a
pretty strong guarantee that the UI thread components would be shut down before
the sync thread components.  That guarantee is not as strong as it used to be. 
The current code looks safe, but it's possible that future changes to the code
could make it unsafe.

The argument against using raw pointers is that it's almost always a bug to
reference a refcountable object using raw pointers.  Tim pointed out that the
danger here is that if the SyncBackendRegistrar were for some reason to lose its
reference to a worker while backend initialization is in progress, the program
would crash as soon as the backend components tried to take a reference to the
(now deleted) worker.  I don't think that's possible today, but it seems like a
reasonable concern.

See discussion at
https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re....

[rlarocque, 2014-01-10 18:43:57]

On 2014/01/10 18:42:02, rlarocque wrote:
> On 2014/01/10 18:15:11, haitaol1 wrote:
> > On 2014/01/09 22:47:54, rlarocque wrote:
> > > Here's the patch to fix the ModelSafeWorker issue pointed out in
> > > https://codereview.chromium.org/93433006/.
> > > 
> > > Haitao: Please review.
> > 
> > I don't think this change is necessary. ModelSafeWorkers are owned by
> > SyncBackendRegistrar by the design of lock-free shutdown.
> > Making them ref-counted may introduce subtle bugs. I would urge caution if
> > there's no evidence indicating something is wrong.
> 
> I was hoping you would say something like that.  That was my first response,
> too.  Now let's see if I can convince you otherwise.
> 
> I think the reason these weren't refcounted is due to history.  A long time
ago,
> the workers were owned by the UI thread but accessed (via shared memory and a
> lock) from the sync thread.  Maybe we thought the lock and the design of the
> SyncBackendRegistrar made the refcounting redundant?  Also, at the time we had
a
> pretty strong guarantee that the UI thread components would be shut down
before
> the sync thread components.  That guarantee is not as strong as it used to be.

> The current code looks safe, but it's possible that future changes to the code
> could make it unsafe.
> 
> The argument against using raw pointers is that it's almost always a bug to
> reference a refcountable object using raw pointers.  Tim pointed out that the
> danger here is that if the SyncBackendRegistrar were for some reason to lose
its
> reference to a worker while backend initialization is in progress, the program
> would crash as soon as the backend components tried to take a reference to the
> (now deleted) worker.  I don't think that's possible today, but it seems like
a
> reasonable concern.
> 
> See discussion at
>
https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re....

Correction: The sentence

...pretty strong guarantee that the UI thread components would be shut down
before
the sync thread components.

should read

..pretty strong guarantee that the UI thread components would be shut down
*after*
the sync thread components.

[haitaol1, 2014-01-10 19:40:17]

On 2014/01/10 18:43:57, rlarocque wrote:
> On 2014/01/10 18:42:02, rlarocque wrote:
> > On 2014/01/10 18:15:11, haitaol1 wrote:
> > > On 2014/01/09 22:47:54, rlarocque wrote:
> > > > Here's the patch to fix the ModelSafeWorker issue pointed out in
> > > > https://codereview.chromium.org/93433006/.
> > > > 
> > > > Haitao: Please review.
> > > 
> > > I don't think this change is necessary. ModelSafeWorkers are owned by
> > > SyncBackendRegistrar by the design of lock-free shutdown.
> > > Making them ref-counted may introduce subtle bugs. I would urge caution if
> > > there's no evidence indicating something is wrong.
> > 
> > I was hoping you would say something like that.  That was my first response,
> > too.  Now let's see if I can convince you otherwise.
> > 
> > I think the reason these weren't refcounted is due to history.  A long time
> ago,
> > the workers were owned by the UI thread but accessed (via shared memory and
a
> > lock) from the sync thread.  Maybe we thought the lock and the design of the
> > SyncBackendRegistrar made the refcounting redundant?  Also, at the time we
had
> a
> > pretty strong guarantee that the UI thread components would be shut down
> before
> > the sync thread components.  That guarantee is not as strong as it used to
be.
> 
> > The current code looks safe, but it's possible that future changes to the
code
> > could make it unsafe.
> > 
> > The argument against using raw pointers is that it's almost always a bug to
> > reference a refcountable object using raw pointers.  Tim pointed out that
the
> > danger here is that if the SyncBackendRegistrar were for some reason to lose
> its
> > reference to a worker while backend initialization is in progress, the
program
> > would crash as soon as the backend components tried to take a reference to
the
> > (now deleted) worker.  I don't think that's possible today, but it seems
like
> a
> > reasonable concern.
> > 
> > See discussion at
> >
>
https://codereview.chromium.org/93433006/diff/90001/sync/engine/model_type_re....
> 
> Correction: The sentence
> 
> ...pretty strong guarantee that the UI thread components would be shut down
> before
> the sync thread components.
> 
> should read
> 
> ..pretty strong guarantee that the UI thread components would be shut down
> *after*
> the sync thread components.

See the comment at the end of sync_backend_registrar.h about why sync_thread_ is
placed last to make sure backend is destroyed before other members.

[haitaol1, 2014-01-10 22:34:19]

I still believe it's unnecessary to pass ref-counted objects on stack. But if
that's the opinion of majority, I'm fine with that.

LGTM

[rlarocque, 2014-01-10 22:49:51]

On 2014/01/10 22:34:19, haitaol1 wrote:
> I still believe it's unnecessary to pass ref-counted objects on stack. But if
> that's the opinion of majority, I'm fine with that.
> 
> LGTM

Thanks.

[commit-bot: I haz the power, 2014-01-10 23:27:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/130193002/250001

[commit-bot: I haz the power, 2014-01-11 03:11:12]

Change committed as 244329