Remove friend classes from PasswordStore.

Remove friend classes from PasswordStore.

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

[vasilii, 2014-04-23 16:54:41]

Hi,

please review
Ilya: password_manager
Nicolas: sync

[Nicolas Zea, 2014-04-23 19:43:15]

sync lgtm

[Ilya Sherman, 2014-04-23 20:40:45]

I don't think this is the right approach (see inline).  Instead, we should
decide which methods are actually meant to be public, and which methods Sync
should stop using.  This CL currently just makes all of the methods public, but
in a slightly obfuscated way.

Perhaps you intended to have PasswordStoreSync inherit from PasswordStore?  That
would make at least a little bit more sense to me -- then other clients of
PasswordStore would not be affected by this change.

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store.h:195: virtual bool
ScheduleTask(const base::Closure& task) OVERRIDE;
nit: Overridden methods from PasswordStoreSync should be grouped together and
documented as "// PasswordStoreSync:"

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store.h:213: const
autofill::PasswordForm& form) = 0;
These are no longer needed here, right?

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:19: public:
You've currently made a bunch of formerly private methods public.  That doesn't
really seem like an improvement to me.  Why introduce an extra interface just to
increase the methods' visibility?  You could just elevate their visibility in
the PasswordStore's declaration instead.

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:34: virtual void
NotifyLoginsChanged(const PasswordStoreChangeList& changes) = 0;
All of the methods in this class should be documented.

[Mike Mammarella, 2014-04-24 00:08:17]

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:19: public:
On 2014/04/23 20:40:46, Ilya Sherman wrote:
> You've currently made a bunch of formerly private methods public.  That
doesn't
> really seem like an improvement to me.  Why introduce an extra interface just
to
> increase the methods' visibility?  You could just elevate their visibility in
> the PasswordStore's declaration instead.

So, when these friend declarations were first added, we didn't yet have OWNERS.
I would have objected to it at the time if I'd been asked to review the code. It
has come up again a few times since, and I've tried to get some traction on
actually using the public API rather than the synchronous version rather than
friending this class, but without much success. It has never been safe for sync
to use these methods: they can deadlock in some cases when used from outside the
class, which is why they are private to begin with. We've only been able to get
away with it because the deadlocks are rare.

The most recent time this has come up, the argument was made that we need to
think about sync as more of a library that datatypes use to sync themselves,
rather than as a separate service that syncs the data owned by all the
datatypes, and thus we should use whatever internal APIs the implementation of
the datatype uses in order to get the data to sync. OK, but that still doesn't
solve the deadlock problem with the way the sync code is currently set up
(which, last I checked, is more like a separate service).

I haven't worked on Chromium for a few years now, so some of my information may
be getting increasingly stale. But I don't think much has changed in the
password store, other than apparently moving it to a new location in the source
tree, so probably this is a mostly accurate statement: you cannot safely access
the password store synchronously, at all, ever. These private synchronous
methods may block indefinitely waiting for the user to act on OS-level dialogs,
and may wait indefinitely for other threads to perform work. This design is
unfortunately dictated by the way the OS interfaces for password storage work,
and cannot be changed.

It would be much better to actually fix the sync code that calls these methods
to use the public PasswordStore API instead.

[vasilii, 2014-04-24 15:04:52]

Ilya: I didn't understand your proposal. Do you want to return PasswordStoreSync
from PasswordStoreFactory::GetForProfile? It's equivalent to making the methods
public.
Alternative could be to privately inherit PasswordStore from PasswordStoreSync.

Mike: the refactoring was done by this doc
https://docs.google.com/a/google.com/document/d/1L9NPFyv2-TauXAmI6Y8L3WVqBwlR....
It says that with the new API the deadlock isn't possible anymore. I don't see
how we can use async methods to implement SyncableService.

[Ilya Sherman, 2014-04-25 00:27:18]

Fundamentally, I think we should figure out for certain whether calls to these
methods are safe or not.  Based on the last discussion that I was involved in, I
think that we'd concluded that they weren't necessarily safe, but that the new
Sync API was in no worse shape than the old one.  If that's still correct, we
should focus on fixing the code to be safe.  It's probably worth auditing what
all the PasswordStore methods can block on.

On 2014/04/24 15:04:52, vasilii wrote:
> Ilya: I didn't understand your proposal. Do you want to return
PasswordStoreSync
> from PasswordStoreFactory::GetForProfile? It's equivalent to making the
methods
> public.
> Alternative could be to privately inherit PasswordStore from
PasswordStoreSync.

Before we talk about concrete proposals to expose these methods, let's think
about why we want to expose some methods *only* to Sync.  I don't think it's
appropriate to claim that some methods are private, except visible to Sync.  If
methods need to be visible to Sync, then they probably ought to be public.  If
it's not generally safe for other classes to call these methods, then Sync
probably shouldn't call them either.

Put another way: What is the Sync code doing to make calls to these methods
safe?  If it's just a matter of running on the correct thread, then perhaps
these methods could be public, but DCHECK that they're called from the
appropriate thread?  If there's something else, can we DCHECK that as well?

[vasilii, 2014-04-25 09:03:14]

It's mostly about calling on the background thread.
ScheduleTask - can be public
FillAutofillableLogins - GetAutofillableLogins
FillBlacklistLogins - GetBlacklistLogins
AddLoginImpl - AddLogin
UpdateLoginImpl - UpdateLogin
RemoveLoginImpl - RemoveLogin
NotifyLoginsChanged - used for optimization to update observers only once.
Internal methods like AddLoginImpl don't notify observers automatically.
Other classes aren't supposed to run on the background thread. Therefore, they
aren't supposed to call internal methods.

[Ilya Sherman, 2014-04-25 18:22:01]

On 2014/04/25 09:03:14, vasilii wrote:
> It's mostly about calling on the background thread.
> ScheduleTask - can be public
> FillAutofillableLogins - GetAutofillableLogins
> FillBlacklistLogins - GetBlacklistLogins
> AddLoginImpl - AddLogin
> UpdateLoginImpl - UpdateLogin
> RemoveLoginImpl - RemoveLogin
> NotifyLoginsChanged - used for optimization to update observers only once.
> Internal methods like AddLoginImpl don't notify observers automatically.
> Other classes aren't supposed to run on the background thread. Therefore, they
> aren't supposed to call internal methods.

So, can we just DCHECK that the methods are called from the correct thread?

[vasilii, 2014-04-28 15:18:32]

NotifyLoginsChanged contains this check. Do you want to add the check to all
private methods (like AddLoginImpl) of all subclasses?

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store.h:195: virtual bool
ScheduleTask(const base::Closure& task) OVERRIDE;
On 2014/04/23 20:40:46, Ilya Sherman wrote:
> nit: Overridden methods from PasswordStoreSync should be grouped together and
> documented as "// PasswordStoreSync:"

Done.

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store.h:213: const
autofill::PasswordForm& form) = 0;
On 2014/04/23 20:40:46, Ilya Sherman wrote:
> These are no longer needed here, right?

Done.

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:19: public:
On 2014/04/24 00:08:18, Mike Mammarella wrote:
> On 2014/04/23 20:40:46, Ilya Sherman wrote:
> > You've currently made a bunch of formerly private methods public.  That
> doesn't
> > really seem like an improvement to me.  Why introduce an extra interface
just
> to
> > increase the methods' visibility?  You could just elevate their visibility
in
> > the PasswordStore's declaration instead.
> 
> So, when these friend declarations were first added, we didn't yet have
OWNERS.
> I would have objected to it at the time if I'd been asked to review the code.
It
> has come up again a few times since, and I've tried to get some traction on
> actually using the public API rather than the synchronous version rather than
> friending this class, but without much success. It has never been safe for
sync
> to use these methods: they can deadlock in some cases when used from outside
the
> class, which is why they are private to begin with. We've only been able to
get
> away with it because the deadlocks are rare.
> 
> The most recent time this has come up, the argument was made that we need to
> think about sync as more of a library that datatypes use to sync themselves,
> rather than as a separate service that syncs the data owned by all the
> datatypes, and thus we should use whatever internal APIs the implementation of
> the datatype uses in order to get the data to sync. OK, but that still doesn't
> solve the deadlock problem with the way the sync code is currently set up
> (which, last I checked, is more like a separate service).
> 
> I haven't worked on Chromium for a few years now, so some of my information
may
> be getting increasingly stale. But I don't think much has changed in the
> password store, other than apparently moving it to a new location in the
source
> tree, so probably this is a mostly accurate statement: you cannot safely
access
> the password store synchronously, at all, ever. These private synchronous
> methods may block indefinitely waiting for the user to act on OS-level
dialogs,
> and may wait indefinitely for other threads to perform work. This design is
> unfortunately dictated by the way the OS interfaces for password storage work,
> and cannot be changed.
> 
> It would be much better to actually fix the sync code that calls these methods
> to use the public PasswordStore API instead.

I can make them protected and put a friend declaration here. Ideally
PasswordStore would privately implement this interface.

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:34: virtual void
NotifyLoginsChanged(const PasswordStoreChangeList& changes) = 0;
On 2014/04/23 20:40:46, Ilya Sherman wrote:
> All of the methods in this class should be documented.

Done.

[Ilya Sherman, 2014-04-28 21:38:47]

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:19: public:
On 2014/04/28 15:18:32, vasilii wrote:
> On 2014/04/24 00:08:18, Mike Mammarella wrote:
> > On 2014/04/23 20:40:46, Ilya Sherman wrote:
> > > You've currently made a bunch of formerly private methods public.  That
> > doesn't
> > > really seem like an improvement to me.  Why introduce an extra interface
> just
> > to
> > > increase the methods' visibility?  You could just elevate their visibility
> in
> > > the PasswordStore's declaration instead.
> > 
> > So, when these friend declarations were first added, we didn't yet have
> OWNERS.
> > I would have objected to it at the time if I'd been asked to review the
code.
> It
> > has come up again a few times since, and I've tried to get some traction on
> > actually using the public API rather than the synchronous version rather
than
> > friending this class, but without much success. It has never been safe for
> sync
> > to use these methods: they can deadlock in some cases when used from outside
> the
> > class, which is why they are private to begin with. We've only been able to
> get
> > away with it because the deadlocks are rare.
> > 
> > The most recent time this has come up, the argument was made that we need to
> > think about sync as more of a library that datatypes use to sync themselves,
> > rather than as a separate service that syncs the data owned by all the
> > datatypes, and thus we should use whatever internal APIs the implementation
of
> > the datatype uses in order to get the data to sync. OK, but that still
doesn't
> > solve the deadlock problem with the way the sync code is currently set up
> > (which, last I checked, is more like a separate service).
> > 
> > I haven't worked on Chromium for a few years now, so some of my information
> may
> > be getting increasingly stale. But I don't think much has changed in the
> > password store, other than apparently moving it to a new location in the
> source
> > tree, so probably this is a mostly accurate statement: you cannot safely
> access
> > the password store synchronously, at all, ever. These private synchronous
> > methods may block indefinitely waiting for the user to act on OS-level
> dialogs,
> > and may wait indefinitely for other threads to perform work. This design is
> > unfortunately dictated by the way the OS interfaces for password storage
work,
> > and cannot be changed.
> > 
> > It would be much better to actually fix the sync code that calls these
methods
> > to use the public PasswordStore API instead.
> 
> I can make them protected and put a friend declaration here. Ideally
> PasswordStore would privately implement this interface.

Protected + friend class in the interface seems like an ok compromise, since the
interface describes exactly what methods should be friended.  I'm not seeing the
advantage of private inheritance, as you're proposing.

https://codereview.chromium.org/246253014/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:20:
std::vector<autofill::PasswordForm*>* forms) = 0;
nit: Please leave blank lines between documented methods.

[vasilii, 2014-04-29 11:14:04]

I added DCHECKs to examine the current thread.

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:19: public:
On 2014/04/28 21:38:48, Ilya Sherman wrote:
> On 2014/04/28 15:18:32, vasilii wrote:
> > On 2014/04/24 00:08:18, Mike Mammarella wrote:
> > > On 2014/04/23 20:40:46, Ilya Sherman wrote:
> > > > You've currently made a bunch of formerly private methods public.  That
> > > doesn't
> > > > really seem like an improvement to me.  Why introduce an extra interface
> > just
> > > to
> > > > increase the methods' visibility?  You could just elevate their
visibility
> > in
> > > > the PasswordStore's declaration instead.
> > > 
> > > So, when these friend declarations were first added, we didn't yet have
> > OWNERS.
> > > I would have objected to it at the time if I'd been asked to review the
> code.
> > It
> > > has come up again a few times since, and I've tried to get some traction
on
> > > actually using the public API rather than the synchronous version rather
> than
> > > friending this class, but without much success. It has never been safe for
> > sync
> > > to use these methods: they can deadlock in some cases when used from
outside
> > the
> > > class, which is why they are private to begin with. We've only been able
to
> > get
> > > away with it because the deadlocks are rare.
> > > 
> > > The most recent time this has come up, the argument was made that we need
to
> > > think about sync as more of a library that datatypes use to sync
themselves,
> > > rather than as a separate service that syncs the data owned by all the
> > > datatypes, and thus we should use whatever internal APIs the
implementation
> of
> > > the datatype uses in order to get the data to sync. OK, but that still
> doesn't
> > > solve the deadlock problem with the way the sync code is currently set up
> > > (which, last I checked, is more like a separate service).
> > > 
> > > I haven't worked on Chromium for a few years now, so some of my
information
> > may
> > > be getting increasingly stale. But I don't think much has changed in the
> > > password store, other than apparently moving it to a new location in the
> > source
> > > tree, so probably this is a mostly accurate statement: you cannot safely
> > access
> > > the password store synchronously, at all, ever. These private synchronous
> > > methods may block indefinitely waiting for the user to act on OS-level
> > dialogs,
> > > and may wait indefinitely for other threads to perform work. This design
is
> > > unfortunately dictated by the way the OS interfaces for password storage
> work,
> > > and cannot be changed.
> > > 
> > > It would be much better to actually fix the sync code that calls these
> methods
> > > to use the public PasswordStore API instead.
> > 
> > I can make them protected and put a friend declaration here. Ideally
> > PasswordStore would privately implement this interface.
> 
> Protected + friend class in the interface seems like an ok compromise, since
the
> interface describes exactly what methods should be friended.  I'm not seeing
the
> advantage of private inheritance, as you're proposing.

With private inheritance these methods are private from outside and none of the
classes can cast PasswordStore to PasswordStoreSync. However PasswordStore can
cast |this| to the interface and pass the pointer down to
PasswordSyncableService. For me an interface with protected methods looks
strange.

https://codereview.chromium.org/246253014/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:20:
std::vector<autofill::PasswordForm*>* forms) = 0;
On 2014/04/28 21:38:48, Ilya Sherman wrote:
> nit: Please leave blank lines between documented methods.

Done.

[Ilya Sherman, 2014-04-29 20:35:01]

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:19: public:
On 2014/04/29 11:14:04, vasilii wrote:
> On 2014/04/28 21:38:48, Ilya Sherman wrote:
> > On 2014/04/28 15:18:32, vasilii wrote:
> > > On 2014/04/24 00:08:18, Mike Mammarella wrote:
> > > > On 2014/04/23 20:40:46, Ilya Sherman wrote:
> > > > > You've currently made a bunch of formerly private methods public. 
That
> > > > doesn't
> > > > > really seem like an improvement to me.  Why introduce an extra
interface
> > > just
> > > > to
> > > > > increase the methods' visibility?  You could just elevate their
> visibility
> > > in
> > > > > the PasswordStore's declaration instead.
> > > > 
> > > > So, when these friend declarations were first added, we didn't yet have
> > > OWNERS.
> > > > I would have objected to it at the time if I'd been asked to review the
> > code.
> > > It
> > > > has come up again a few times since, and I've tried to get some traction
> on
> > > > actually using the public API rather than the synchronous version rather
> > than
> > > > friending this class, but without much success. It has never been safe
for
> > > sync
> > > > to use these methods: they can deadlock in some cases when used from
> outside
> > > the
> > > > class, which is why they are private to begin with. We've only been able
> to
> > > get
> > > > away with it because the deadlocks are rare.
> > > > 
> > > > The most recent time this has come up, the argument was made that we
need
> to
> > > > think about sync as more of a library that datatypes use to sync
> themselves,
> > > > rather than as a separate service that syncs the data owned by all the
> > > > datatypes, and thus we should use whatever internal APIs the
> implementation
> > of
> > > > the datatype uses in order to get the data to sync. OK, but that still
> > doesn't
> > > > solve the deadlock problem with the way the sync code is currently set
up
> > > > (which, last I checked, is more like a separate service).
> > > > 
> > > > I haven't worked on Chromium for a few years now, so some of my
> information
> > > may
> > > > be getting increasingly stale. But I don't think much has changed in the
> > > > password store, other than apparently moving it to a new location in the
> > > source
> > > > tree, so probably this is a mostly accurate statement: you cannot safely
> > > access
> > > > the password store synchronously, at all, ever. These private
synchronous
> > > > methods may block indefinitely waiting for the user to act on OS-level
> > > dialogs,
> > > > and may wait indefinitely for other threads to perform work. This design
> is
> > > > unfortunately dictated by the way the OS interfaces for password storage
> > work,
> > > > and cannot be changed.
> > > > 
> > > > It would be much better to actually fix the sync code that calls these
> > methods
> > > > to use the public PasswordStore API instead.
> > > 
> > > I can make them protected and put a friend declaration here. Ideally
> > > PasswordStore would privately implement this interface.
> > 
> > Protected + friend class in the interface seems like an ok compromise, since
> the
> > interface describes exactly what methods should be friended.  I'm not seeing
> the
> > advantage of private inheritance, as you're proposing.
> 
> With private inheritance these methods are private from outside and none of
the
> classes can cast PasswordStore to PasswordStoreSync. However PasswordStore can
> cast |this| to the interface and pass the pointer down to
> PasswordSyncableService. For me an interface with protected methods looks
> strange.

Ah, right, I forgot that the PasswordSyncableService is owned by the
PasswordStore.  In that case, private inheritance sounds ok to me.  Let's also
document the exact reason why we're using private inheritance.

That assumes, though, that the PasswordSyncableService *only* needs to call
private methods on the PasswordStore, and doesn't need to call any public ones. 
That actually seems a little bit improbable to me; but if it works, it works.

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
components/password_manager/core/browser/password_store.h:175: bool
ScheduleTask(const base::Closure& task);
Hmm, why did this become public?

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:15: //
PasswordStore interface for PasswordSyncableService.
Please expand this documentation to clarify why an extra interface is needed for
this purpose.

[vasilii, 2014-04-30 14:50:50]

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/1/components/password_manager/...
components/password_manager/core/browser/password_store_sync.h:19: public:
On 2014/04/29 20:35:01, Ilya Sherman wrote:
> On 2014/04/29 11:14:04, vasilii wrote:
> > On 2014/04/28 21:38:48, Ilya Sherman wrote:
> > > On 2014/04/28 15:18:32, vasilii wrote:
> > > > On 2014/04/24 00:08:18, Mike Mammarella wrote:
> > > > > On 2014/04/23 20:40:46, Ilya Sherman wrote:
> > > > > > You've currently made a bunch of formerly private methods public. 
> That
> > > > > doesn't
> > > > > > really seem like an improvement to me.  Why introduce an extra
> interface
> > > > just
> > > > > to
> > > > > > increase the methods' visibility?  You could just elevate their
> > visibility
> > > > in
> > > > > > the PasswordStore's declaration instead.
> > > > > 
> > > > > So, when these friend declarations were first added, we didn't yet
have
> > > > OWNERS.
> > > > > I would have objected to it at the time if I'd been asked to review
the
> > > code.
> > > > It
> > > > > has come up again a few times since, and I've tried to get some
traction
> > on
> > > > > actually using the public API rather than the synchronous version
rather
> > > than
> > > > > friending this class, but without much success. It has never been safe
> for
> > > > sync
> > > > > to use these methods: they can deadlock in some cases when used from
> > outside
> > > > the
> > > > > class, which is why they are private to begin with. We've only been
able
> > to
> > > > get
> > > > > away with it because the deadlocks are rare.
> > > > > 
> > > > > The most recent time this has come up, the argument was made that we
> need
> > to
> > > > > think about sync as more of a library that datatypes use to sync
> > themselves,
> > > > > rather than as a separate service that syncs the data owned by all the
> > > > > datatypes, and thus we should use whatever internal APIs the
> > implementation
> > > of
> > > > > the datatype uses in order to get the data to sync. OK, but that still
> > > doesn't
> > > > > solve the deadlock problem with the way the sync code is currently set
> up
> > > > > (which, last I checked, is more like a separate service).
> > > > > 
> > > > > I haven't worked on Chromium for a few years now, so some of my
> > information
> > > > may
> > > > > be getting increasingly stale. But I don't think much has changed in
the
> > > > > password store, other than apparently moving it to a new location in
the
> > > > source
> > > > > tree, so probably this is a mostly accurate statement: you cannot
safely
> > > > access
> > > > > the password store synchronously, at all, ever. These private
> synchronous
> > > > > methods may block indefinitely waiting for the user to act on OS-level
> > > > dialogs,
> > > > > and may wait indefinitely for other threads to perform work. This
design
> > is
> > > > > unfortunately dictated by the way the OS interfaces for password
storage
> > > work,
> > > > > and cannot be changed.
> > > > > 
> > > > > It would be much better to actually fix the sync code that calls these
> > > methods
> > > > > to use the public PasswordStore API instead.
> > > > 
> > > > I can make them protected and put a friend declaration here. Ideally
> > > > PasswordStore would privately implement this interface.
> > > 
> > > Protected + friend class in the interface seems like an ok compromise,
since
> > the
> > > interface describes exactly what methods should be friended.  I'm not
seeing
> > the
> > > advantage of private inheritance, as you're proposing.
> > 
> > With private inheritance these methods are private from outside and none of
> the
> > classes can cast PasswordStore to PasswordStoreSync. However PasswordStore
can
> > cast |this| to the interface and pass the pointer down to
> > PasswordSyncableService. For me an interface with protected methods looks
> > strange.
> 
> Ah, right, I forgot that the PasswordSyncableService is owned by the
> PasswordStore.  In that case, private inheritance sounds ok to me.  Let's also
> document the exact reason why we're using private inheritance.
> 
> That assumes, though, that the PasswordSyncableService *only* needs to call
> private methods on the PasswordStore, and doesn't need to call any public
ones. 
> That actually seems a little bit improbable to me; but if it works, it works.

I was a bit wrong above.
static_cast<PasswordStoreSync*>((PasswordStore*)0); // Error.
(PasswordStoreSync*) ((PasswordStore*)0); // OK.

PasswordSyncableService may call public methods as well. We can add them to
PasswordStoreSync, PasswordStore redeclares them as public.

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
components/password_manager/core/browser/password_store.h:175: bool
ScheduleTask(const base::Closure& task);
On 2014/04/29 20:35:01, Ilya Sherman wrote:
> Hmm, why did this become public?

PasswordModelWorker and PasswordDataTypeController post tasks on the background
thread. They don't need to access other PasswordStore methods.
Also I was able to remove friend passwords_helper:: methods.

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/40001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:15: //
PasswordStore interface for PasswordSyncableService.
On 2014/04/29 20:35:01, Ilya Sherman wrote:
> Please expand this documentation to clarify why an extra interface is needed
for
> this purpose.

Done.

[Mike Mammarella, 2014-04-30 17:35:50]

I'm still not convinced the deadlock potential is actually resolved, but this
arrangement doesn't seem any worse than the current state, either. (Except in
that it gets rid of the friend declarations, which were a reminder of the
underlying problem. It remains the case that you don't really get any guarantees
by using the synchronous methods; the data can still be modified from outside
the process.)

That being said, this looks fine to me.

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:16: // classes.
It seems worthwhile to also mention the thread restrictions that apply when
calling these methods.

[Ilya Sherman, 2014-05-01 00:10:07]

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store.h:200: // These will be
run in PasswordStore's own thread.
nit: What does "these" refer to?

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store.h:210: const
autofill::PasswordForm& form) = 0;
Is this still necessary with protected inheritance?  If so, why?

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:15: //
synchronous methods of PasswordStore which shouldn't be accessible for other
nit: "accessible for" -> "accessible to"

[vasilii, 2014-05-02 11:45:37]

On 2014/04/30 17:35:50, Mike Mammarella wrote:
> I'm still not convinced the deadlock potential is actually resolved, but this
> arrangement doesn't seem any worse than the current state, either. (Except in
> that it gets rid of the friend declarations, which were a reminder of the
> underlying problem. It remains the case that you don't really get any
guarantees
> by using the synchronous methods; the data can still be modified from outside
> the process.)
> 
> That being said, this looks fine to me.
> 
>
https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
> File components/password_manager/core/browser/password_store_sync.h (right):
> 
>
https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
> components/password_manager/core/browser/password_store_sync.h:16: // classes.
> It seems worthwhile to also mention the thread restrictions that apply when
> calling these methods.

Mike, what deadlock and guarantees you are talking about?
The sqlite database is locked while the profile is open.

[vasilii, 2014-05-02 11:45:51]

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store.h:200: // These will be
run in PasswordStore's own thread.
On 2014/05/01 00:10:08, Ilya Sherman wrote:
> nit: What does "these" refer to?

Done.

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store.h:210: const
autofill::PasswordForm& form) = 0;
On 2014/05/01 00:10:08, Ilya Sherman wrote:
> Is this still necessary with protected inheritance?  If so, why?

The problem is with base::Bind. base::Bind(&PasswordStore::AddLoginImpl, this)
doesn't compile even if I use "using PasswordStoreSync::AddLoginImpl".
&PasswordStore::AddLoginImpl has type "PasswordStoreChangeList
(PasswordStoreSync::*)(const autofill::PasswordForm&)". Bind fails to cast
|this| to PasswordStoreSync*. If I cast manually then it doesn't compile because
PasswordStoreSync* isn't ref-counted.

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:15: //
synchronous methods of PasswordStore which shouldn't be accessible for other
On 2014/05/01 00:10:08, Ilya Sherman wrote:
> nit: "accessible for" -> "accessible to"

Done.

https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:16: // classes.
On 2014/04/30 17:35:51, Mike Mammarella wrote:
> It seems worthwhile to also mention the thread restrictions that apply when
> calling these methods.

Done.

[Ilya Sherman, 2014-05-02 23:13:58]

LGTM, thanks.

[Mike Mammarella, 2014-05-03 21:10:24]

On 2014/05/02 11:45:37, vasilii wrote:
> On 2014/04/30 17:35:50, Mike Mammarella wrote:
> > I'm still not convinced the deadlock potential is actually resolved, but
this
> > arrangement doesn't seem any worse than the current state, either. (Except
in
> > that it gets rid of the friend declarations, which were a reminder of the
> > underlying problem. It remains the case that you don't really get any
> guarantees
> > by using the synchronous methods; the data can still be modified from
outside
> > the process.)
> > 
> > That being said, this looks fine to me.
> > 
> >
>
https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
> > File components/password_manager/core/browser/password_store_sync.h (right):
> > 
> >
>
https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
> > components/password_manager/core/browser/password_store_sync.h:16: //
classes.
> > It seems worthwhile to also mention the thread restrictions that apply when
> > calling these methods.
> 
> Mike, what deadlock and guarantees you are talking about?
> The sqlite database is locked while the profile is open.

On Linux and OS X, the passwords are not stored in the sqlite database. On OS X,
all the metadata is, but the passwords themselves are stored in the user's
keychain. On Linux, nothing is stored in the sqlite database at all; everything
is stored in either GNOME Keyring or KWallet (depending on the desktop
environment). These services can be accessed at any time by any other process,
and access to them may block indefinitely waiting for the user's input.

[vasilii, 2014-05-05 10:59:45]

On 2014/05/03 21:10:24, Mike Mammarella wrote:
> On 2014/05/02 11:45:37, vasilii wrote:
> > On 2014/04/30 17:35:50, Mike Mammarella wrote:
> > > I'm still not convinced the deadlock potential is actually resolved, but
> this
> > > arrangement doesn't seem any worse than the current state, either. (Except
> in
> > > that it gets rid of the friend declarations, which were a reminder of the
> > > underlying problem. It remains the case that you don't really get any
> > guarantees
> > > by using the synchronous methods; the data can still be modified from
> outside
> > > the process.)
> > > 
> > > That being said, this looks fine to me.
> > > 
> > >
> >
>
https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
> > > File components/password_manager/core/browser/password_store_sync.h
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/246253014/diff/60001/components/password_mana...
> > > components/password_manager/core/browser/password_store_sync.h:16: //
> classes.
> > > It seems worthwhile to also mention the thread restrictions that apply
when
> > > calling these methods.
> > 
> > Mike, what deadlock and guarantees you are talking about?
> > The sqlite database is locked while the profile is open.
> 
> On Linux and OS X, the passwords are not stored in the sqlite database. On OS
X,
> all the metadata is, but the passwords themselves are stored in the user's
> keychain. On Linux, nothing is stored in the sqlite database at all;
everything
> is stored in either GNOME Keyring or KWallet (depending on the desktop
> environment). These services can be accessed at any time by any other process,
> and access to them may block indefinitely waiting for the user's input.

This is a known issue not related to sync. For example, Chrome can't shutdown on
OS X if user doesn't close the Keychain message box. I don't think it's a bug
BTW.

[vasilii, 2014-05-05 11:00:00]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-05-05 11:00:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/246253014/80001

[commit-bot: I haz the power, 2014-05-05 13:29:08]

Change committed as 268168

[Mike Mammarella, 2014-05-06 17:51:40]

Well, it's related to sync in that sync is the only code that calls these
formerly-private synchronous methods, so it has to work correctly even when
these methods block for such long times. But with the checks that they're
only called on the right threads I think it'll be fine, as long as they
don't hold any sync-wide locks.


On Mon, May 5, 2014 at 3:59 AM, <vasilii@chromium.org> wrote:

> On 2014/05/03 21:10:24, Mike Mammarella wrote:
>
>> On 2014/05/02 11:45:37, vasilii wrote:
>> > On 2014/04/30 17:35:50, Mike Mammarella wrote:
>> > > I'm still not convinced the deadlock potential is actually resolved,
>> but
>> this
>> > > arrangement doesn't seem any worse than the current state, either.
>> (Except
>> in
>> > > that it gets rid of the friend declarations, which were a reminder of
>> the
>> > > underlying problem. It remains the case that you don't really get any
>> > guarantees
>> > > by using the synchronous methods; the data can still be modified from
>> outside
>> > > the process.)
>> > >
>> > > That being said, this looks fine to me.
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/246253014/diff/60001/
> components/password_manager/core/browser/password_store_sync.h
>
>> > > File components/password_manager/core/browser/password_store_sync.h
>>
> (right):
>
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/246253014/diff/60001/
> components/password_manager/core/browser/password_store_sync.h#newcode16
>
>> > > components/password_manager/core/browser/password_store_sync.h:16: //
>> classes.
>> > > It seems worthwhile to also mention the thread restrictions that apply
>>
> when
>
>> > > calling these methods.
>> >
>> > Mike, what deadlock and guarantees you are talking about?
>> > The sqlite database is locked while the profile is open.
>>
>
>  On Linux and OS X, the passwords are not stored in the sqlite database.
>> On OS
>>
> X,
>
>> all the metadata is, but the passwords themselves are stored in the user's
>> keychain. On Linux, nothing is stored in the sqlite database at all;
>>
> everything
>
>> is stored in either GNOME Keyring or KWallet (depending on the desktop
>> environment). These services can be accessed at any time by any other
>> process,
>> and access to them may block indefinitely waiting for the user's input.
>>
>
> This is a known issue not related to sync. For example, Chrome can't
> shutdown on
> OS X if user doesn't close the Keychain message box. I don't think it's a
> bug
> BTW.
>
> https://codereview.chromium.org/246253014/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.