Linux: access GNOME Keyring on the GLib main thread, rather than the DB thread.

Linux: access GNOME Keyring on the GLib main thread, rather than the DB thread.
I was unable to reproduce the crash, but it's happening to a lot of people and the web suggests that moving access to GNOME Keyring to the GLib main thread should fix the problem.
BUG=48343, 25404
TEST=hopefully it won't crash for some people any more

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52729

[Mike Mammarella, 2010-07-13 18:24:53]

ping

[Evan Martin, 2010-07-13 20:33:27]

Sorry to put off the review, I wanted to ponder it over a bit.

The change is awesome and I'm impressed you managed to hack it up so quickly. 
But I'm worried about the complexity explosion.  I believe PasswordStore itself
was only introduced when we added the Mac port (before it was just encryptor,
maybe?) and it seems to me like it's working at the wrong layer.

That is, say there was a PasswordStoreOnUIThread or something that on Mac/Win
understood all this DB thread fiddling.  Then it would have a trivial
implementation here, because we must do it on the UI thread.

What do you think?

[Mike Mammarella, 2010-07-13 20:47:35]

On 2010/07/13 20:33:27, Evan Martin wrote:
> Sorry to put off the review, I wanted to ponder it over a bit.
> 
> The change is awesome and I'm impressed you managed to hack it up so quickly. 
> But I'm worried about the complexity explosion.  I believe PasswordStore
itself
> was only introduced when we added the Mac port (before it was just encryptor,
> maybe?) and it seems to me like it's working at the wrong layer.
> 
> That is, say there was a PasswordStoreOnUIThread or something that on Mac/Win
> understood all this DB thread fiddling.  Then it would have a trivial
> implementation here, because we must do it on the UI thread.
> 
> What do you think?

It does seem like we're fighting with PasswordStore a bit here; it gets us onto
the DB thread only to have us jump right off again. Another approach that might
be reasonable would be to have the PasswordStore constructor take a ChromeThread
constant for the thread to get us on, so we can pass it ChromeThread::DB in
Windows and ChromeThread::UI here. But we'd still have to avoid blocking in the
UI thread, so it's not clear exactly how much simpler it would really get.

I wrote the comment about how this situation is unfortunate a little before I
had actually finished the change: in particular, before I had realized that we
could parallelize the previously serial calls to GNOME Keyring that we do to
tiptoe around the broken secure memory allocator. This is actually kind of neat
and basically uses the DB thread as a patsy that can block, allowing us to avoid
inverting the flow of control to build a state machine to do the same thing on
the UI thread.

So, I sort of like this change more than that comment makes it sound.

[Evan Martin, 2010-07-13 21:26:38]

http://codereview.chromium.org/2953006/diff/1/2
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

http://codereview.chromium.org/2953006/diff/1/2#newcode783
chrome/browser/password_manager/native_backend_gnome_x.cc:783: if (all_forms[i])
{
Should you consider the value of success here?  (e.g. if one fails, then later
ones succeed...)

http://codereview.chromium.org/2953006/diff/1/2#newcode803
chrome/browser/password_manager/native_backend_gnome_x.cc:803:
forms->push_back(all_forms[i]);
Does the caller know to clean up |forms| when success=false on return?

[Evan Martin, 2010-07-13 21:27:10]

Oops, and the major comment:
what happens if this is going on during shutdown?  e.g. the UI thread goes away,
or the DB thread goes away (which one happens first? who gets null pointers/)

[Mike Mammarella, 2010-07-13 21:55:39]

I believe the UI thread is the main thread, so when it exits, the entire process
gets shut down (because __glibc_start_main calls exit(main(argc, argv)), more or
less). So, if the DB thread is blocked waiting for the UI thread to reply during
shutdown, that's OK. (That can happen because the message posted but was never
handled, or theoretically because the message failed to post, but I don't see
how the latter case can happen since the UI thread should always exist.) Also, I
believe waiting for the UI thread is safe from a deadlock point of view since
the UI thread should never in turn be waiting on us (or anyone else) so we won't
get a circular wait.

http://codereview.chromium.org/2953006/diff/1/2
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

http://codereview.chromium.org/2953006/diff/1/2#newcode783
chrome/browser/password_manager/native_backend_gnome_x.cc:783: if (all_forms[i])
{
On 2010/07/13 21:26:38, Evan Martin wrote:
> Should you consider the value of success here?  (e.g. if one fails, then later
> ones succeed...)

We could stop posting new GetItemInfo methods after a failure, yes. I didn't do
this because it doesn't change whether we have to enter the loop below, since we
might have already posted a GetItemInfo method earlier in this loop, so the code
would just be more complicated. But if you like I can make it work that way.

http://codereview.chromium.org/2953006/diff/1/2#newcode803
chrome/browser/password_manager/native_backend_gnome_x.cc:803:
forms->push_back(all_forms[i]);
On 2010/07/13 21:26:38, Evan Martin wrote:
> Does the caller know to clean up |forms| when success=false on return?

Nope! Good catch.

[Evan Martin, 2010-07-14 21:19:06]

LGTM

http://codereview.chromium.org/2953006/diff/15001/16001
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

http://codereview.chromium.org/2953006/diff/15001/16001#newcode807
chrome/browser/password_manager/native_backend_gnome_x.cc:807: for (size_t i =
0; i < item_ids.size(); ++i)
nit: curlies on the for

[Mike Mammarella, 2010-07-14 22:02:41]

http://codereview.chromium.org/2953006/diff/15001/16001
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

http://codereview.chromium.org/2953006/diff/15001/16001#newcode807
chrome/browser/password_manager/native_backend_gnome_x.cc:807: for (size_t i =
0; i < item_ids.size(); ++i)
On 2010/07/14 21:19:06, Evan Martin wrote:
> nit: curlies on the for

Done.