94349 - Unable to go to HomePage on startup in MfcEmbed.

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Chak Nanga]

Looks like the profile Observer stuff has changed somewhere along which is
causing this.

Previously we used to get a "profile-after-change" topic fired when you start
MfcEmbed in response to which CMfcEmbedApp::InitializePrefs() was being called
where we read the prefs.

Now we do not get called on that topic on startup and hence InitializePrefs()
does not get called to read the prefs.
[However all of this works when we switch profiles after the app is running]

Comment 1

[Conrad Carlen (not reading bugmail)]

I'll take a look at it.

Comment 2

[Chak Nanga]

Attachment: Patch to fix the prefs issue...

Comment 3

[Chak Nanga]

While the patch above will fix the issue i'm curious as to whether or not the
observer topic as mentioned above should be called or not.

If not, then this patch should do the job.

Comment 4

[Conrad Carlen (not reading bugmail)]

> Previously we used to get a "profile-after-change" topic fired when you start
> MfcEmbed in response to which CMfcEmbedApp::InitializePrefs() was being
> called where we read the prefs.

If you're not getting a "profile-after-change" notification when starting up,
something is seriously wrong. That needs to be found out before anything.
Explicitly calling InitializePrefs() may be masking the problem.

Comment 5

[Conrad Carlen (not reading bugmail)]

Yikes. I found the problem. nsIObserverService doesn't handle element removal
during iteration. What happens is this:

1) nsAutoConfig also observes the "profile-after-change" topic (news to me) and
it is the first thing to register for that topic - before mfcEmbed.
2) When nsAutoConfig observes this change for the first time, it removes itself
as an  observer by calling nsIObserverService::RemoveObserver().
3) This causes the iteration to skip the next observer (mfcEmbed)

Scott, what to do about this? Is nsAutoConfig doing something illegal in
removing itself during an Observe() call? Or is nsIObserverService's iteration
broken?

Comment 6

[Conrad Carlen (not reading bugmail)]

Mitesh, the code which is causing this (or surfacing this problem with
nsIObserverService) is yours:
http://lxr.mozilla.org/mozilla/source/modules/libpref/src/nsAutoConfig.cpp#207.

It seems reasonable that it should be able to remove itself as an observer and
not cause the next observer to be skipped.
On the other hand, why is it removing itself? Why would we want some behavior to
occur only on the *first* profile chosen and not on subsequent profiles?
MfcEmbed supports runtime profile switching and I'm not sure if once-only
observation is what we want.

Comment 7

[Mitesh Shah]

I never knew abt run time profile switching. The reason I am calling remove
observer  so that it will decrease the reference on AutoConfig object and as a
consequence it will get deleted. Previously, it was not getting deleted even if
it goes out of context and I thought it was a memory leak. Please guide me, if
you know a better way to release the memory associated with nsAutoConfig object.
CCing bnesse who was also involved withthis bug. 

Comment 8

[Conrad Carlen (not reading bugmail)]

The reason you have a leak is that nsAutoConfig does not mix in
nsSupportsWeakReference and have nsISupportsWeakReference in its QueryInterface.
When that's the case, registering it with nsIObserverService will force the
observer service to use a strong ref rather than a weak ref and since that
service has the lifetime of the app, it will never be released until XPCOM is
shut down.

Having the object support weak refs will solve that. Take a look at some of the
other profile change observers. I think they all use the weak ref model.

Comment 9

[Mitesh Shah]

The problem with supporting weak reference is that it gets deleted prematurely
when it is created as a part of AppStartup notifiers. Please refer to bug 86688
for more details. The other solution is to add a reference when it is created
and remove a reference when we are done. 

On the other hand, I am also interested in observing the change in the profile.
If browser is also going to support run time profile switching it whould be
worth while to keep the nsAutoConfig live to listen to these profile changes. 

Also, this seems to be an odd behavior on part of nsIObserverService to skip the
next element when the previous element remove itself from the observer list. 

Comment 10

[Brian Nesse (gone)]

Yuk! Making nsAutoConfig support nsWeakReference could be worked out, but right
now the nsAutoConfig object is created as a local object. Thus when the scope of
that function is left, the only reference to the object is in the
nsIObserverService. Supporting nsWeakReference causes it to be deleted
immediately. :)

After talking with conrad, based on the assumption that seamonkey will be
supporting runtime profile switching in the near future, we will most likely
either need to keep the object around or find a better place to create it.

Comment 11

[Conrad Carlen (not reading bugmail)]

> Also, this seems to be an odd behavior on part of nsIObserverService to skip
> the next element when the previous element remove itself from the observer list. 

I agree. That's the real problem here. However, making nsAutoConfig stay in
memory doesn't seem too bad because (1) it would have to in order to respond to
later profile switches if this feature ever comes to seamonkey and (2)
nsAutoConfig is in the pref dll so it's code is already in memory forever anyway
and it looks like the memory footprint of an nsAutoConfig object is pretty small.

Comment 12

[Chak Nanga]

Looks like this one does not belong to me. Which one of you want to take
this...Thanks

Comment 13

[Mitesh Shah]

I will take it

Comment 14

[Conrad Carlen (not reading bugmail)]

Since AutoConfig uses the startup notifier mechanism, it gets used by any app
(embedding and seamonkey) which uses the prefs dll. Do we want this feature to
be used universally, i.e. in embedding apps or do we want to explicitly
initialize this in seamonkey only? Jud, what's your opinion?

Comment 15

[Judson Valeski]

I haven't tracked the memory stuff well in this bug, but here's what I think....

a) list element removal during traversal works in some cases, and doesn't in
others. it's a function of the list implementation. this list impl doesn't seem
to support it, so we need to rectify that issue for sure (remove the element
after traversal most likely).
b) from a distance, nsIWeakReference should be supported here.
c) I don't know enough about the AutoConfig feature, can someone inlighten me?

Comment 16

[Mitesh Shah]

AutoConfig is an eClient feature for enterprise customers.  It allows IS to
store the enterprise specific preferences on  remote/local server. If browser
finds a specific preference, it goes and downloads the preferences file and
applies it in the current context. Please refer bug 70348 for more details.

Comment 17

[Judson Valeski]

this should be eClient/seamonkey only. 

sounds like it should be a separate module that only eClient installs. at
registration time it can hook itself in using the startup registration stuff.

should I file a bug for separation? how much code does it add?

Comment 18

[Brian Nesse (gone)]

AutoConfig is fairly lightweight. Unfortunately it is closely tied to prefapi.c
(that whole annoying JS parsing thing). Without exporting from libpref some of
the cruft we are trying to hide inside it, we can't currently seperate
AutoConfig from libpref.

What I'd like to see is the JS parsing stuff pulled from standard prefs support.
We could still do it for autoconfig and netscape.cfg... just move the JS parsing
stuff into a seperate library with autoconfig and netscape.cfg support. This
would also keep us from having to start up JS just to launch the prefs service.

Comment 19

[Judson Valeski]

that's what I figured. in that case, can we turn this off somehow in embedding
builds?

Comment 20

[Brian Nesse (gone)]

Well... if you get right down to it... Trying to do exactly that is what caused
this bug in the first place. :)

But seriously, as it seems we need to support live profile switching, we are
going to have to change something anyway. It's a matter of figuring out how much
we want to try to tackle at one time.

How critical is this timewise? We can whip up a band-aid that eliminates this
bug in a minute, or we can try and come up with a fix that deals with all of the
issues.
What's your preference?

Comment 21

[Judson Valeski]

runtime profile switching is a requirement, we need to address this with that in
mind. chak's patch is a workaround we should avoid IMO.

Comment 22

[Conrad Carlen (not reading bugmail)]

> Without exporting from libpref some of the cruft we are trying to hide inside
> it

Besides PREF_EvaluateConfigScript, is there any other nesc cruft? If not, it
seems like if nsIPrefService::ReadConfigFile() was like
nsIPrefService::ReadUserPrefs(in nsIFile aFile) then AutoConfig could pull the
data down to a local file, use nsIPrefService::ReadConfigFile(aFile) and not
have to link with libpref. Also, it would make nsAutoConfig::evaluateLocalFile()
 simpler.

Unless there was a good reason (that I don't know about) to make ReadConfigFile
not take a file param.

Doing this, and putting the AutoConfig component in another dll, would remove
this component from embedding builds which would fix the immediate problem. 

Comment 23

[Brian Nesse (gone)]

Sure, what I'm getting at is...

If you want mfcEmbed fixed today, we can basically back out the change which
caused this to occur. We won't be any worse off then we were before as far as
profile switching goes.

If you want it fixed by 0.9.4, we can try and come up with a better solution.

Comment 24

[Brian Nesse (gone)]

Yes PREF_EvaluateConfigScript is the root of it... not certain if that's all or
not. There could be issues with having AutoConfig actually write the file
locally (possibly security or maybe write access, I don't know... I'm thinking
out loud here).

ReadConfigFile() is basically a wrapper function that gets the name of the file
to read and passes it to the function which opens and reads the file... not much
to be gained by modifing that.

Comment 25

[Chak Nanga]

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

Comment 26

[Brian Nesse (gone)]

We have come up with a solution which will fix this by keeping the nsAutoConfig
object from being instantiated in embedding builds. Mitesh is working on a patch.

Comment 27

[gordon]

This bug may be currently breaking the cache as well.

Comment 28

[Judson Valeski]

this is blocking a topembed bug.

Comment 29

[Mitesh Shah]

Actually, the problem is with nsIObserverService, which skips the element if you
remove an observer during the enumeration. Created a new bug  94636 against
XPCOM. I am attaching a small patch to solve the issue right now. Also working
on changing the nsAutoConfig creation process that will instantiate the object
only when netscape.cfg contains the required pref.

Comment 30

[Mitesh Shah]

Attachment: patch - commenting out RemoveObserver() code

Comment 31

[Mitesh Shah]

anyone wants to do r/sr?

Comment 32

[Darin Fisher]

sr=darin

Comment 33

[Conrad Carlen (not reading bugmail)]

r=ccarlen

Comment 34

[Judson Valeski]

I like the idea of changing the auto config creation to a "need only" model.
what's the bug number for that work?

Comment 35

[Mitesh Shah]

Fix cheked in

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

Comment 36

[Judson Valeski]

re-opening until we get this on the 0.9.2 branch. if it's not relevant there,
please close this again.

Comment 37

[Judson Valeski]

patch available in the blocking bug.

Comment 38

[Judson Valeski]

ignore my last comment; sorry.

Comment 39

[Mitesh Shah]

This is not relevant in 0.9.2 branch. The code was added after 0.9.2 branch as a
fix to bug 86688. 

Comment 40

[Brian Nesse (gone)]

Re-closing.

Comment 41

[Mitesh Shah]

The bug number for moving nsAutoConfig to 'need only' basis is 87661

Comment 42

[Tracy Walker [:tracy]]

verified fixed on mfcEmbed build 2001-08-10-05-trunk