46013 - tree painting glitch in themes prefpanel tree

Status: VERIFIED WORKSFORME

(Core Graveyard :: Skinability, defect, P1)

Description

[selmer (gone)]

2000-17-20-08

In edit prefs, the themes panel has ZERO themes to select.  Resizing the dialog
does not make anything show up.  This makes it pretty tricky to switch themes :-(

Comment 1

[hangas]

Theme switching bugs are to be assigned to Skinability.  The Themes component is 
only for problems with The actual Theme.  Sending to component owner.

We may need more info on this one, it has been working and I saw it working 
today.  Can someone in QA confirm this is really happening?

Comment 2

[Blake Ross]

We have a couple of scattered bugs about specific themes (i.e. Classic) not 
appearing in prefs, but I think this is a different (and more recent) 
regression.  I saw this as well in yesterday's builds on WinNT 4; there were 
absolutely no themes to choose.  A workaround *seemed* to be to quit and 
restart Mozilla, but even that seemed flaky at times.  Selmer, does that work 
for you?  Nominating for PDT, letting PDT decide if that oft-flaky workaround 
is good enough for beta2.

Comment 3

[Blake Ross]

At the end of bug 32688, jimmylee said:

"One side effect is that after installing, other themes from Installed Themes 
preference pane are absent.  Dan is going to submit a new bug about this since 
he has specific data to attach."

I'm not sure if this could be related (it doesn't sound like it since this 
happens regardless of whether you install a theme), but cc'ing dan and jimmy 
anyways.

Comment 4

[Blake Ross]

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

Comment 5

[Blake Ross]

Fixing up as bug 46035, though fenella said it works on linux.  cc'ing people 
from 46035

Comment 6

[Joe O'Connor]

I'm also seeing this on Linux with 2000-07-20-20.
No themes at all are showing up in the "installed themes" list.
I've had no luck with the quitting and restarting workaround...

Comment 7

[Daniel Veditz [:dveditz]]

The problem Jimmy mentioned at the end of bug 32688 was written up as bug 
45951, a completely different problem that comes about if you have both profile 
and global all-skins.rdf files. And you don't see nothing in that bug, you do 
see all the locally (profile) installed skins.

Comment 8

[don]

I'm marking the "nsbeta2+".  This is bad.  We have no themes.  Whatthehell 
happened?

Comment 9

[don]

Chris, Ben tells me this is actually your bug (some RDF thing) that you already
know about.

Comment 10

[Blake Ross]

nsbeta2+, P1...M20 target milestone?

Comment 11

[tao]

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

Comment 12

[Chris Waterson]

There was a regression this morning (7/21/2000) with RDF files getting trashed;
however, I've since fixed that.

I just installed 2000-07-21-08, which according to the FTP site, was really
produced at about 13:00 today (respin, with fixes). The *first* time I launched
the browser, opened prefs, I saw no themes. Shutdown, restart, and then themes
appeared.

So, it appears that you need to have a clean install. Changing summary to
indicate this. I'm probably not the right person to own this bug; it's either an
installer thang or an ordering problem with the way that all-packages.rdf etc.
are being generated. dveditz? ben? danm?

Comment 13

[Chris Waterson]

I should've noted above: I was using *commercial* bits. Results may vary with
2000-07-21-08 mozilla bits. Just pull something that was spun after
2000-07-21-13 or so.

Comment 14

[John Dobbins]

Tests with assorted nightly builds on Win98
all builds mozilla-win-talkback.zip
Build 071910 WFM.
Build 071920 Busted.
nsPrefWindow.js was modified between these builds. 17:00 7/19 ver 1.8
Build 072008 Busted
copied nsPrefWindow.js ver 1.7 from 071910 to 071920 and 072008.
Build 071920 with nsPrefWindow.js ver 1.7 WFM
Build 072008 with nsPrefWindow.js ver 1.7 WFM
Build 072108 with nsPrefWindow.js ver 1.7 Busted
files that generate the RDF were modified between builds 072008 and 072108.
copied the generated rdf files in bin/chrome from 072008 to 072108.
build 072108 with nsPrefWindow.js ver 1.7 and copied RDF files WFM :-)

Comment 15

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

nsPrefWindow.js has nothing to do with this. 

Comment 16

[Blake Ross]

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

Comment 17

[Chris Waterson]

See stack below. The problem is this:
- To initialize the chrome registry, we install new chrome.
- To install new chrome, we try to create and load RDF/XML datasource.
- To create and load an RDF/XML datasource, we need Necko
- To initialize Necko, we need to get Necko's string bundle.
- To get Necko's string bundle, we need the chrome registry.

Oops. I'm going to see if updating to get warren's new necko juice fixes this.


nsChromeRegistry::AddToCompositeDataSource(nsChromeRegistry * const 0x00af8040, 
int 0x00000000) line 1985
nsChromeRegistry::ConvertChromeURL(nsChromeRegistry * const 0x00af8040, nsIURI 
* 0x00fc3500, char * * 0x0012e72c) line 448
nsStringBundleService::getStringBundle(const char * 0x00fc3820, nsILocale * 
0x00000000, nsIStringBundle * * 0x0012e830) line 736 + 78 bytes
nsStringBundleService::CreateBundle(nsStringBundleService * const 0x00fc36f0, 
const char * 0x00fc3820, nsILocale * 0x00000000, nsIStringBundle * * 
0x0012e830) line 842
nsPref::GetLocalizedUnicharPref(nsPref * const 0x00b34a30, const char * 
0x020ac8ac INTL_ACCEPT_LANGUAGES, unsigned short * * 0x0012e908) line 912 + 74 
bytes
nsHTTPHandler::PrefsChanged(const char * 0x00000000) line 1436 + 71 bytes
nsHTTPHandler::Init() line 776
nsHTTPHandlerConstructor(nsISupports * 0x00000000, const nsID & {...}, void * * 
0x0012ea40) line 85 + 149 bytes
nsGenericFactory::CreateInstance(nsGenericFactory * const 0x00fc21e0, 
nsISupports * 0x00000000, const nsID & {...}, void * * 0x0012ea40) line 48
nsComponentManagerImpl::CreateInstance(nsComponentManagerImpl * const 
0x00aa47d0, const nsID & {...}, nsISupports * 0x00000000, const nsID & {...}, 
void * * 0x0012ea40) line 1237 + 24 bytes
nsComponentManager::CreateInstance(const nsID & {...}, nsISupports * 
0x00000000, const nsID & {...}, void * * 0x0012ea40) line 82
nsServiceManagerImpl::GetService(nsServiceManagerImpl * const 0x00aa4b40, const 
nsID & {...}, const nsID & {...}, nsISupports * * 0x0012eacc, 
nsIShutdownListener * 0x00000000) line 346 + 19 bytes
nsServiceManager::GetService(const nsID & {...}, const nsID & {...}, 
nsISupports * * 0x0012eacc, nsIShutdownListener * 0x00000000) line 562
nsGetServiceByCID::operator()(const nsID & {...}, void * * 0x0012eacc) line 46 
+ 22 bytes
nsCOMPtr<nsIHTTPProtocolHandler>::assign_from_helper(const nsCOMPtr_helper & 
{...}, const nsID & {...}) line 856 + 18 bytes
nsCOMPtr<nsIHTTPProtocolHandler>::nsCOMPtr<nsIHTTPProtocolHandler>(const 
nsCOMPtr_helper & {...}) line 553
nsLayoutModule::SetUserAgent() line 458 + 30 bytes
nsLayoutModule::Initialize() line 239
nsLayoutModule::GetClassObject(nsLayoutModule * const 0x00f98d80, 
nsIComponentManager * 0x00aa47d0, const nsID & {...}, const nsID & {...}, void 
* * 0x0012f070) line 290 + 8 bytes
nsNativeComponentLoader::GetFactoryFromModule(nsDll * 0x00aab710, const nsID & 
{...}, nsIFactory * * 0x0012f070) line 1170 + 44 bytes
nsNativeComponentLoader::GetFactory(nsNativeComponentLoader * const 0x00aa6a30, 
const nsID & {...}, const char * 0x00f98c40, const char * 0x00f98c80, 
nsIFactory * * 0x0012f070) line 136 + 20 bytes
nsFactoryEntry::GetFactory(nsIFactory * * 0x0012f070, nsComponentManagerImpl * 
0x00aa47d0) line 214 + 58 bytes
nsComponentManagerImpl::FindFactory(nsComponentManagerImpl * const 0x00aa47d0, 
const nsID & {...}, nsIFactory * * 0x0012f070) line 1067
nsComponentManagerImpl::CreateInstance(nsComponentManagerImpl * const 
0x00aa47d0, const nsID & {...}, nsISupports * 0x00000000, const nsID & {...}, 
void * * 0x0012f244) line 1234 + 20 bytes
nsComponentManager::CreateInstance(const nsID & {...}, nsISupports * 
0x00000000, const nsID & {...}, void * * 0x0012f244) line 82
RDFXMLDataSourceImpl::Refresh(RDFXMLDataSourceImpl * const 0x00fa87b4, int 
0x00000001) line 888 + 45 bytes
nsChromeRegistry::InstallProvider(nsChromeRegistry * const 0x00af8040, const 
nsCString & {"package"}, const nsCString & 
{"resource:/chrome/packages/widget-toolkit/"}, int 0x00000000, int 0x00000001, 
int 0x00000000) line 1617 + 37 bytes
nsChromeRegistry::InstallPackage(nsChromeRegistry * const 0x00af8040, const 
char * 0x0012fa1c, int 0x00000000) line 1826 + 44 bytes
nsChromeRegistry::ProcessNewChromeBuffer(char * 0x00fa898d, int 0x0000022d) 
line 2298
nsChromeRegistry::CheckForNewChrome(nsChromeRegistry * const 0x00af8040) line 
2215
CheckForNewChrome() line 718 + 23 bytes
main1(int 0x00000001, char * * 0x00aa4f00, nsISupports * 0x00aa3050) line 832
main(int 0x00000001, char * * 0x00aa4f00) line 1093 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77f1b304()

Comment 18

[Chris Waterson]

Ok, I was wrong: it's not necko's string bundles that are causing the 
re-entrancy here. It's the fact that RDF relies on layout (and not just 
parser). It needs layout because where the "namespace manager" lives. 
Initializing the layout DLL gets the string bundle service to set the User 
Agent.

Comment 19

[Chris Waterson]

I moved the namespace manager out of the layout DLL and into the parser DLL. 
This fixes the above problem.

Long, long ago, I'd argued with peterl that the namespace manager should live 
in the parser. My reasoning was that the namespace manager was a necessary 
utility for anyone "outside layout" who was going to use the nsIContentSink 
interface to parse a document. He argued that it was really just part of 
layout, and not part of the parser.

The RDF/XML uses the namespace manager (like all the other content sinks do) to 
maintain the current namespace stack. It need not use the namespace manager: I 
could trivially re-write it to maintain its own namespace stack.

Frankly, at this point, doing the latter is more appealing to me than the 
amount of tree whackery that I'll need to do to move the namespace manager out 
of the layout DLL. However, that work is done, and I can commit it and do the 
necessary tree babysitting.

I thought I'd cc some other people to solicit feedback on what "the right thing 
to do" here is...

Comment 20

[vidur (gone)]

The real solution would be to separate all content-related code from the layout 
DLL. I think it's something we should consider post-Netscape 6. Our parser and 
DOM implementations should be components that can be used separately from layout 
- say by a server-based application. At this point, I'd say do what's needed. We 
can worry about module boundaries later.

Comment 21

[Warren Harris]

FYI, the necko -> string-bundle dependency went away last night. Mitch was 
hitting the same sort of problems with circularity between necko, 
string-bundles, chrome, rdf, layout, prefs and security. My checkin fixed his 
problem.

Comment 22

[Peter Trudelle]

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

Comment 23

[Chris Waterson]

Attachment: remove dependency between RDF/XML parser and nsINameSpaceManager

Comment 24

[Chris Waterson]

I think this is the right way to fix it. I've removed the RDF/XML 
content sink's dependency on nsINameSpaceManager. It was probably silly to have 
ever used it in the first place, since RDF/XML doesn't care about namespaces 
once the document has been parsed.

So now, reading an RDF/XML file does *not* require the layout engine to be 
loaded. Wow, imagine that.

Comment 25

[Chris Waterson]

rjc, any chance I could get you to code review the above patch?

- removed use of nsINameSpace and nsINameSpaceManager from RDF/XML parsing 
process.

- Instead, use an ad hoc stack to manage name space scoping: mNameSpaceStack is 
the stack of all opened namespaces. mNameSpaceScopes is an nsVoidArray that 
holds pointers into that stack, and allows us to pop namespace scopes when a 
tag is closed.

- Replaced GetNameSpaceID() with GetNameSpaceURI(), which returns a "const 
char**" that points to the URI of the namespace, or null if there is no 
namespace. Requires us to now do a null check & string compare when we need to 
test for a specific namespace.

- Misc cleanup to use nsCOMPtr's and stuff. This code was really ancient.

Comment 26

[Robert John Churchill]

Looks good to me, r=rjc  (hey, that rhymes)

Comment 27

[Chris Waterson]

fix checked in

Comment 28

[Blake Ross]

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

Comment 29

[fenella]

Linux (2000-07-26-08 M17)
Win32 (2000-07-26-09 M17)
Mac (2000-07-26-08 M17)
This problem has been fixed.

Comment 30

[hangas]

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

Comment 31

[Scott MacGregor]

I hate to be a schmuck and re-open this but I still have this problem. I just
performed a clean install on todays's beta2 release build: 7/27/00 and I still
have the same problem. No themes in the prefs picker.


It's only on the first launch.

Comment 32

[Blake Ross]

same here. 2000072708 WinNT4, installer build.

Comment 33

[Chris Waterson]

Attachment: fix chrome UI ownership model

Comment 34

[Chris Waterson]

hyatt: please review. I "fixed" the chrome UI datasource ownership model by just
making the chrome registry own both the composite datasource and the chrome UI
datasource objects, and letting it manually connect and disconnect the observer
mechanism.

The nsChromeUIDataSource now holds a weak reference to the composite datasource.
Maybe it should be cleared out once the two are disconnected in case somebody
holds on to the composite datasource longer than the chrome registry (actually,
that's not ever going to happen, but, WTF...)

I also got rid of that kooky implementation of nsChromeUIDataSource::Release(),
so things are a bit simpler.

Anyway, let me know what you think.

Comment 35

[Chris Waterson]

Attachment: better fix

Comment 36

[Chris Waterson]

Ok, I understand the problem now, and have coughed up a better hairball.

Since the nsChromeRegistry was not addref-ing the mUIDataSource, it's refcount
stayed at "one". This causes problems, because as soon as somebody addref's and
then release's it (which was happening as part of the normal RDF notification
mechanism in the composite datasource's OnAssert callback), the refcount goes up
to two, and then back down to one.

Now here's the broken part. There's a special implementation of Release() that
assumes when the refcount goes to 1 on the nsChromeUIDataSource, that it's time
to die. Unfortunately, it is *not* time to die yet!

I made nsChromeRegistry hold an owning reference to the nsChromeUIDataSource,
and cleaned up the "special" release method so that we'd do the right bloat log
accounting.

Comment 37

[Chris Waterson]

Attachment: additional patch, for extra safety

Comment 38

[Chris Waterson]

The second patch replaces nsCompositeDataSource's mObservers array with an
nsVoidArray. This means that we need to do "hand management" of references;
however, it keeps us from doing addref/release pairs just for notification.
(Which is what got us into trouble in the first place, here.)

While strictly not necessary to fix the bug, it will avoid crashers in the
chrome UI datasource where somebody tries to use the nsChromeRegistry's
composite datasource after the nsChromeUIDataSource has been nuked. (This never
happened when I was experimenting with the patch.)

Comment 39

[Chris Waterson]

fix checked in, r=hyatt

Comment 40

[leger]

Updating QA Contact.

Comment 41

[Paul Wyskoczka]

Seems to be fixed in build 2000072920 on Windows.  Will check other platforms
later today.

Comment 42

[Paul Wyskoczka]

Clean installs on Windows NT, Linux and Mac.
Was able to see the themes in the prefs panel
Switched from Classic to Modern without a problem.
Marking verified in build 2000080104 on all builds

Comment 43

[fenella]

Linux (2000-08-29-08 M18)
Mac (2000-08-29-08 M18)
No theme to select in the Preferences->Appearance->Theme field.
On Linux, when I resize it, theme names appear.
But on Mac, they do not appear even after resizing. Reopen the bug.

Comment 44

[Daniel Veditz [:dveditz]]

The Mac bug is probably something else -- looks like chrome is not getting 
registered by the installer because Mail and IRC do not show up on the tasks 
menu either.

Comment 45

[Chris Waterson]

to ben for initial investigation

Comment 46

[Paul Wyskoczka]

Updating QA contact to [e-mail]

Comment 47

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

spreading the love. 

hyatt, sounds like there's a tree painting bug for you here on linux. Also, I 
did some testing and this failure to register happens on both commercial and 
mozilla mac installers. The installed-chrome.txt file is apparently read as it 
vanishes, but the rdf files are never generated. Seems like the installer is 
doing everything right but first-run registration is failing. I'll poke a little 
more and add any further information.

Comment 48

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

I'll open a separate bug on the painting problem if I can reproduce it. 
Reassigning this to dveditz. Some investigation led me to believe that it was 
the file URLs in the installed-chrome.txt file that were causing the heartache. 
My mac debug build uses resource:/ urls for chrome registration and works just 
fine. Trying to use file:// urls caused problems. Would it be possible to change 
the installer to fill this file with resource URLs? Then you wouldn't need to 
maintain complex code to locate the file on disk, etc, you'd just be echoing 
resource:/Chrome/<provider_type>/<provider_name>/ to a file. 

Comment 49

[Daniel Veditz [:dveditz]]

Let's keep this bug for the painting thing and stop bug drift. Bug 50689 has 
already been opened for the chrome not getting registered on the Mac.

Back to Ben.

Comment 50

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

painting problem worksforme, commercial linux installer. 

updating summary. if this still happens, reassign to hyatt, who deals with such 
bugs. 

Comment 51

[Patty Mac]

This bug is fixed in all three platforms (Build: 2000-09-05-08-M18).

Comment 52

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

Comment on attachment 12062 [details] [diff] [review]
additional patch, for extra safety

This patch caused regression bug 199310.  (And that took quite a while to
notice, too. :-)