86688 - nsAutoConfig object is not destroyed after it is done.

Status: RESOLVED DUPLICATE of bug 97182

(Core :: Preferences: Backend, defect)

Description

[Mitesh Shah]

nsAutoConfig object is not being destroyed even it finishes its task. This is 
because it is registering itself as an observer to the profile change event and 
it is not a weak reference. Profile manager stays alive till browser exits so it 
keeps a reference to nsAutoConfig till end. Actually, once the nsAutoConfig 
finished loading/executing autoconfig.jsc, it is no longer needed (unless it 
kicks of a timer routine). There are two solutions:
- Declare it as a weak reference. The problem is that it gets deleted right away 
when it is created as a part of the nsAppStartupNotifier::Observe(). Because it 
is created on a stack and when method finishes it gets out of scope and without 
a reference it gets deleted.
- Call a removeObserver() once you are done. Right now removeObserver() is 
called through destructor which is like catch22. Destructor will not be called 
without removeObserver() and vice-a-versa. Instead, write a separate method 
which will call removeObserver(). Once the refCnt reaches zero, Release() will 
be called on the object.

Comment 1

[sairuh (rarely reading bugmail)]

->rvelasco for qa [punt if you're not the right person!]

Comment 2

[Brian Nesse (gone)]

If you wish to support nsWeakReference, you should be able to eliminate this by
ADDREF'ing yourself on creation and RELEASE'ing yourself when you are "done".

This would be essentially the same as calling "removeObserver" when you are
"done" without supporting nsWeakReference.

All things considered, it sounds like option 2 is less work...

Comment 3

[Rodney Velasco]

over to luke for QA contact.

Comment 4

[Mitesh Shah]

Attaching a patch for review.

Caling removeObserver once we are done with AutoConfig.

Comment 5

[Mitesh Shah]

Attachment: propsed patch 1

Comment 6

[Mitesh Shah]

Attachment: updated patch - moved FinishAutoConfig in the if (first_time) {}

Comment 7

[Brian Nesse (gone)]

Shouldn't you only be calling FinishAutoConfig() if you haven't started a
callback timer?

Comment 8

[Mitesh Shah]

It's ok to remove from the observer list even if the timer callback has started.
It won't get released if the timer callback has started and that's what we want.
Since the timer callback will initiate DownloadAutoConfig() through Notify() we
don't need to be an observer to "profile-after-chage" topic anymore to get it
started. It is required for the first time only. So I have put the
FinishAutoConfig() in the first time only code. In fact, if you put it outside
the if (first_time) block, it will fail to remove itself from the observerlist
during the timer callback since it has already been removed once.

Comment 9

[Brian Nesse (gone)]

Of course...duh. For some reason I was equating removal from the observer list
with immediate destruction.
r=bnesse.

Comment 10

[Mitesh Shah]

Attachment: updated patch

Comment 11

[Mitesh Shah]

New pach with additional FinisAutoConfig() at the early exits from the
DownloadAutoConfig() and Observe() in case of errors. We need to release the
object when we exit out of the autoconfig in any case

Comment 12

[Brian Nesse (gone)]

Good catch on the error condition case, and my bad for not noticing this before, 
but... Wouldn't it just be much simpler to add the RemoveObserver code into the 
Observe() method, right after calling DownloadAutoCfg()? ;)

Comment 13

[Mitesh Shah]

I think it's a very good suggestion. The new patch moves the removeObsever to
Observe() itself. 

Comment 14

[Mitesh Shah]

Attachment: updated patch

Comment 15

[Brian Nesse (gone)]

Two things:

First, the mPrefBranch = 0 in the destructor is not necessary. You have no 
circular reference issues with the prefBranch object so you don't need to do 
this.

Second, you have changed the logic in Observe such that DownloadAutoCfg() now can 
get called with mCurrProfile being null if it fails to get the profile name. 
Previously you returned an error if this failed... I don't think this was your 
intention, though it appears that the code will deal with this situation. If this 
was your intention, you should add comments to make it clear.

Comment 16

[Mitesh Shah]

Ok. I will remove the mPrefBranch = 0 from the destructor.
Regarding, the profile name being NULL, I checked that the code will deal with
the situation so I didn't return on error. I will put a comment there.

Comment 17

[Mitesh Shah]

Attachment: latest patch

Comment 18

[Brian Nesse (gone)]

Oh crap... I forgot... please indent these lines under the if case before you
check in...
 // setting the member variable to the current profile name
 mCurrProfile = NS_ConvertUCS2toUTF8(profileName); 

with that change. r=bnesse.

Comment 19

[Mitesh Shah]

Adding Seth for super review

Comment 20

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

it seems to me that the reason you are continuing on after
profile->GetCurrentProfile(getter_Copies(profileName)); fails (instead of
returning immediately) is so that we'll hit the code that removes the observer.

but you'll be returning NS_OK instead of NS_ERROR_*, assuming that
do_GetService() and RemoveObserver() succeed.

is that what you want?

Comment 21

[Mitesh Shah]

That's right. I am not returning on error so that it will hit the remove
observer code. 

It's fine to return NS_OK, even if profile->GetCurrentProfile() fails. 

Actually, the previous version was wrong in a sense that it was not calling 
DownloadAutoCfg() if the call to profile->GetCurrentProfile() fails. We want to
go and download the autoconfig.jsc even if we don't get the profile name.
Profile name is used as a backup if we run the browser without a default email
address. Email address or profile name is used to pass as an parameter to the
URL. If none of them is available, nothing will be added to the URL and the
default case of the script will be used and that's exactly what we want. And of
course, it would be good if we report an error saying that GetCurrentProfile()
fails. Right now, we don't have a proper error reporting mechanism for all these
preferences code.

I will add a NS_WARNING("..") for the else clause of if (NS_SUCCEEDED(rv)) {}

Comment 22

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

sounds good.

attach the final patch bnesse can do the final r= and I can do the final sr=

Comment 23

[Mitesh Shah]

indentation didn't seem right in the prev patch beacuse I ignored the white
space changes in the diffs. Here is the new patch. Line numbers is not very
accurate since I already have another patch in the file. 

Comment 24

[Mitesh Shah]

Attachment: latest patch

Comment 25

[Brian Nesse (gone)]

Indentation also appears incorrect in this patch, but I verified with mitesh that 
it was done with ignore whitespace again. Looks correct in his source file.
r=bnesse.

Comment 26

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

sr=sspitzer

Comment 27

[Mitesh Shah]

Code checked in.
Checking in modules/libpref/src/nsAutoConfig.cpp;
/cvsroot/mozilla/modules/libpref/src/nsAutoConfig.cpp,v  <--  nsAutoConfig.cpp
new revision: 3.9; previous revision: 3.8
done

Comment 28

[Rodney Velasco]

Luke, please work with mitesh on how to verify this bug.  This looks like
backend autoconfig stuff.

Comment 29

[lrg]

This bug was removed because of bug 94349

Comment 30

[lrg]

THis the duplicate, sorry about that mix up.

*** This bug has been marked as a duplicate of 97182 ***