124208 - Address Book window shows LDAP directories from previous Profile in turbo mode

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[yulian chang]

2002020703 Wins build on NT.

WIth new Profile to launch Address Book window, all the LDAP directories from
previous profile displayed in the Address Book Pane. Checking with
Preferences|Mail&Newsgroup|Addressing and Mail&Newsgroups Account
Settings|Addressing, there is no LDAP directory created yet for the new Profile.

Steps to reproduce:

1. Launch N6 & Address Book window with existing Profile
2. All the LDAP directories appear in the Address Book window
3. Exit N6
4. Launch Profile manager, create a new Profile and launch N6 with it
5. Launch Address Book window

Actual result: LDAP directories from previous Profile appear in the Address Book
  Window.

Expected result: No directory displayed in Address Book Window

Comment 1

[(not reading, please use [e-mail] instead)]

are you using turbo?

Comment 2

[yulian chang]

Yes, if not using turbo, the problem is gone!

Comment 3

[yulian chang]

20020401 trunk builds
This problem still exists in trubo mode.

Comment 4

[Peter Trudelle]

nominating for MachV, since QuickLaunch is on by default. cc law

Comment 5

[scottputterman]

reassigning to Cavin.

Comment 6

[Conrad Carlen (not reading bugmail)]

The addressbook service, as far as I know, is not observing profile changes. If
it's not, of course this will happen and it's probably fairly easy to fix. Also,
then it's my fault :-/ I could use suggestions with how to hook up the observer:
(1) What is the object that holds the addressbook data from the user profile?
(2) When it's time to internalize the data from the profile, is it done lazily
or at some defined moment?

Comment 7

[(not reading, please use [e-mail] instead)]

taking.

I'll work with ccarlen on fixing how the addressbook works.

he's right, it's not observing profile changes at all.

Comment 8

[(not reading, please use [e-mail] instead)]

starting on this now.

Comment 9

[(not reading, please use [e-mail] instead)]

ok, the first problem is that nsDirPrefs caches the list of directories.

it needs to observe "profile-do-change", and reset state.

working on this now...

Comment 10

[(not reading, please use [e-mail] instead)]

I've got an initial patch started that calls DIR_Shutdown() when the profiles 
change, and so when we relaunch, we re-get the prefs.

working on the asserts and kinks now.  I should have a patch for review by 
tomorrow.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: patch making addrbook friendly to profile switches / turbo

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: patch, v2. (tab -> spaces, more code cleanup)

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: same as before, with more comments.

Comment 14

[(not reading, please use [e-mail] instead)]

Attachment: latest patch.

Comment 15

[Conrad Carlen (not reading bugmail)]

Comment on attachment 79647 [details] [diff] [review]
same as before, with more comments.

> 
>-nsAbDirectoryDataSource::~nsAbDirectoryDataSource (void)
>+nsAbDirectoryDataSource::~nsAbDirectoryDataSource()
> {
>+  // release ourselves from the observer service
>+  nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1");
>+  if (observerService) {
>+    observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
>+    observerService->RemoveObserver(this, "profile-do-change");
>+  }
>+}

Since you're going to the effort to support weak refs and using that on the
call to AddObserver, what need is there to remove yourself as an observer in
the destructor?

The rest looks good - not that I'm that familiar with the code.

> 
>+
>+  rv = observerService->AddObserver(this, "profile-do-change", PR_TRUE);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  rv = observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_TRUE);
>+  NS_ENSURE_SUCCESS(rv,rv);
> 
>-	mInitialized = PR_TRUE;
> 	return NS_OK;
> }
> 
>-NS_IMPL_ISUPPORTS_INHERITED1(nsAbDirectoryDataSource, nsAbRDFDataSource, nsIAbListener)
>+NS_IMPL_ISUPPORTS_INHERITED3(nsAbDirectoryDataSource, nsAbRDFDataSource, nsIAbListener, nsIObserver, nsISupportsWeakReference)
>

Comment 16

[David :Bienvenu]

Comment on attachment 79651 [details] [diff] [review]
latest patch.

r/sr = bienvenu - there are some changes that aren't needed to fix this bug in
the patch, and Seth is going  to make a more minimal patch for the branch.

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: addrbook only patch (the rest is trunk only cleanup)

Comment 18

[(not reading, please use [e-mail] instead)]

>Since you're going to the effort to support weak refs and using that on the
>call to AddObserver, what need is there to remove yourself as an observer in
>the destructor?

nsObserverList::AddObserver() asserts if my observer does not support weak ref.

Is it our convention to use weak ref observers, and just add them, and not 
remove them?

Comment 19

[Conrad Carlen (not reading bugmail)]

> nsObserverList::AddObserver() asserts if my observer does not support weak ref.

Only if you pass TRUE as the last param to addObserver, right?

Is it our convention to use weak ref observers, and just add them, and not 
remove them?

Doing that cuts down on calls to getService() from destructors at shutdown time
(which will fail).

Comment 20

[(not reading, please use [e-mail] instead)]

Attachment: patch, don't call RemoveObserver() in the dtor, as per ccarlen's suggestion. add a comment above the call to AddObserver(), fix some tabs / whitespace.

Comment 21

[(not reading, please use [e-mail] instead)]

ccarlen, thanks for the info.  I've fixed the patch.

Comment 22

[David :Bienvenu]

Comment on attachment 79670 [details] [diff] [review]
patch, don't call RemoveObserver() in the dtor, as per ccarlen's suggestion.  add a comment above the call to AddObserver(), fix some tabs / whitespace.

r=bienvenu

Comment 23

[Scott MacGregor]

Comment on attachment 79670 [details] [diff] [review]
patch, don't call RemoveObserver() in the dtor, as per ccarlen's suggestion.  add a comment above the call to AddObserver(), fix some tabs / whitespace.

sr=mscott

Comment 24

[(not reading, please use [e-mail] instead)]

*** Bug 134289 has been marked as a duplicate of this bug. ***

Comment 25

[(not reading, please use [e-mail] instead)]

fixed on trunk.

Comment 26

[Jaime Rodriguez, Jr.]

Yulian - Can you verify this fix on the trunk today, and that no related
regressions occur?

Comment 27

[yulian chang]

20020419 Trunk build on Win2K

Verified.

Comment 28

[yulian chang]

20020419 Trunk build on WIns

The problem has been fixed.

Comment 29

[scottputterman]

adding adt1.0.0+.  Please check into the branch as soon as possible and add
fixed1.0.0

Comment 30

[yulian chang]

Has been checked into the branch yet? No keyword: fixed1.0.0

Comment 31

[(not reading, please use [e-mail] instead)]

this has not been checked on the branch yet.  I'll do that today.

Comment 32

[(not reading, please use [e-mail] instead)]

fixed on the branch as well.  setting fixed1.0.0

Comment 33

[yulian chang]

20020425 branch build on Wins

Verified in branch build

Comment 34

[timeless]

*** Bug 136274 has been marked as a duplicate of this bug. ***

Comment 35

[yulian chang]

20020603 branch build on Win2K

Verified with New Turbo Model. No regression.