322845 - Form Fill doesn't notice change in which card is "Me" card, or edits to "Me" card, while Camino is running

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: OS Integration, defect)

Description

[Elayne Leidecker]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060108 Camino/1.0b2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060108 Camino/1.0b2+

Even though I was able to resolve my Forms Fill problem, I thought it might merit some attention.  

Last night, I attempted to complete a form using Fill Form button.  Button was totally unresponsive.  I tried again using keyboard shortcut, but form fields still did not populate.  So I tried Edit, Fill Form, but still no response.  This was In a recent nightly build of Camino (within the last week), so I downloaded latest nightly build 2006010804 (v1.0b2+), but still had same results.   

Troubleshooting Notes:  

1.  I did not receive message re: allowing keychain access (after the download and upon attempting to fill out a form).  I thought it might be a keychain access problem, so I checked to make sure data was intact.  It was. 
2.  I vaguely remembered that Camino uses the Apple Mail “Me” card for forms information, so I decided to check it out.  For some reason, the “Me” card was no longer designated as such (Why, I don’t have a clue other than that I had recently edited the card).  I designated the card as “Me” and tried to reproduce this by editing the card again, but this time it kept the “Me” designation.   
3.  After re-designating the card as “Me”, I tried again to fill a form in Camino.  Still did not work.  
4.  I quit Camino, re-started Camino, and attempted to fill a form.  This time, the form fields populated. 

My troubleshooting notes lead me to conclude that the items listed above could be the reason for some of the Fill Form “bug” reports.   Camino’s dependency on the Apple Address Book’s “Me” card for forms information can create what appears to be a bug but is, in fact, such a simple fix (if you know what to check) that is likely to be overlooked.   Evidently, Camino does not cache the "Me" card info in the event that the "Me" card changes? 

Therefore, why does Camino depend on the Apple address book for forms information?  (I really don’t use or like Apple Mail and much prefer Microsoft Office’s Entourage.)  If one doesn’t use Apple Mail as their email application, Camino should clearly state in BIG BOLD text that the “Me” card in the Apple Mail address book still must be filled out and so designated to avoid any problems such as stated above. 


Reproducible: Couldn't Reproduce

Actual Results:  
Couldn't reproduce because it currently will not allow me to undo "Make this my card".  Will keep trying.


Also, latest downloads don't seem to have "Talk Back" feature or "Allow Keychain Access" message after a Camino new download.

Comment 1

[Mike Pinkerton (not reading bugmail)]

re-summarizing. 

Comment 2

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

Yeah, I can repro this on 10.3.9.

It looks like we're caching which card is set as the "Me" card.

Any chance there are more deprecated properties (a la bug 313884, but for 10.3 as well) lurking around here?

Comment 3

[Stuart Morgan]

This would be really easy to fix, but because we just use wallet directly and wallet's a big opaque blob, it's not.  I think the wallet code will need to be substantially rewritten first.

Comment 4

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

Mass-reassign of bugs still assigned to pinkerton to nobody; filter on "NoMoPinkBugsInCamino".

Comment 5

[Chris Lawson (gone)]

(In reply to Stuart Morgan from comment #3)
> This would be really easy to fix, but because we just use wallet directly
> and wallet's a big opaque blob, it's not.  I think the wallet code will need
> to be substantially rewritten first.

Just at a glance, it looks like we need a way to tell wallet to re-initialize itself (or just re-import the AB data) whenever a kABDatabaseChangedExternallyNotification is posted. That doesn't *sound* too difficult, but I didn't look that closely, either.

Comment 6

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

Attachment: Fix as outlined by cl; does not compile!

(In reply to Chris Lawson from comment #5)
> Just at a glance, it looks like we need a way to tell wallet to
> re-initialize itself (or just re-import the AB data) whenever a
> kABDatabaseChangedExternallyNotification is posted. That doesn't *sound* too
> difficult, but I didn't look that closely, either.

Such a fix would look something like this.

There are two main problems with it, currently:

1) It does not at all compile.  The compiler does not like Obj-C method definitions in this supposedly-Obj-C++ file:

/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/formfill/wallet.h:75: error: expected unqualified-id before ‘-’ token
/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/formfill/wallet.mm: In function ‘void wallet_InitListFromAppleAddressBook(nsVoidArray**)’:
/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/formfill/wallet.mm:1466: error: ‘self’ was not declared in this scope
/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/formfill/wallet.mm: At global scope:
/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/formfill/wallet.mm:1508: error: expected unqualified-id before ‘-’ token
** BUILD FAILED **

2) wallet doesn't have a dealloc or any obvious cleanup function (have I ever mentioned that wallet is an awesome pile of code?), so there's no obvious place to remove the observer.

wallet currently works like this:
1) We call Wallet_Prefill every time we do Fill Form
2) Wallet_Prefill calls wallet_TraversalForPrefill, which calls wallet_Initialize
3) wallet_Initialize checks some static variables to see if it's done its setup yet; the very first time through, said variables are false, so wallet_Initialize sets up all the lists/values/mappings and twiddles the static variables to say it's done said setup.
4) Subsequent Form Fill attempts go through steps 1-2; step 3 is essentially a no-op because those variables all are true.

The patch (would) works like this:

1) The first time wallet_InitListFromAppleAddressBook is called, sets up a notification observer for the AB notifications, and sets a static variable to say we've set up the notifications.
2) When a notification comes in, the poorly-named updateListFromAddressBook: twiddles the static variable for "has wallet set up its user data" back to false.
3) The next time we do Form Fill, when things wind their way back into wallet_Initialize we update the values from the address book.

(Also, since we only care about the "Me" card changing, or changes to the "Me" card's values, any chance there's a way to scope the notifications to only those things, instead of every change to anything in the Address Book!?)

Comment 7

[Stuart Morgan]

You're trying to declare an ObjC member method outside of an ObjC class declaration, which you can't do, and trying to refer to a C++ object as "self" as if it were an ObjC object.

Objective-C++ lets you mix C++ and Objective-C in the same file, but C++ classes and Objective-C classes are still totally different things. You can't make a class that's both a C++ class and an Objective-C class at the same time.

Comment 8

[Chris Lawson (gone)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #6)

> wallet currently works like this:
> 1) We call Wallet_Prefill every time we do Fill Form
> 2) Wallet_Prefill calls wallet_TraversalForPrefill, which calls
> wallet_Initialize
> 3) wallet_Initialize checks some static variables to see if it's done its
> setup yet; the very first time through, said variables are false, so
> wallet_Initialize sets up all the lists/values/mappings and twiddles the
> static variables to say it's done said setup.
> 4) Subsequent Form Fill attempts go through steps 1-2; step 3 is essentially
> a no-op because those variables all are true.

This is what I was wondering about and didn't dig into thoroughly. It suggests an alternative solution, one that isn't exactly "ideal" but is definitely far easier than rewriting wallet:

Remove the line of code (in this case, http://mxr.mozilla.org/camino/source/camino/src/formfill/wallet.mm#1596 ) that tells wallet_Initialize that its Address Book setup is done.

Since we're really only re-reading one card and we care *very much* about changes to that card, in the absence of a proper NSNotification-based solution, re-reading that card every time seems like it's worth the minor time penalty that would be incurred.

> (Also, since we only care about the "Me" card changing, or changes to the
> "Me" card's values, any chance there's a way to scope the notifications to
> only those things, instead of every change to anything in the Address Book!?)

Not from what I read at http://developer.apple.com/library/mac/#documentation/UserExperience/Conceptual/AddressBook/Tasks/ManagingGroups.html#//apple_ref/doc/uid/20001022-SW1 , unfortunately. Seems like that'd be useful, though. File a rdar?

cl

Comment 9

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

Attachment: Fix option 2 as outlined by cl; ugly but compiles!

> This is what I was wondering about and didn't dig into thoroughly. It
> suggests an alternative solution, one that isn't exactly "ideal" but is
> definitely far easier than rewriting wallet:
> 
> Remove the line of code (in this case,
> http://mxr.mozilla.org/camino/source/camino/src/formfill/wallet.mm#1596 )
> that tells wallet_Initialize that its Address Book setup is done.

Fortunately, that does indeed compile (and work).  wallet_ValuesReadIn is only checked in one place (where |if (!wallet_ValuesReadIn)|, we then read from Address Book), and only twiddled two places, right after calling our Address Book method, and in WLLT_ClearUserData, which is inside one of pink's giant #if UNUSED blocks.
 
> Since we're really only re-reading one card and we care *very much* about
> changes to that card, in the absence of a proper NSNotification-based
> solution, re-reading that card every time seems like it's worth the minor
> time penalty that would be incurred.

I agree.  Stuart, what say you?

I'd prefer to get the proper solution working, of course, so if you can give me some tips on comment 6, I'm happy to pursue that route (short of rewriting wallet!), but barring that, the approach in this patch seems like a sane-enough solution (given that, after all, it's wallet!) :P

Comment 10

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

(In reply to comment #9)
> I'd prefer to get the proper solution working, of course, so if you can give
> me some tips on comment 6, I'm happy to pursue that route (short of
> rewriting wallet!), but barring that, the approach in this patch seems like
> a sane-enough solution (given that, after all, it's wallet!) :P

Er, I totally missed comment 7 somehow (twice!) :( ; based on that, it sounds like the NSNotification solution is "impossible" without rewriting wallet, then?

Comment 11

[Stuart Morgan]

The way to deal with a notification would be to create a little "bridge" object that is an ObjC class that takes a non-retained pointer to the wallet class in init, sets up a notification listener, and calls a method on its walle instance when the notification comes in. Then the wallet class would get a new member that is a pointer to that class (forward-declared with @class in the header, and then declared and defined in the implementation file), and it would create an instance in its constructor (passing 'this' as the parameter), and destroy it in its destructor.

Comment 12

[Chris Lawson (gone)]

Sorry for being so dense...

(In reply to Stuart Morgan from comment #11)
> The way to deal with a notification would be to create a little "bridge"
> object that is an ObjC class that takes a non-retained pointer to the wallet
> class in init

Which wallet class? AFAICT, there isn't a specific "wallet" class that does all the managing of data; there are just a bunch of related classes that talk to each other. I can't tell which one is the actual "wallet" object (and it doesn't help that we apparently don't directly use *any* of the wallet classes anywhere in our code at the moment -- the only place we even call into wallet code from Obj-C classes is http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWindowController.mm#5387 -- so I have nothing to reference for help).

It *looks* like our wallet might have, at one time, been an implementation of nsIWalletService, but the nsIWalletService.h line in wallet.h is specifically commented out (which is itself sort of weird).

> sets up a notification listener, and calls a method on its
> wallet instance when the notification comes in. Then the wallet class would

I assume this means the same wallet class as before?

> get a new member that is a pointer to that class (forward-declared with
> @class in the header, and then declared and defined in the implementation

"The header" being wallet.h, correct?

Comment 13

[Stuart Morgan]

Oh God. I'd never actually looked at the totality of the wallet code before. Wow.

So, I was just assuming there was a class (rather than a steaming pile of globals and a bunch of static functions using them). The stark reality version of comment 11 would be to declare a little listener class that registers for the appropriate notification in its init, declare a global variable that's a pointer to one, create and populate that global in the main wallet initialization. Then when the notification fires, have it call whatever wallet function(s) are necessary to refresh the address book.

(I didn't actually look at the details of how to re-initialize just that section of wallet; it's a pretty big mess, so you'd have to be pretty careful to make sure you weren't re-running things that would cause problems.)

Comment 14

[Chris Lawson (gone)]

Attachment: "proper" fix v1.0

Something like this, then?

As I noted in a code comment, the AddressBook framework actually sends out that notification *twice* every time the "Me" designation moves from one card to another, plus once each time any data on any card is changed. That makes us do a bunch of extra work, but I guess we're assuming (probably correctly) that AB data is changing much less often than users are triggering Fill Form.

Comment 15

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

Comment on attachment 599484 [details] [diff] [review]
"proper" fix v1.0

I reviewed this for cl on irc the other night; I'm just going to go ahead and paste my comments and clear it from my queue while we wait for Stuart to answer the bigger questions ;)

[01:38am] sauron: cl|zzz: aren't you leaking the WalletABNotificationListener ?
[01:39am] cl|zzz: maybe, but I have no clue how to dealloc it, seeing as there's nothing that ever cleans up wallet except shutdown :P
[01:40am] cl|zzz: I wondered about that
[01:40am] cl|zzz: figured smorgan might be able to shed some light on it
[01:40am] sauron: and also leaving dangling notifications
[01:41am] cl|zzz: ?
[01:41am] cl|zzz: oh, i never unregister for them
[01:41am] sauron: never unregistering
[01:41am] cl|zzz: yeah, oops
[01:41am] cl|zzz: forgot that part :P
[01:41am] sauron: which is often done in the clean-up
[01:42am] sauron: where we'd not-leak the WalletABNotificationListener ;)
[01:42am] cl|zzz: right, i could do it in the class's dealloc, which would make sense, but i don't think the dealloc would ever get called
[01:43am] cl|zzz: unless...is there actually something in wallet that *does* get called to clear everything out?
[01:43am] cl|zzz: and if so, do we ever hit it?
[01:43am] sauron: maybe, and no
[01:43am] cl|zzz: are you sure we never hit it?
[01:43am] sauron: my logs never ran
[01:44am] sauron: Wallet_ReleaseAllLists
[01:45am] cl|zzz: oh, you logged in there and still never saw it?
[01:45am] sauron: that clears the schema stuff
[01:45am] sauron: yeah, iirc
[01:45am] cl|zzz: even on quit?
[01:45am] sauron: mhm
[01:45am] sauron: we may in fact be leaking all of wallet
[01:45am] sauron: :P                     
[01:45am] cl|zzz: lol
[01:45am] sauron: pink-- :P
[01:46am] cl|zzz: in that case, what's one more variable? :P
[01:46am] sauron: well, my concern is we already have notification-related crashes
[01:46am] cl|zzz: what i should probably do is autorelease that
[01:46am] sauron: i'd like not to add the possibility for more
[01:47am] cl|zzz: and then actually put the removeObserver thingy in the dealloc like i meant to
[01:47am] sauron: you going to spin a new patch then?
[01:47am] cl|zzz: we do declare (alloc/init) a PhoneNumber* inside our AB reader function
[01:48am] cl|zzz: and it's autoreleased, so that's probably the way to go
[01:48am] sauron: y
[01:48am] sauron:        http://mxr.mozilla.org/camino/source/camino/src/formfill/wallet.mm#1467    
[01:49am] sauron: anyway, if you're going to spin a new patch, in the header comment, stick to "Me" and not swap between "Me" and "me" :P
[01:51am] cl|zzz: I do that for a reason
[01:51am] sauron:        http://hg.mozilla.org/camino/filelog/d373e3905731/src/formfill/PhoneNumber.mm     
[01:51am] cl|zzz: the "Me" card is a proper name, if you will
[01:51am] sauron: mhm…
[01:51am] cl|zzz: the "card designated as 'me'" is not
[01:53am] sauron: cl|zzz: also: http://mxr.mozilla.org/camino/source/camino/src/bookmarks/AddressBookManager.m#54
[01:54am] sauron: er, back ti "Me"
[01:54am] cl|zzz: doesn't that get called on startup?
[01:55am] sauron: I think that's confusing, and "card designated as the "Me" card" would be a lot more clear
[01:55am] sauron: what?
[01:55am] cl|zzz: fair enough
[01:55am] cl|zzz: the AddressBookManager
[01:55am] cl|zzz: or do we not hit that code until someone actually opens the BM Manager and clicks on that particular smart group?
[01:56am] sauron: dunno
[01:56am] sauron: but either way, it seems dangerous to rely on the ordering remaining the same in disconnected code
[01:58am] cl|zzz: well, i just figured if it gets called on startup, there's no way Form Fill is ever going to try to work before it's loaded
[02:01am] sauron: i don't know the order of things, but the notification stuff is possibly going to be set up before we even set up the Address Book data for wallet
[02:02am] cl|zzz: right, so wouldn't it not ever be a problem?
[02:03am] sauron: if that happens, we wouldn't have valid constants and would presumably never get notifications
[02:03am] cl|zzz: god, what a mess
[02:03am] sauron: welcome to my world ;)

So here's the I-hope-expected r-; hopefully Stuart can shed some more light on the issues.

Comment 16

[Chris Lawson (gone)]

I have all the issues mentioned in comment 15 fixed locally, but I don't see much point in trying to update this patch when there's a bunch of other stuff pending that's going to touch code very near this. Once bug 315469 lands, I should be able to get this re-spun in a jiffy.