413400 - With multiple accounts, prefill the account associated with Camino (Keychain)

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

When I was testing bug 178607, I noticed that all of the sudden Camino decided it should prefill Gmail's login on pageload with the (default?) Safari account instead of the account saved by Camino.

We should initially prefer the account currently saved by Camino.

STR:
1. Have a Gmail account saved to the Keychain and one or more Gmail accounts saved to the Keychain in Safari.
2. https://mail.google.com in a fresh profile

AR:

1. Keychain prompts to access the (default?) Safari account
2. Camino fills in said Safari account

ER:

1. Camino autofills the same old account it's been using all along

Comment 1

[Chris Lawson (gone)]

Isn't this what http://mxr.mozilla.org/camino/source/camino/src/formfill/KeychainService.mm#316 was supposed to address? Either that code isn't working, the initial setDefaultWebFormKeychainEntry method isn't working, or something else is wonky. It'd probably be useful to try this in a fresh user account to see exactly what's happening here, but it shouldn't be too difficult to fix once we know where the problem lies.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

[3:41pm] cl: because at a glance, it looks like batwood's code should account for the scenario you describe
[3:42pm] sauron: yes, it should
[3:42pm] cl: so either his code isn't doing what he meant it to do, or something was weird about the way his code interacted with your keychain items
[3:42pm] sauron: mhm
[3:42pm] sauron: i looked at that not too long ago
[3:43pm] sauron: i still see the problem occasionally, so I'm not sure what's going on

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> [3:43pm] sauron: i still see the problem occasionally, so I'm not sure
> what's going on

So, what's happening these days is (roughly) that if there's a camino_default (and, I think, also having a default is required, but I'd have to do a little more controlled testing to see), camino_default gets cleared on login.

Then, when I log in next and the default account is autofilled, and I choose the other (former camino_default) account, we store camino_default on that account again.

The next time I go to log in, we prefill the camino_default account…and clear camino_default from it.

I'd have to do some instrumentation to figure out where things are going bad exactly, but the comment for setDefaultWebFormKeychainEntry: says

"It first clears any existing default Camino entry before setting the passed in keychain to the new default."

So my guess is that something isn't working correctly right there.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

So, I tried to set up a clean demo here: http://demo.ardisson.org/index.php

This time, what I'm seeing is that most of the time we do the right thing, but randomly we'll switch from user1 to user2--but I'm not seeing the "camino_default" being deleted, ever.  ?:|

Oh, it's being deleted; it's just for some reason, for this site, I have to quit and restart Keychain Access to see the changes, whereas before I could see the deletion "live" ?:|

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Logtacular debugging patch

setDefaultWebFormKeychainEntry: *appears* to be doing what it says and *appears* to be successfully setting the right comment on the right item.

However, as Wevah pointed out, there's no Obj-C API for the Keychain or KeychainItem, so KeychainItem.m is our Obj-C wrapper.

KeychainItem setComment: should be succeeding, because there's an NSLog there for failure cases, which I'm not seeing (though I haven't yet logged it explicitly to see that it's being called, but from the logging I am doing, you can see an item gaining and losing its comment).

[12:03am] sauron: i wonder if we're just setting the comment on our object
[12:04am] sauron: and it's not propagated into the real item
[12:07am] Wevah: could be
[12:08am] sauron: because we're using the wrappers
[12:10am] sauron: although, after a complete cycle, we seem to get it right
[12:10am] Wevah: probably not syncing right away
[12:11am] sauron: but even if i wait, it doesn't seem to do the right thing… :(
[12:12am] sauron: hrm, there it worked

In particular, setDefaultWebFormKeychainEntry: is acting (separately) on two KeychainItems: the one we want to be the default, which we pass in (keychainItem), and the current default (keychainTemp), which is accessed via enumeration of an array of KeychainItems matching host/port/protocol/authType.

So, if there are sync-time issues, there's a problem doing all of these operations right after each other.  Or, if these are two different objects, rather than, say, two aliases to the same object, there are problems.  To wit:

> 1a kdbg - camino_default is <KeychainItem: 0xb557d0> (demo.ardisson.org (user1))
> 2 kdbg - setDefaultWebFormKeychainEntry: <KeychainItem: 0xb557d0> (demo.ardisson.org (user1)) with comment “camino_default”
> 3 kdbg - Clearing comment on old camino_default <KeychainItem: 0xbcced0> (demo.ardisson.org (user1))
> 4 kdbg - Former camino_default <KeychainItem: 0xbcced0> (demo.ardisson.org (user1)) now has comment “”
> 5 kdbg - After clearing the old default, to-be-default keychainItem <KeychainItem: 0xb557d0> (demo.ardisson.org (user1)) currently has comment “camino_default”
> 7 kdbg - Leaving setDefaultWebFormKeychainEntry: new default keychainItem <KeychainItem: 0xb557d0> (demo.ardisson.org (user1)) has comment  “camino_default”
> 1b kdbg - default is <KeychainItem: 0x171bc210> (demo.ardisson.org (user2))

The above logging is from a situation where "camino_default" is currently set on the desired account (user1) at prefill (line 1).

At line 2, we call setDefaultWebFormKeychainEntry: on that item (keychainItem), which we note currently has "camino_default".

In line 3, we're clearing the comment on the current camino_default item, which is an enumerated keychainTemp.

On line 4, you can see that clearing (aka setComment:@"") was successful on the keychainTemp item.

On line 5, just before we check to see if the desired camino_default entry, the keychainItem we were passed in, has an empty comment (so we don't stomp Safari's "default"), you'll notice that keychainItem *still* has a comment of "camino_default".  Uh-oh.

Line 6 is missing; notice that there's never any logging of "Setting comment on new default", because, since keychainItem has a non-empty comment going in, we never change that comment.

Line 7 is kind of a red-herring in this case, because it just tells us at the end of the method that nothing has changed since line 5.  (In the case where we swap from "user2" to "user1", line 5 has no comment and line 7 has the comment "camino_default", after line 6 gets logged.)

So, it's not clear to me whether this is a delayed-sync issue, or whether we're just dealing with two different representations of the same underlying key, each of which is a separate object we act upon.  But whichever it is, that's the problem, and it effectively leads to us clearing our deleting our current default when we actually want to (and think we are) preserving it.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Depending on what the underlying problem really is, there are several possible ways to fix this bug:

1) If the item we pass into setDefaultWebFormKeychainEntry: currently has the comment "camino_default" || "default", return.  Although I can't be sure, it seems like there's no reason to go through this whole rigamarole to set as default something that's currently the default (unless something could change this out from under us in the time between the previous method and starting setDefaultWebFormKeychainEntry:).

2) If the item we pass into setDefaultWebFormKeychainEntry: currently has the comment "camino_default", clear it at the beginning of the method.  This will ensure that it can be set successfully at the end of the method.  (I don't know what this will do to the enumeration loop, but presumably it's already safe for cases where there is no default.)

3) If the item from the enumeration that has the comment "camino_default" == the item we pass in to setDefaultWebFormKeychainEntry:, don't delete the comment on the enumerated item.  While we'll still fail to set the comment at the end of the method, in theory we shouldn't need to since we're not deleting it.  I'm not sure if we can compare two KeychainItems like that, though.

To me, solution 1 seems both the easiest and sanest, but I don't know if it adequately addresses the underlying bug.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #6)

> 1) If the item we pass into setDefaultWebFormKeychainEntry: currently has
> the comment "camino_default" || "default", return.

If we use this solution, it should only check for "camino_default", because otherwise we prevent consolidating the default accounts across browsers.  (Other than that, it seems to work nicely in my tests.)

Stuart, can you look at comment 5 and 6 and give me some pointers on how to proceed (either how to determine which possible underlying cause is really the cause, or, if we don't need to care, which proposed solution or yet-to-be-proposed solution sounds best)?

I didn't see anything in our wrapper code (nor in the description for SecKeychainItemModifyAttributesAndData) that jumped out at me as "possible sync issue", but that might just be me.

Comment 8

[Stuart Morgan]

I don't have a good explanation; it does sound a lot like keychain doesn't guarantee that multiple actions on different references in the same thread are actually ordered. That's pretty sucky, but I guess it doesn't come up much :P

1 seems like the cleanest solution, and it seems very likely to fix the bug.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Don't do anything if the entry is already Camino's default

This is the "option 1" solution; it's mostly updating the method comment, but there are actually two lines of code that do the hard work of fixing this bug ;-)

I'd like some additional eyes on it to make sure there's not some edge case I've overlooked that depends on the current buggy behavior.

Comment 10

[philippe (part-time)]

Is this the expected behaviour with attachment 599498 [details] [diff] [review]?

Given Gmail account A (camino_default) and Gmail account B (default)
[1] Load Gmail --> account A is pre-filled. OK. Log in, log out.
    no relevant changes to the Keychain entry for account A
[2] Load Gmail and access account B. Log out.
    Keychain entry for account B is not modified (still 'default')
    Keychain entry for account A is modified – the 'camino_default' is removed.

Note that the behaviour in [2] mostly matches Safari on 10.7.3 (the difference is that Safari will blow away a 'camino_default' if it exists).

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to philippe from comment #10)
> Is this the expected behaviour with attachment 599498 [details] [diff] [review]
> [review]?
> 
> Given Gmail account A (camino_default) and Gmail account B (default)

Yes, that's the expected behavior.

> [2] Load Gmail and access account B. Log out.
>     Keychain entry for account B is not modified (still 'default')
>     Keychain entry for account A is modified – the 'camino_default' is
> removed.

[2] is the current behavior for that scenario, and it didn't (and shouldn't have) change(d) with the patch.

> [1] Load Gmail --> account A is pre-filled. OK. Log in, log out.
>     no relevant changes to the Keychain entry for account A

[1] was the scenario that had the "amnesia" bug before; with the behavior as described here, the patch works as expected.

> Note that the behaviour in [2] mostly matches Safari on 10.7.3 (the
> difference is that Safari will blow away a 'camino_default' if it exists).

According to the code comments for setDefaultWebFormKeychainEntry:, Safari deletes/updates the comment field indiscriminately :-(

Comment 12

[philippe (part-time)]

Comment on attachment 599498 [details] [diff] [review]
Don't do anything if the entry is already Camino's default

(In reply to philippe from comment #10)
> Is this the expected behaviour with attachment 599498 [details] [diff] [review]
> [review]?
> 
> [...]

captured from IRC:
sauron|away: Safari always deletes whatever it finds
sauron|away: at least according to the code comments
sauron|away: and yes, [2] is the expected behavior there; it allows a user to consolidate their preferences if they want
sauron|away: it's not a great behavior if you want to use multiple accounts just in Camino, but that's the way the current code works
sauron|away: every time you log in with a new account, we change the default to that account

In that case, f+ as the behaviour is otherwise a little bit more sane (the camino_default is not lost when the user only / consistently uses one account with Camino).

Comment 13

[Chris Lawson (gone)]

Comment on attachment 599498 [details] [diff] [review]
Don't do anything if the entry is already Camino's default

>diff --git a/src/formfill/KeychainService.mm b/src/formfill/KeychainService.mm
>--- a/src/formfill/KeychainService.mm
>+++ b/src/formfill/KeychainService.mm
>@@ -255,22 +255,28 @@ static KeychainService *sInstance = nil;
> //  by setting 'camino_default' in the comment field. Camino first checks for
> //  'camino_default', then looks for 'default' if there isn't one.  Safari will

Ugh. I know this isn't a problem with your comment, but this whole block has double spaces after periods. That drives me crazy :-p (Since it's just comments, can we clean up the spacing in conjunction with fixing this bug?)

>+// setting the passed-in keychain entry to the new default.  It doesn't modify
>+// any other comment values, meaning that the Safari default will preserved.

While you're editing, change this to "default will be preserved".

Looks good to me. r=cl.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 599498 [details] [diff] [review]
Don't do anything if the entry is already Camino's default

(In reply to Chris Lawson from comment #13)

> >diff --git a/src/formfill/KeychainService.mm b/src/formfill/KeychainService.mm
> >--- a/src/formfill/KeychainService.mm
> >+++ b/src/formfill/KeychainService.mm
> >@@ -255,22 +255,28 @@ static KeychainService *sInstance = nil;
> > //  by setting 'camino_default' in the comment field. Camino first checks for
> > //  'camino_default', then looks for 'default' if there isn't one.  Safari will
> 
> Ugh. I know this isn't a problem with your comment, but this whole block has
> double spaces after periods. That drives me crazy :-p (Since it's just
> comments, can we clean up the spacing in conjunction with fixing this bug?)

Since I'm touching the bottom paragraph of the comment, I guess it's OK.  There are other places in the tree that do this, too, so you probably want to file yourself a giant bug on this peeve ;-(

Comment 15

[Stuart Morgan]

Comment on attachment 599498 [details] [diff] [review]
Don't do anything if the entry is already Camino's default

sr=smorgan

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/61dbdd259d98 with cl's comment comments addressed.

/me does a dance of joy!