116306 - Switching themes by switching profiles is broken

Status: RESOLVED WORKSFORME

(Core Graveyard :: Skinability, defect, P2)

Description

[Conrad Carlen (not reading bugmail)]

Sometime between 2001121703 and 2001121803, this feature got completely broken.
Steps To reproduce:
(1) Have more than 1 profile, each with a different theme.
(2) Turn turbo on in the preferences and exit.
(3) Choose "Navigator" in the system tray and the profile picker will come up
(4) Choose a profile with a different theme and click "Start"
(5) The Navigator window that comes up has completely malformed chrome.

Didn't some live theme switching code go in recently? Could this have wacked the
theme switching which happens on a profile switch (which has worked for a long time)

Comment 1

[Conrad Carlen (not reading bugmail)]

Here is the check-in for theme switching that went in during that time:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=hyatt%25netscape.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=12%2F17%2F2001&maxdate=12%2F18%2F2001&cvsroot=%2Fcvsroot
Looking at all check-ins during that time, this seems most likely to have caused
it. Hyatt - Can you look at this?

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

-> hyatt

Comment 3

[Bill Law]

I'd suggest mozilla0.9.8 for this one.  It makes turbo mode pretty much useless.

Comment 4

[Bill Law]

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

Comment 5

[Bill Law]

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

Comment 6

[Peter Trudelle]

p2/nsbeta1

Comment 7

[David Hyatt]

Attachment: Patch to handle a case that was missing. Aha! :)

So I was being bitten by something that was just never implemented, namely
support for fetching stylesheets on a miss from the XUL cache (when the proto
XUL doc was hit in the cache).

Comment 8

[David Hyatt]

Ignore the XULElement patch.  XULDocument is the only relevant file.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 65766 [details] [diff] [review]
Patch to handle a case that was missing. Aha! :)

r=dbaron, except you could switch the logic to

if (!IsChromeURI(uri))
  continue;

and avoid an extra indent.

Comment 10

[Blake Ross]

Comment on attachment 65766 [details] [diff] [review]
Patch to handle a case that was missing. Aha! :)

sr=blake

Comment 11

[David Hyatt]

Fixed.

Comment 12

[Conrad Carlen (not reading bugmail)]

Yay! It works again. Thanks.

Comment 13

[Brendan Eich [:brendan]]

Don't mix two-space indentation with four-space -- respect emporer Watersonus
Maximus when in his Rome!

a=[e-mail] retroactively.

/be

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Note: The checkin to this bug caused bug 121057  (Prefs panel "Advanced|Scripts
& Windows" can only be visited once per session)

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

I see two files being patched in attachment 65766 [details] [diff] [review] but only 1 file with a checkin
on the bonsai log. 
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=01%2F19%2F2002&maxdate=01%2F20%2F2002&cvsroot=%2Fcvsroot

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/content/xul/content/src/nsXULElement.cpp
shows no checkin to that file which this patch also has changes for.

Looks like a botched checkin?  Reopening this bug.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Err, I guess I missed comment #8 which nullifies the XULElement part of the
patch.  Sorry about my last comment.  I'll leave this open though as it did
cause a regression.

Comment 17

[Conrad Carlen (not reading bugmail)]

I spoke too soon that it worked. While the chrome that opens after switching the
profile is not mangled anymore, it's not the right skin. Check out the stack trace:

nsChromeRegistry::LoadProfileDataSource() line 2853
nsChromeRegistry::ConvertChromeURL(nsChromeRegistry * const 0x004c7cf0, nsIURI *
0x033ae480, char * * 0x033ae3f0) line 524 + 8 bytes
nsChromeProtocolHandler::NewChannel(nsChromeProtocolHandler * const 0x004c1c50,
nsIURI * 0x033ae480, nsIChannel * * 0x0012c298) line 619 + 63 bytes
nsIOService::NewChannelFromURI(nsIOService * const 0x004b33d0, nsIURI *
0x033ae480, nsIChannel * * 0x0012c298) line 794 + 31 bytes
NS_OpenURI(nsIChannel * * 0x0012c2d8, nsIURI * 0x033ae480, nsIIOService *
0x004b33d0, nsILoadGroup * 0x00000000, nsIInterfaceRequestor * 0x00000000,
unsigned int 0) line 148 + 20 bytes
NS_OpenURI(nsIInputStream * * 0x0012c428, nsIURI * 0x033ae480, nsIIOService *
0x00000000, nsILoadGroup * 0x00000000, nsIInterfaceRequestor * 0x00000000,
unsigned int 0) line 186 + 49 bytes
CSSLoaderImpl::LoadAgentSheet(CSSLoaderImpl * const 0x033ae6a0, nsIURI *
0x03ddd320, nsICSSStyleSheet * & 0x00000000, int & 65, nsICSSLoaderObserver *
0x00000000) line 1598 + 21 bytes
nsXBLPrototypeResources::FlushSkinSheets() line 140 + 62 bytes
nsXBLPrototypeBinding::FlushSkinSheets(nsXBLPrototypeBinding * const 0x03dda2f0)
line 431 + 11 bytes
FlushScopedSkinSheets(nsHashKey * 0x03dda120, void * 0x03dda2f0, void *
0x00000000) line 383
_hashEnumerate(PLHashEntry * 0x03dda0e0, int 3, void * 0x0012c54c) line 198 + 26
bytes
PL_HashTableEnumerateEntries(PLHashTable * 0x03dda218, int (PLHashEntry *, int,
void *)* 0x1002cbf0 _hashEnumerate(PLHashEntry *, int, void *), void *
0x0012c54c) line 429 + 15 bytes
nsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x01e1a340
FlushScopedSkinSheets(nsHashKey *, void *, void *), void * 0x00000000) line 361
+ 21 bytes
nsSupportsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x01e1a340
FlushScopedSkinSheets(nsHashKey *, void *, void *), void * 0x00000000) line 190
nsXBLDocumentInfo::FlushSkinStylesheets(nsXBLDocumentInfo * const 0x03ddd4e0)
line 391
FlushScopedSkinStylesheets(nsHashKey * 0x03dc20f0, void * 0x03ddd4e0, void *
0x00000000) line 399
_hashEnumerate(PLHashEntry * 0x03dc1c70, int 0, void * 0x0012c5ec) line 198 + 26
bytes
PL_HashTableEnumerateEntries(PLHashTable * 0x005093c8, int (PLHashEntry *, int,
void *)* 0x1002cbf0 _hashEnumerate(PLHashEntry *, int, void *), void *
0x0012c5ec) line 429 + 15 bytes
nsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x01da9360
FlushScopedSkinStylesheets(nsHashKey *, void *, void *), void * 0x00000000) line
361 + 21 bytes
nsSupportsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x01da9360
FlushScopedSkinStylesheets(nsHashKey *, void *, void *), void * 0x00000000) line 190
nsXULPrototypeCache::FlushSkinFiles(nsXULPrototypeCache * const 0x00509330) line 422
nsChromeRegistry::FlushCaches() line 1180
nsChromeRegistry::Observe(nsChromeRegistry * const 0x004c7cf4, nsISupports *
0x004b3cd0, const char * 0x0178c918, const unsigned short * 0x0178c8ac) line 3194

nsChromeRegistry::Observe is being called for "profile-before-change" when the
profile is going away and so it calls FlushCaches. Notice that this causes a
chrome load and we end up re-loading the profile data even though we're in the
middle of unloading it. Next time around, on getting a "profile-after-change"
nothing happens because mProfileInitialized is now true.
 

Comment 18

[Brendan Eich [:brendan]]

hyatt says it seems as though the flush shouldn't happen until UI is gone, but
hyatt isn't around for the rest of today and for much of tomorrow -- conrad, can
you attempt a fix for 0.9.8?

/be

Comment 19

[David Hyatt]

Could the flushing call be moved to after-change?  I don't really want to have 
to split up the function into a "Flush" and a "Reload", since I'd do two 
crawls of open UI window trees.  

Comment 20

[Conrad Carlen (not reading bugmail)]

> Could the flushing call be moved to after-change?
Actually, the call to FlushCaches() from Observe("profile-before-change") can
just be done away with. FlushCaches() is going to be called by the next
LoadProfileDataSource() anyway. Just tried it and it appears to work. A little
more testing...

Comment 21

[Conrad Carlen (not reading bugmail)]

Attachment: Removes unneeded call to FlushCaches()

This seems to do it.

Comment 22

[Conrad Carlen (not reading bugmail)]

need r=/sr=/a=

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 66112 [details] [diff] [review]
Removes unneeded call to FlushCaches()

r/sr=[e-mail], hyatt, can you review also?

/be

Comment 24

[David Hyatt]

r=hyatt

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 66112 [details] [diff] [review]
Removes unneeded call to FlushCaches()

Recording hyatt's review and adding a=[e-mail] for checkin to the
0.9.8 branch.

/be

Comment 26

[Conrad Carlen (not reading bugmail)]

Checked into trunk - will check into branch ASAP.

Comment 27

[Conrad Carlen (not reading bugmail)]

Checked into 0.9.8

Comment 28

[R.K.Aa.]

a long shot.. but can this have caused bug 121860?

Comment 29

[Conrad Carlen (not reading bugmail)]

No, the line of code removed by the latest checkin only would have been executed
when a profile is shutdown during a profile switch. That currently only happens
with turbo mode and not with changing themes via prefs or the view menu. 

Comment 30

[Grace Bush]

I'm still seeing weird behavior with build 2002012506

I have 3 profiles, each with their own theme,  I am getting duplicate buttons on
my browser- two completely different print icons.

Also on the launch of second profile, activation comes up with blank screen and
option to save the activation.xul file??? not sure if that is related but no
activation screens can be seen

Comment 31

[Conrad Carlen (not reading bugmail)]

Have to check this with commercial and its activation window. On my current
mozilla build, it's working fine.

Comment 32

[Asa Dotzler [:asa]]

removing keyword since the fix for this has landed on the 0.9.8 branch

Comment 33

[Grace Bush]

see also bug 11920 in bugscape  for activation activity
at one point on XP testing- I had 3 profiles, 3 themes and could not launch app- 
error as no xbl binding.  I had to clean all profiles and start over

Comment 34

[Patty Mac]

Grace, what platform, is that windows 98?

Comment 35

[Grace Bush]

NT and XP

Comment 36

[Peter Trudelle]

->0.9.9

Comment 37

[Conrad Carlen (not reading bugmail)]

Grace, Can you check this with mozilla, not commercial? I'd like to separate out
the activation window problems.

Comment 38

[Peter Trudelle]

nsbeta1+ per Nav triage team, but Dave needs to check for possible dup.

Comment 39

[Grace Bush]

tested mozilla
3 profiles - 3 themes (downloaded eskimo to try)
I did not see any weirdness in chrome except after using profile with specific 
theme, next time ProfileManager comes up, it has that theme
I thought mozilla PM always came up classic...but I may be wrong.

Regular turbo tests passed

Comment 40

[Conrad Carlen (not reading bugmail)]

Grace: Thanks, knowing that this happens only in commercial helps narrow it down
a bit.

> next time ProfileManager comes up, it has that theme
> I thought mozilla PM always came up classic...but I may be wrong.

It will use the theme that was in use last. See bug 99387 for why.

Comment 41

[Peter Trudelle]

->ccarlen

Comment 42

[Asa Dotzler [:asa]]

Comment on attachment 66112 [details] [diff] [review]
Removes unneeded call to FlushCaches()

obsoleting old patch.

Comment 43

[Patty Mac]

I couldn't reproduce this one on my windows 2k (branch build: 
2002-05-29-08-1.0.0).
Grace, are you able to reproduce this in the recent build?

Comment 44

[Grace Bush]

I tested again today and cannot reproduce either

Comment 45

[scottputterman]

If no one can reproduce this, can you mark it Worksforme.  sspitzer fixed a
theme related bug where you could get in a state where the theme was a mixture
of modern and classic.  He fixed this sometime in April so perhaps that fixed
this bug.

Comment 46

[Andrew Hagen]

This bug seems to rely on multiple profiles and Quick Launch, a combination not
supported in recent releases.

Comment 47

[scottputterman]

marking nsbeta1- for Mach V per ADT.

Comment 48

[Peter Trudelle]

Recent changes in the QuickLaunch model have obviated all other related
multi-profile bugs, please retest this with the pref
"browser.turbo.singleProfileOnly" set to false.

Comment 49

[Grace Bush]

marking worksforme
Have not seen on builds for a long time- retested this morning with branch build 
and turbo pref set appropriately for using multiple profiles