Closed Bug 278534 Opened 15 years ago Closed 15 years ago

Get rid of contents.rdf chrome registration nightmare.

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: benjamin)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Finally, it's time to get rid of contents.rdf for chrome registration, in favor
of simple text manifests. This will fix a whole series of bugs relating to
chrome registration and the extension manager (see dep list), as well as make
authoring extensions and xulrunner apps less painful.

This includes a backwards-compatibility layer so that existing extensions (and
even the core apps) which use contents.rdf will not notice the change.
Attached patch Checkpoint patch (obsolete) — Splinter Review
This isn't meant for review, though you're welcome to comment on it. There are
various fixes in xpinstall and EM-land that I'm working on, as well as making
the seamonkey chrome registry compile (the nsIXULChromeRegistry install*
methods are moved to nsIChromeRegistrySea).
Will these new-style manifests introduce new features, such as the explicit
specification of a fallback locale, or the definition of locale aliases (see
http://forums.mozillazine.org/viewtopic.php?p=1153000#1153626 )? The former
would definitely be a plus, the latter, while nice, wouldn't be a blocker.
The question of a "fallback" locale is something to think about, but I don't see
how/why it would be specified in the manifest file. Aliases could be handled by
the manifest file, but I'm not sure what the purpose would be.
Well, a specified behaviour for the chrome registry to always fallback to the
first listed locale would be enough.

As for locale aliases, the proposal was to use this for e.g. providing an en-GB
locale for some non-GB locales, but separately providing an en-US one. Norbert
can certainly explain this a bit better; however I'm not sure how the syntax
should look like, or whether it's even doable.
As mentioned in the thread that's linked to in Post#2, there seem to be some
fallback in place already (either that, or my testing procedure is wrong). But
with installed localizations for de-DE, en-US, en-GB & fr-FR, I will get the
German version if I call Firefox with a UILocale of 'de-AT', and I will get the
French version if I call it up with 'fr-CA'...

But - nevertheless - a formal fallback protocol would be good, mainly because,
if I have several language packs installed for one language and several regions
I would want control over which the fallback one would be. I might, for example,
 have fr-FR and fr-CA installed, but since the European French is more common
than the Canadian French I would prefer to fall back to fr-FR if someone comes
in from Belgium, let's say. 
The way the default is specified, could be by either the entry position in the
install.rdf (first <em:locale> for a language is the default), or by a qualifier
in the <em:locale> tag.

The other issue was that it would be useful to have language 'packs', where I
could define (in the language's contents.rdf for example) which regions this
pack is responsible for. That way I could have a single folder for all
South-American Spanish versions, and another one for European Spanish, rather
than having to deal with a dozen identical ones.
Attached patch Checkpoint 2 (obsolete) — Splinter Review
Second checkpoint patch. This is pretty much ready for review, but 1) it's from
an older tree and needs merging 2) it doesn't have the seamonkey changes for
methods moved to nsIChromeRegistrySea
Attachment #171366 - Attachment is obsolete: true
Note to self: test the firefox installer.
dveditz, do you want to review the xpinstall changes yourself, or can I get
mconnor/darin to review them along with everything else?
I know reviewing code in chromereg is a pain; I'm sorry. The most "dangerous"
part of this patch is that nsIXULChromeRegistry.allowScriptsForSkin returns
false for all skin packages.

I have tested seamonkey (basic run-test, and xpinstall).
Firefox (run-test, theme install, extension install, old-style install,
installer).
Attachment #171763 - Attachment is obsolete: true
Attachment #171954 - Flags: second-review?(darin)
Attachment #171954 - Flags: first-review?(mconnor)
Quick note to myself, audit this patch for extraneous printf() and dump()
statements.
Comment on attachment 171954 [details] [diff] [review]
Remove contents.rdf, version 1 (ready for review)

I only reviewed the xpinstall files

>Index: xpinstall/src/nsRegisterItem.cpp
>===================================================================
>+                           NS_LITERAL_CSTRING("resource;///chrome/xpinstall.manifest"));

typo: colon (':') not semi-(';')

>+                rv = reg->ProcessContentsManifest(baseuri, manifesturi,
>+                                                  PR_TRUE,
>+                                                  mChromeType == CHROME_SKIN);

You know it can't be delayed chrome, but there might be other bits set
in the script (PROFILE_CHROME, SELECT_CHROME). 
    (mChromeType & CHROME_ALL) == CHROME_SKIN
might be more what you want.

>+                reg->CheckForNewChrome();

There can't be any errors from CheckForNewChrome?

>+
>+            // app-chrome.manifest is not regenerated if it exists
>+            rv = startupFile->SetNativeLeafName(NS_LITERAL_CSTRING("app-chrome.manifest"));
>+            if (NS_SUCCEEDED(rv))
>+                startupFile->Remove(PR_FALSE);

Should this chunk be #ifdef MOZ_XUL_APP ?

>Index: xpinstall/src/nsSoftwareUpdateRun.cpp
>===================================================================
> static PRInt32
> CanInstallFromExtensionManifest(nsIZipReader* hZip, nsIFile* jarFile, nsIPrincipal* aPrincipal)

You could #ifdef MOZ_XUL_APP this function now

There's a bug in CanInstallFromExtensionManifest() -- it's supposed to return
an nsInstall status code for finalStatus. It mostly does this, but at the end
returns the raw result from hZip->Test(). Instead
1) kill the useless PRInt32 result = NS_OK; at the top
2) change
-   return hZip->Test("install.rdf");
+   if (NS_FAILED(hZip->Test("install.rdf"))
+	return nsInstall::NO_INSTALL_SCRIPT; // as good as any
+   return nsInstall::SUCCESS;

Then ...

>         finalStatus = CanInstallFromExtensionManifest( hZip,
>                                                        jarpath,
>                                                        installInfo->mPrincipal);
>         if (NS_SUCCEEDED(finalStatus)) 

should be "if (finalStatus == nsInstall::SUCCESS)"

else don't use finalStatus and make C.I.F.E.M.() return a consistent nsresult
(possible because any errors caught here get re-caught in
GetInstallScriptFromJarfile()).

>+        if (info->GetType() == CHROME_SKIN) {
>+            static NS_DEFINE_CID(kZipReaderCID,  NS_ZIPREADER_CID);

This one is OK comparing to CHROME_SKIN without the mask. It's only an install
script registerChrome() call that supports any extra flags and we get here from
a web page calling InstallTrigger.installChrome(). If someone adds unsupported
flags there that's their own problem.

>+        if (!installed)
>+            reg->ProcessContentsManifest(info->GetFileJARURL(),
>+                                         info->GetManifestURL(),
>+                                         PR_TRUE,
>+                                         info->GetType() == CHROME_SKIN);

You don't need to call reg->CheckForNewChrome() as in nsRegisterItem ?
I'm going to seriously try to get to this review within the next week sometime.

bsmedberg: can you do me a favor and summarize the changes you are making?  i
understand the high level design from reading the wiki, but i'd like to
understand what each of the code changes is about.  i.e., what are the major
pieces of code that are being deleted?  why are they being deleted?  what is
being moved if anything?  what do i need to be thinking about as a reviewer of
this patch?  what were some of the tricky parts of this patch if any?  are there
any compromises that you had to make that we should know about?  etc.
Comment on attachment 171954 [details] [diff] [review]
Remove contents.rdf, version 1 (ready for review)

>Index: content/base/public/nsIChromeRegistry.idl

>-   * @param aChromeURL the URL that is to be canonified.
>-   */
>-  void canonify(in nsIURI aChromeURL);

This method is an "internal to chromereg" method, that should not be exposed.

>-  AUTF8String convertChromeURL(in nsIURI aChromeURL);
>+  nsIURI convertChromeURL(in nsIURI aChromeURL);

This removes the need for  uri<->string roundtrip, and this method is only used
by the chrome protocol handler.

>-  /* Whether or not an addon package can dynamically overlay another package. 
>-     You can use these APIs to effectively disable a chrome add-on without
>-     uninstalling it. */
>-  void setAllowOverlaysForPackage(in wstring packageName, in boolean allowOverlays);

EM doesn't need this method since it can just remove that extension's manifest
from the list.

>-  /* Installation APIs */

All moved to the seamonkey-specific interface unaltered.

>Index: chrome/public/nsIToolkitChromeRegistry.idl

>+/**
>+ * The chrome registry will ask the directory service for this file list and
>+ * process each file as a manifest. Skin manifests may contain only skin
>+ * and style overlays.
>+ */
>+%{ C++
>+#define NS_CHROME_MANIFESTS_FILE_LIST "ChromeML"
>+#define NS_SKIN_MANIFESTS_FILE_LIST "SkinML"
>+%}

The distinction between "skin" and "regular" manifests allows us to prevent
themes from running scripts, so we can use the less-scary xpinstall dialog.

>Index: chrome/src/nsChromeProtocolHandler.cpp

Most of the changes to this file are removing old #ifdef MOZ_XUL (this file is
only built when XUL is enabled), and updating to the new signatures of
nsChromeRegistry::Canonify and nsChromeChromeRegistry::ConvertChromeURL. There
should be no significant logic changes.

>Index: chrome/src/nsChromeRegistry.cpp

Basically, the entire significant guts of this file are replaced. Any of the
old functions that used RDF are replaced with the new data structures. An
overview of these data structures will significantly help understand the new
code:

nsChromeRegistry::mPackagesHash is a pldhashtable which maps a package name
(e.g. "global") to a
  nsChromeRegistry::PackageEntry type, which contains:
    the base URI for the global/content package.
    a flag which currently only contains a PLATFORM_PACKAGE bit
    two arrays of locale and skin providers for this package; these arrays are
of type
    nsChromeRegistry::nsProviderArray, which is a subclass of nsVoidArray with
a ProviderEntry* in each slot.

nsChromeRegistry::mOverlayHash and nsChromeRegistry::mStyleHash are hashtables
which match a URI
  (e.g. chrome://browser/content/browser.xul) to a list of of XUL overlays and
stylesheets. OverlayListHash
  is like a value-added nsClassHashtable<nsURIHashKey,nsCOMArray<nsIURI> >, but
the entry struct has the COMArray
  inline, instead of allocated separately.

Believe it or not, all the RDF of the chrome registry can be boiled down to
this ;) So you should for the most part ignore the "-" sections, and only
review the "+" sections, with provisos below.

>+static PRBool
>+LanguagesMatch(const nsACString& a, const nsACString& b)

This is just moved, doesn't need review.

>+nsChromeRegistry::ProviderEntry*
>+nsChromeRegistry::nsProviderArray::GetProvider(const nsACString& aPreferred, MatchType aType)

All of these "hashtable helpers" should get careful review. I know they work,
but... it's hashtables.

>+nsresult
>+nsChromeRegistry::GetProviderAndPath(nsIURL* aChromeURL,
>+                                     nsACString& aProvider, nsACString& aPath)

This takes over some of the logic from SplitChromeURL 

>+// xxxbsmedberg Move me to nsIWindowMediator
>+NS_IMETHODIMP
>+nsChromeRegistry::ReloadChrome()

I don't think I changed this, I think "diff" is cofused.

>+nsChromeRegistry::LoadStyleSheetWithURL(nsIURI* aURL, nsICSSStyleSheet** aSheet)

ditto

>+NS_IMETHODIMP
>+nsChromeRegistry::AllowScriptsForSkin(nsIURI* aChromeURI, PRBool *aResult)

This does have one significsnt change: formerly there was a lot of (faulty)
logic to determine which skins were allowed to run scripts. Now no skins are
allowed to run scripts.

Hrm, this textarea doesn't hold enough text, more comments shortly.
Attachment #171954 - Flags: first-review?(mconnor)
+NS_IMETHODIMP
+nsChromeRegistry::ProcessContentsManifest(nsIURI* aOldManifest, nsIURI* aFile,
+                                          PRBool aAppend, PRBool aSkinOnly)

This is a major simplification of the original RDF logic in Install*. At some
point in the past, a single contents.rdf (or manifest.rdf) could register
multiple packages of different types (content/locale/skin), and multiple
providers of each type, in multitudinous combinations. This has long since been
superceded by the common practice of using one contents.rdf per package/provider.

The one special case where we use one contents.rdf to register all the packages
for a single theme. This is handled simply in nsChromeRegistry::ProcessProvider

+      if (count > 1) {
+        line.Append(packageName);
+        line.Append('/');
+      }

nsChromeRegistry::ProcessManifestBuffer

I cannot believe how much fun it is to parse simple text files using nsCRT::strtok.

For xpinstall, the changes are pretty simple.

1) If we're doing a registerChrome() with CHROME_DELAYED, we add the package to
installed-chrome.txt like we currently do, and we delete app-chrome.manifest so
that it will be recreated.

2) If we're doing an immediate registerChrome() or a scripted theme install, we
have a special bin/chrome/xpinstall.manifest to which the chrome registrations
are appended, and we immediately re-process manifests.

Finally, the extension manager changes combine the separate components.ini and
defaults.ini into one extensions.ini, which lists the location of all the
installed extensions (and uses this for extension manifest files as well). Upon
installation, extensions/themes are processed to create a chrome.manifest.
I bet some of #13 and #14 would be good in a comment in the header? At least
the datastructure stuff?

> OverlayListHash is like a value-added 
> nsClassHashtable<nsURIHashKey,nsCOMArray<nsIURI> >, but the entry struct has
> the COMArray inline, instead of allocated separately.

Is this significantly different from a nsDataHashtable? Just poking, didn't
look too closely at the actual code fragments from both.
Comment on attachment 171954 [details] [diff] [review]
Remove contents.rdf, version 1 (ready for review)

>Index: content/base/public/nsIChromeRegistry.idl

>+   * Convert a chrome URL to a "real" using the information in the registry.
>+   * Does not modify aChromeURL.

nit: a "real" what?  URL, right?


>Index: chrome/public/Makefile.in

>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.

really?


>Index: chrome/src/nsChromeProtocolHandler.cpp

>     nsCOMPtr<nsIXULPrototypeCache> cache =
>+             do_GetService(kXULPrototypeCacheCID);

nit: in other places you use "()" instead of "=".. might be good
to make the style consistent in this file / module.


>+    if (cache) {
>+        cache->GetPrototype(aURI, getter_AddRefs(proto));
>+    }
>+    else {
>+        NS_WARNING("Unable to obtain the XUL prototype cache!");
>+    }
...
>         if (! result)
>             return NS_ERROR_OUT_OF_MEMORY;

nit: some brace inconsistency


>+        nsCOMPtr<nsIChromeRegistry> reg (do_GetService(NS_CHROMEREGISTRY_CONTRACTID));
>+        NS_ENSURE_TRUE(reg, NS_ERROR_FAILURE);
...
>         nsCOMPtr<nsIIOService> ioServ(do_GetIOService(&rv));

nit: whitespace inconsistency.


>Index: chrome/src/nsChromeRegistry.cpp

>+nsIURI*
>+nsChromeRegistry::nsProviderArray::GetBase(const nsACString& aPreferred, MatchType aType)
> {
>+  ProviderEntry* provider = GetProvider(aPreferred, aType);
> 
>+  nsCAutoString spec;
>+  if (provider && provider->baseURI)
>+    provider->baseURI->GetSpec(spec);
> 
>+  return provider ? provider->baseURI : nsnull;
>+}

why are you getting the spec from the baseURI here?  i don't see it being
used for anything.  also, it seems that you could code an early return
if !provider to avoid having to null check it twice.


>+  if (provider)
>+    return provider->provider;

this looks funny.  should the member variable have another name?


>+void
>+nsChromeRegistry::nsProviderArray::SetBase(const nsACString& aProvider, nsIURI* aBaseURL)
>+{
>+  ProviderEntry* provider = GetProvider(aProvider, EXACT);
>+
>+  if (provider) {
>+    provider->baseURI = aBaseURL;

no reference counting, really?	what if baseURI is not null?
should we assert?


>+nsChromeRegistry::nsProviderArray::Clear()
...
>+    delete (entry);

micro nit: no need for parentheses.  good to minimize operators IMO.


>+  return & (nsACString&) pentry->package;

how about this instead:

    return (nsACString*) &pentry->package;

though it should be the same thing, c++ compilers are free to 
require a public copy-ctor from typeof(package) to nsACString
when you cast by reference, and they are free to use it though
most do not.  this sort of thing appears elsewhere too.


>+nsChromeRegistry::OverlayListEntry::AddURI(nsIURI* aURI)
...
>+    if (NS_SUCCEEDED(aURI->Equals(mArray.SafeObjectAt(i), &equals)) && equals)

so, elements of mArray can be null?  i.e., it can have holes?
this fact should be documented somewhere if it isn't already.


>+nsChromeRegistry::nsChromeRegistry()
>+{
>+}

nit: how about moving this to the header file, so it can be
inlined away.


> nsresult
> nsChromeRegistry::Init()
> {
>+  nsresult rv;
>+
>   // these atoms appear in almost every chrome registry manifest.rdf
>   // in some form or another. making static atoms prevents the atoms
>   // from constantly being created/destroyed during parsing

"manifest.rdf" or "contents.rdf"?  i seem to recall that there was
some code for handling an old manifest file format.  is that what
this is?  should we preserve it?


>+nsChromeRegistry::GetProviderAndPath(nsIURL* aChromeURL,
...
>+      aPath.Assign(nsDependentCSubstring(path, slash + 1));
...
>+  aProvider.Assign(nsDependentCSubstring(path, 1, slash));

nit: avoiding constructing nsDependentCSubstring if
not needed.  use the form of Assign that takes two
parameter (char pointer and length).


>+nsresult
>+nsChromeRegistry::Canonify(nsIURL* aChromeURL)
> {
>+    nsCAutoString package;
>+    rv = aChromeURL->GetHostPort(package);
>+    NS_ENSURE_SUCCESS(rv, rv);

nit: I would just call GetHost instead of GetHostPort since you don't
really expect there to be a port value.  It doesn't really change
anything, but I think it'd be more clear to someone reading this code.


>+    else {
>+      // XXXbsmedberg: report this to the console service?
>+      return NS_ERROR_INVALID_ARG;

yeah, good idea :)


>+nsChromeRegistry::ConvertChromeURL(nsIURI* aChromeURI, nsIURI* *aResult)
...
>+  nsCOMPtr<nsIURL> chromeURL (do_QueryInterface(aChromeURI));

nit: keep nsCOMPtr initialization style consistent.


>+  obsSvc->NotifyObservers((nsIChromeRegistry*) this,

nit: i'm not a big stickler about c++ style casts, but it seems
you have used them in other places, so might as well be consistent.


>+nsChromeRegistry::CheckForNewChrome()
...
>+  // this is the main manifest; if it doesn't exist we generate it from
>+  // installed-chrome.txt. When the build system learns about the new system,
>+  // this code can go away.

XXX we may need to worry about installers that place JAR packages in chrome/
and edit installed-chrome.txt to get their extensions registered.  i know
these installers are to be discouraged, but for a variety of reasons EM is
not sufficient for some cases.	e.g., --install-global-extension is not very
useful to folks building global extensions since there is no corresponding
--uninstall-global-extension.  thus, it is very likely that people will choose
to hand install their chrome packages.	it's a mess, but i hope we can support
it for a while still.  however, it might be very difficult to do what i am
suggesting.  maybe it isn't even worth it :(


>+  result.Assign(NS_ConvertUTF16toUTF8(value));

nit: use CopyUTF16toUTF8 instead to avoid intermediate buffer copy.


>+nsChromeRegistry::ProcessContentsManifest(nsIURI* aOldManifest, nsIURI* aFile,

nit: this function is rather large.. maybe you could break it up?


>+  PRFileDesc* fd;

nit: lot's of indenting in this function because of this guy.
stack based auto PR_Close?  or use goto to avoid indenting?


>+        name.AppendLiteral(PLATFORM_N);

nit: use NS_LINEBREAK instead (defined in nsCRT.h)?


>+GetLiteralText(nsIRDFLiteral* lit, nsACString& result)
...
>+  result.Assign(NS_ConvertUTF16toUTF8(value));

nit: use CopyUTF16toUTF8 instead


>+      nsCAutoString overlayName;
>+      GetLiteralText(overlay, overlayName);
>+
>+      overlayName.Insert('\t', 0);
>+      overlayName.Insert(overlaidName, 0);
>+      overlayName.Insert('\t', 0);
>+      overlayName.Insert(aType, 0);
>+      overlayName.Append('\n');

this looks like a pretty costly way to build up this string.
might be a good candidate for operator+ instead :)


>+const char kWhitespace[] = "\t ";
>+const char kNewlines[]   = "\r\n";

nit: mark these static


>+      PRBool platform = PR_FALSE;
>+      while (nsnull != (token = nsCRT::strtok(whitespace, kWhitespace, &whitespace))) {
>+        if (!strcmp(token, "platform"))
>+          platform = PR_TRUE;
>       }
...
>+      if (platform)
>+        entry->flags |= PackageEntry::PLATFORM_PACKAGE;

what is this "platform" thing?	i don't see it mentioned on the wiki page.
maybe it's just an old concept that i'm not familiar with?


>+      rv |= io->NewURI(nsDependentCString(overlay), nsnull, nsnull,
>+                       getter_AddRefs(overlayuri));
>+      if (NS_FAILED(rv))
>+        continue;

nit: output warning in this failure case too (same with other continue
statements due to error in this loop)


>Index: chrome/src/nsChromeRegistry.h

>+    enum MatchType {
>+      EXACT = 0,
>+      LOCALE = 1,
>+      SKIN = 2
>+    };

nit: provide a quick comment about what these mean.


>+  struct PackageEntry : public PLDHashEntryHdr
>+  {
>+    PackageEntry(const nsACString& package);
>+    ~PackageEntry() { }
> 
>+    enum {
>+      PLATFORM_PACKAGE = 1 << 0
>+    };

nit: document me :)



>+    void AddURI(nsIURI* aURI);

if this function has interesting reference counting semantics,
please document those.	same with other nsIURI-taking functions.


>Index: xpinstall/src/nsInstall.cpp

> {
>     MOZ_COUNT_CTOR(nsInstallInfo);
>+
>+    nsresult rv;
>+
>+    // Failure is an option, and will occur in the stub installer.
>+
>+    NS_WITH_ALWAYS_PROXIED_SERVICE(CHROMEREG_IFACE, cr,
>+                                   NS_CHROMEREGISTRY_CONTRACTID,
>+                                   NS_UI_THREAD_EVENTQ, &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+      mChromeRegistry = cr;
>+
>+      nsCAutoString spec;
>+      rv = NS_GetURLSpecFromFile(aFile, spec);
>+      if (NS_SUCCEEDED(rv)) {
>+        spec.Insert(NS_LITERAL_CSTRING("jar:"), 0);
>+        spec.AppendLiteral("!/");
>+#ifdef MOZ_XUL_APP
>+        NS_NewURI(getter_AddRefs(mFileJARURL), spec);

necko is not threadsafe... and neither is the chrome registry,
yet this code wants to build a proxy to the chrome registry.
is that because you think this code may run on a background
thread?  if so, then we have necko threadsafety problems.

maybe the old code was thinking it was threadsafe, yet never
actually used from other than the main thread?	premature
multithreading is a pervasive problem in our codebase... one
of those things i try to cleanup when i have the chance.


>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in

>+      dump ("Not in safe mode\n");
>       var wasInSafeModeFile = getFile(KEY_PROFILEDIR, [DIR_EXTENSIONS, FILE_WASINSAFEMODE]);
>       if (wasInSafeModeFile.exists()) {
>+        dump("Was in safe mode.\n");

are these dump statements meant to be be here still?


overall, i'm very happy with this patch.  i think this will
be a great improvement to the chrome registration process
and will greatly simplify XUL development.  nice work!

i think you should ask dveditz for review on the xpinstall
changes and i think you should get ben to look over your
changes to EM at least.


r=darin with nits picked.
Attachment #171954 - Flags: second-review?(darin) → second-review+
> >+nsChromeRegistry::nsProviderArray::SetBase(const nsACString& aProvider, 
[snip]
> >+    provider->baseURI = aBaseURL;
> 
> no reference counting, really?	what if baseURI is not null?
> should we assert?

It's a comptr, I can assign into it without thinking ;)

> so, elements of mArray can be null?  i.e., it can have holes?

oops, no. I confused nsVoidArray.FastElementAt and nsCOMArray.SafeElementAt

> "manifest.rdf" or "contents.rdf"?  i seem to recall that there was
> some code for handling an old manifest file format.  is that what

I have completely dropped support for manifest.rdf files, which I have never
encountered. I will update the comment.

> >+      // XXXbsmedberg: report this to the console service?
> >+      return NS_ERROR_INVALID_ARG;
> 
> yeah, good idea :)

I plan to write a secondary patch which will report all kinds of
extension/chrome/startup errors to the console service and optionally log the
console service to a file. Hopefully in time for 1.8.

> XXX we may need to worry about installers that place JAR packages in chrome/
> and edit installed-chrome.txt to get their extensions registered.  i know

I don't want to do that, just for complexity's sake. They can either use the
xpinstall registerChrome() method, or install their own chrome/foo.manifest file
(which I encourage as the future correct solution).

> this looks like a pretty costly way to build up this string.
> might be a good candidate for operator+ instead :)

I thought that in the new string world without dependent concatenations,
operator+ was just as expensive as manual append/insert...

> what is this "platform" thing?	i don't see it mentioned on the wiki page.
> maybe it's just an old concept that i'm not familiar with?

Tt is a "modifier" that I added on top of the wiki spec (the parser is
forward-compatible enough that we might add other modifiers in the future). It
implements the old chrome:platformPackage arc logic, separating
chrome://global-platform/locale/ into win/ unix/ mac/ subdirectories.

> >+    void AddURI(nsIURI* aURI);
> 
> if this function has interesting reference counting semantics,
> please document those.	same with other nsIURI-taking functions.

Nope, member COMPtr's rule the world.

> necko is not threadsafe... and neither is the chrome registry,
> yet this code wants to build a proxy to the chrome registry.
> is that because you think this code may run on a background
> thread?  if so, then we have necko threadsafety problems.

The actual install funtions run on a backround thread, but proxy every single
call back to the main thread. I'm surprised you never encountered any of the
xpinstall threading bugs (do a quick search, you will be unpleasantly
surprised). That's why I'm doing this necko logic here on the main thread,
instead of later.

> i think you should ask dveditz for review on the xpinstall
> changes and i think you should get ben to look over your
> changes to EM at least.

Already done.

It looks unlikely, though vaguely possible, that this will land tonight. I will
need to spin another round of smoketest builds. If not, first thing in the beta2
 cycle. Thanks for going through this rather unpleasant patch!
> > this looks like a pretty costly way to build up this string.
> > might be a good candidate for operator+ instead :)
>
> I thought that in the new string world without dependent concatenations,
> operator+ was just as expensive as manual append/insert...

No way! ;-)  Take a look at nsSubstringTuple.  In the new world, methods like
Assign and Insert are overloaded to take nsSubstringTuple.  That class is
automatically constructed on the stack whenever you use operator+.  It is more
efficient now then before when it was a subclass of nsAString.


> Nope, member COMPtr's rule the world.

Yes, they do!  I need to remember to read header files ;-)


> It looks unlikely, though vaguely possible, that this will land tonight.

Go go gadget bsmedberg!!! :-)
*** Bug 210838 has been marked as a duplicate of this bug. ***
This is the final patch committed.
Attachment #171954 - Attachment is obsolete: true
Fixed on trunk. I'm hoping to have documentation up within the hour, I'm looking
for a good place on the website.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
dammit. I had the following change in my tree, but forgot to include it in the
path. bz, can you post-review this? The functional result is the same, since
previously chromeregistry->AllowScriptsForSkin returned true for any non-chrome URI.

--- content/xbl/src/nsXBLDocumentInfo.cpp       31 Jul 2004 23:15:00 -0000     1.24
+++ content/xbl/src/nsXBLDocumentInfo.cpp       19 Feb 2005 16:35:57 -0000
@@ -328,15 +328,13 @@

   return document->GetPrincipal();
 }

-static PRBool IsChromeOrResourceURI(nsIURI* aURI)
+static PRBool IsChromeURI(nsIURI* aURI)
 {
   PRBool isChrome = PR_FALSE;
-  PRBool isResource = PR_FALSE;
-  if (NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) &&
-      NS_SUCCEEDED(aURI->SchemeIs("resource", &isResource)))
-      return (isChrome || isResource);
+  if (NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)))
+      return isChrome;
   return PR_FALSE;
 }

 /* Implementation file */
@@ -347,9 +345,9 @@
     mScriptAccess(PR_TRUE),
     mBindingTable(nsnull)
 {
   nsIURI* uri = aDocument->GetDocumentURI();
-  if (IsChromeOrResourceURI(uri)) {
+  if (IsChromeURI(uri)) {
     // Cache whether or not this chrome XBL can execute scripts.
     nsCOMPtr<nsIXULChromeRegistry>
reg(do_GetService(NS_CHROMEREGISTRY_CONTRACTID));
     if (reg) {
       PRBool allow = PR_TRUE;
Basic documentation up at
http://www.mozilla.org/xpfe/ConfigChromeSpec.html
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050219
Firefox/1.0+ hourly beast build

All extensions stopped working

They show as enabled in EM, but everything else is gone (extension menu's
buttons etc)

Do we need to reinstall them or is there an easy workaround ?

Not sure if this needs reopened, new bug or what ?
Hrm, let me ask Ben. Reinstall should fix things; I was relying on the fact that
the app version number was being upgraded from 1.0 to 1.1, which would force
extension reinstall, but I probably need to add some manual logic to aid nightly
users.
I manually set my "maxversion=1.1" right after 1.0 was released.
That doesn't fix it
I reset all "Maxversions=1.0" in Extensions.rdf and tried
It made no difference.
The still show as enabled in EM, but down't show up in menu's/other places
close/restart didn't change things
Backed out, due to tbox orange and GTK assertions on the GTK1 build.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Umm...  No, that's not an acceptable change (the whole thing, not just the
XBLDocumentInfo bit).  This is a scriptable interface, completely undocumented,
and you're asserting something about the input?  Especially in a
security-related method?  I'm sorry, but that's just not acceptable.  Either
check the scheme in allowScriptsForSkin, or check it in GetProviderAndPath.  In
fact, I'd prefer that this were done and XBL not do a scheme check at all.

Further, this patch makes the two allowScriptsForSkin methods (in the two chrome
registries) have different error semantics (eg one will set aResult to "false"
on error, and one will set it to "true"); furthermore, since the errors that can
happen vary from "not a chrome URI" to "chrome URI but we ran into some other
issue", XBL itself can't set the boolean on error (because those cases need to
be different from a security perspective).

What I would prefer to see is that both allowScriptsForSkin methods should
return true and not throw on non-chrome URIs, and throw errors thereafter.  Then
the calling code can just pass in the URI (without checking), and if an error
comes back assume scripts are not allowed.

All of which should be documented in the interface in some reasonable way, of
course.

Oh, and how about documenting GetProviderAndPath() in the header?  And maybe
even document the other functions you're adding?
One other point on asserts like this one.  Once asserts are fatal, I really
shouldn't be able to trigger them from JS while following the interface comments...
Boris, I would like make both chrome registries behave the same way, and
document that the "allowScriptsForSkin" method should only be called for chrome
URIs (and add assertions to that affect). I originally wanted to only support
chrome://*/skin/ URIs, but that would place unnecessary burden on callers to
parse the URI. We shouldn't be asking the chrome registry about pseudo-security
information on non-chrome (resource) URIs, it doesn't make any sense.
Fixed on trunk, for real this time. I had added static NS_NAMED_LITERAL_CSTRINGs
to help make nsDependentConcatenations, but those are verboten because they have
constructors/destructors.

In addition, I fixed bug 282858 with the second checkin, see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.84&root=/cvsroot&mark=1815-1873#1812

Further changes r=darin over IRC
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
I'm not sure if this should go in this bug or in bug 282858, but using a build I
just pulled, it seems that something is going horribly wrong with my existing
extensions, namely ForecastFox and Duplicate Tab.

Build ID: Mozilla/5.0 (Windows; compatible; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050222 Firefox/1.0+

Preferences in either of these extensions will not save, and they are not
remembered upon closing and reopening of FF.  I tried this with a new profile,
and fresh installs of the extensions.  Something must be going wrong with the
conversion to the chrome.manifest files.  Any ideas?
(In reply to comment #33)
> I'm not sure if this should go in this bug or in bug 282858, but using a build I
> just pulled, it seems that something is going horribly wrong with my existing
> extensions, namely ForecastFox and Duplicate Tab.
> 
> Build ID: Mozilla/5.0 (Windows; compatible; U; Windows NT 5.1; en-US; rv:1.8b2)
> Gecko/20050222 Firefox/1.0+
> 
> Preferences in either of these extensions will not save, and they are not
> remembered upon closing and reopening of FF.  I tried this with a new profile,
> and fresh installs of the extensions.  Something must be going wrong with the
> conversion to the chrome.manifest files.  Any ideas?

Forecastfox has been working on and off for me over the past few days as I
created new builds. As of now, when I install it on a fresh profile, the options
dialog is broken badly. The OK button doesn't work (only cancel), the dropdowns
for icon location are empty, and so forth. I'm not sure if it's something that
needs to be fixed on the Firefox end or the Forecastfox end, which is why I've
held off on saying anything up until this point.
Ryan, feel free to reopen bug 282858. Please provide as much info as possible,
such as any output to the console, whether there are chrome.manifest files for
each extension, and any suspicious chrome.manifest entries, or missing entries.
I should have clarified my first post.  This problem is with ForecastFox 0.5.9
(the latest official release), and Duplicate Tab 0.5.1.  There was no problem
with yesterday's nightly, nor at any time previously.  This just started when I
installed a build from this evening (PST).
Could this have increased leaks on balsa by 30KB or so, looks like (seamonkey
builds were not affected by the leak increase, hence the guess at this bug).
Leak fix committed (I forgot to finish mPackagesHash in the destructor).
Blocks: 283352
Just a quick question. How come when installing themes a copy of the jar file
ends up under the chrome directory in the profile. This seems unecessary when
the extensions.ini file refers to the copy under the extensions folder.
As I understand it, existing installations of extensions are not converted, so
you will see your extensions "lost" (menus under Tools, in Preferences). If that
should stand so this need a hint in the release notes ("reinstall all
extensions"). I did run into this after upgrade from build 02-21-04 to 02-23-05
today with Mozilla Suite 1.8b2.
> As I understand it, existing installations of extensions are not converted, so
> you will see your extensions "lost" (menus under Tools, in Preferences). If that

That is incorrect... extensions should be upgraded to the new format automatically.
(In reply to comment #41)
> > As I understand it, existing installations of extensions are not converted, so
> > you will see your extensions "lost" (menus under Tools, in Preferences).
> 
> That is incorrect... extensions should be upgraded to the new format
automatically.

It seems it doesn't work reliably (at least not for my profile). See also Bug
283454.
filed Bug 283470 to fix xulrunner simple example
My system appeared to go into a loop during the first run. I looked in taskmgr
and found a second firefox.exe process repeatedly starting and stopping. I then
looked in the profile dir and foud that compreg.dat was being repeatedly
recreated. I had a backup of my profile so I deleted the current one and
replaced it with the backup. I then looked in components.ini and found a
reference to a components directory that still existed but should have been
deleted since the extension had been uninstalled. After removing the reference
to this in components.ini and deleted the entire extension dir that had been
installed it started fine on first run.
I asked this per e-mail but I'm still waiting on a repy, so that's why I'll ask
it here:

Benjamin, I'm used to use setAllowOverlaysForPackage() but you seem to have
removed it, so tell me, what do we have to use now instead?
HJ, are you talking about seamonkey or toolkit?
Is there any details on how extensions should now be packaged with this patch in
place? I tried with a manifest file and no contents.rdf files but that doesn't
appear to work.
Dave, this patch actually does not change the way extensions should be packaged
(yet). See bug 283352 for the bit of extra work needed for the extension manager
to use flat-chrome manifests pre-provided by extension authors. I will document
that and announce it loudly when it lands.
(In reply to comment #47)
> HJ, are you talking about seamonkey or toolkit?

Seamonkey it is, and I replaced Components.interfaces.nsIXULChromeRegistry with
Components.interfaces.nsIChromeRegistrySea after someone notified me about this
being broken in MultiZilla, so no worries, thanks ;)
So... this broke the tinderstatus extension (which is somewhat unfortunate for
dogfood use here; I've reverted my build to one that does show me tinderstatus
for the time being).  I assume that's because of the overlay change?  Was this
desirable?  If so, what can I do to make the extension work again in my build?  

Also, were extension authors notified of this change in any way?  Will they be?
bz,

Are you talking about a firefox extension or a seamonkey extension?  This patch
was designed to work well for firefox extensions without modification to those
extensions.  I'm not sure how valuable it is to support extensions that do not
use install.rdf (assuming that's what you're talking about).
SeaMonkey extension, naturally.

I'm not sure what about this patch broke that extension exactly (whether it was
the chrome registry api changes mentioned in comment 50 or something else). 
What could I do to test?

Note that the tinderstatus extension is at http://tinderstatus.mozdev.org/ -- it
seems to have an install.js but no install.rdf.  But the extension was already
installed in this case.
> I'm not sure what about this patch broke that extension exactly (whether it was
> the chrome registry api changes mentioned in comment 50 or something else). 
> What could I do to test?

Well, I'd start by seeing what overlay files have been touched by the extension,
and I'd probably also unzip the JAR file to see what it is using.  Does it use
the chrome registry API directly somehow?


> it seems to have an install.js but no install.rdf.  But the extension was 
> already installed in this case.

Maintaining support for old-style XPIs that use unfrozen APIs (meaning XUL) is a
real pain.  I frankly don't see the point, and I don't think we can manage it. 
install.rdf gives extension authors the ability to say which versions of the
application they support, and as we are seeing this solution works quite well
(given a functioning update.mozilla.org).

I recently had to help someone cleanup some mess in their Firefox profile that
was produced by an extension that manually installed stuff into various dark
corners.  If we care about seamonkey and extensions, then I think we must move
seamonkey to the Extension Manager (install.rdf) framework.  I think we should
warn users before letting install.js run (because it may not be something they
can easily undo, and it may also hinder their ability to upgrade the browser).
It doesn't seem to use the chrome registry api.

All the extension is is a single overlay file that overlays the icon into the
status bar, an entry in contents.rdf that says:

  <!-- overlay for Netscape and Mozilla, whose status bar icons appear on the
right -->
  <RDF:Seq about="chrome://navigator/content/navigator.xul">
    <RDF:li>chrome://tinderstatus/content/tinderstatusOverlay.xul</RDF:li>
  </RDF:Seq>

(and similar for Firefox), and some JS pulled in by the overlay.  It looks like
the contents.rdf entry is just not being used anymore...

Blocks: 285063
The new code doesn't report errors in chrome.manifest. I filed bug 285063 for that.

Also is bug 190195 fixed by this or is RDF still used under the hood?
1) Is tinderstatus broken in seamonkey, or just toolkit apps?
2) Are new installations of tinderstatus broken, or just current installations?

I was able to do a new install of tinderstatus into Firefox quite successfully.
I have no intention of supporting existing installs of install.js-style extensions.
> 1) Is tinderstatus broken in seamonkey, or just toolkit apps?

Seamonkey.  See comment 53.  I haven't tested toolkit apps, so I have no idea
whether it's broken there.

> 2) Are new installations of tinderstatus broken, or just current installations?

Hmm... Just current installations.  Reinstalling it seems to work.  So sounds
like this was an expected effect of the patch, then?
> Hmm... Just current installations.  Reinstalling it seems to work.  So sounds
> like this was an expected effect of the patch, then?

Actually, no. I didn't intend to make any functional changes to the seamonkey
side at all. Tinderstatus does a profile install; I can't see an obvious reason
why this patch would have affected it at all. If you think it's important, can
you file a followup bug, and I will do some debug pokery?
*** Bug 245731 has been marked as a duplicate of this bug. ***
Blocks: 288984
(In reply to comment #61)
> *** Bug 245731 has been marked as a duplicate of this bug. ***

Was this after the fix was checked in?
This is a great improvement, but as a consequence it seems like
nsChromeUIDataSource has been removed from the build (presumably because the
implementation depended on chrome registration information being fed in as RDF).
So the "rdf:chrome" data source no longer exists, and I can't see any way to
access much of the information in the chrome registry (e.g. the list of
installed packages).

Are there plans to provide some other path to this information now that
rdf:chrome is gone, or to put back rdf:chrome with a new implementation? I
looked pretty hard in both the sources and here and wasn't able to find any info
about this.
I have no plans to put back the rdf:chrome datasource in any form. Could you
please indicate exactly what information you actually need from the chrome
registry, and why? You may email me privately to avoid bugspam, or corner me on
irc.mozilla.org#developers (bsmedberg).
Flags: in-testsuite?
Component: XRE Startup → Startup and Profile System
QA Contact: nobody → startup
You need to log in before you can comment on or make changes to this bug.