Fix and test logging of manifest parsing and registration. I discovered that the outer loop doesn't track line numbers correctly when there are multiple newlines in a row, which requires manual looping instead of using nsCRT::strtok.
authorBenjamin Smedberg <benjamin@smedbergs.us>
Mon, 28 Jun 2010 13:55:57 -0400
changeset 47078 5229353383b0eac753fdbf350bafa3f9eb21cccf
parent 47077 74065033cb802d40a6e3fb76845abc38b328485b
child 47079 0eedc6f19e6bbb74d8afe1f5406490feaba400ae
push idunknown
push userunknown
push dateunknown
milestone1.9.3a6pre
Fix and test logging of manifest parsing and registration. I discovered that the outer loop doesn't track line numbers correctly when there are multiple newlines in a row, which requires manual looping instead of using nsCRT::strtok.
xpcom/components/ManifestParser.cpp
xpcom/components/ManifestParser.h
xpcom/components/nsComponentManager.cpp
xpcom/components/nsComponentManager.h
xpcom/tests/unit/compmgr_warnings.manifest
xpcom/tests/unit/test_compmgr_warnings.js
--- a/xpcom/components/ManifestParser.cpp
+++ b/xpcom/components/ManifestParser.cpp
@@ -46,16 +46,17 @@
 #elif defined(XP_MACOSX)
 #include <CoreServices/CoreServices.h>
 #elif defined(MOZ_WIDGET_GTK2)
 #include <gtk/gtk.h>
 #endif
 
 #include "mozilla/Services.h"
 
+#include "nsConsoleMessage.h"
 #include "nsTextFormatter.h"
 #include "nsUnicharUtils.h"
 #include "nsVersionComparator.h"
 #include "nsXPCOMCIDInternal.h"
 
 #include "nsIConsoleService.h"
 #include "nsIScriptError.h"
 #include "nsIXULAppInfo.h"
@@ -112,40 +113,88 @@ static const ManifestDirective kParsingT
     NULL, &nsChromeRegistry::ManifestOverride },
   { "resource",         2, true, true,  false,
     NULL, &nsChromeRegistry::ManifestResource }
 };
 
 static const char kWhitespace[] = "\t ";
 static const char kNewlines[]   = "\r\n";
 
-static void LogMessageWithContext(nsILocalFile* aFile, PRUint32 aLineNumber, const char* aMsg, ...)
+namespace {
+struct AutoPR_smprintf_free
+{
+  AutoPR_smprintf_free(char* buf)
+    : mBuf(buf)
+  {
+  }
+
+  ~AutoPR_smprintf_free()
+  {
+    if (mBuf)
+      PR_smprintf_free(mBuf);
+  }
+
+  operator char*() const {
+    return mBuf;
+  }
+
+  char* mBuf;
+};
+
+} // anonymous namespace
+
+void LogMessage(const char* aMsg, ...)
 {
   nsCOMPtr<nsIConsoleService> console =
     do_GetService(NS_CONSOLESERVICE_CONTRACTID);
-  nsCOMPtr<nsIScriptError> error =
-    do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
-  if (!console || !error)
+  if (!console)
     return;
 
   va_list args;
   va_start(args, aMsg);
-  char* formatted = PR_vsmprintf(aMsg, args);
+  AutoPR_smprintf_free formatted(PR_vsmprintf(aMsg, args));
+  va_end(args);
+
+  nsCOMPtr<nsIConsoleMessage> error =
+    new nsConsoleMessage(NS_ConvertUTF8toUTF16(formatted).get());
+  console->LogMessage(error);
+}
+
+void LogMessageWithContext(nsILocalFile* aFile, PRUint32 aLineNumber, const char* aMsg, ...)
+{
+  va_list args;
+  va_start(args, aMsg);
+  AutoPR_smprintf_free formatted(PR_vsmprintf(aMsg, args));
   va_end(args);
   if (!formatted)
     return;
 
   nsString file;
   aFile->GetPath(file);
 
+  nsCOMPtr<nsIScriptError> error =
+    do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
+  if (!error) {
+    // This can happen early in component registration. Fall back to a
+    // generic console message.
+    LogMessage("Warning: in file '%s', line %i: %s",
+               NS_ConvertUTF16toUTF8(file).get(),
+               aLineNumber, (char*) formatted);
+    return;
+  }
+
+  nsCOMPtr<nsIConsoleService> console =
+    do_GetService(NS_CONSOLESERVICE_CONTRACTID);
+  if (!console)
+    return;
+
   nsresult rv = error->Init(NS_ConvertUTF8toUTF16(formatted).get(),
 			    file.get(), NULL,
 			    aLineNumber, 0, nsIScriptError::warningFlag,
 			    "chrome registration");
-  PR_smprintf_free(formatted);
   if (NS_FAILED(rv))
     return;
 
   console->LogMessage(error);
 }
 
 /**
  * Check for a modifier flag of the following forms:
@@ -422,17 +471,32 @@ ParseManifest(NSLocationType aType, nsIL
   // at the end.
   nsTArray<CachedDirective> contracts;
 
   char *token;
   char *newline = buf;
   PRUint32 line = 0;
 
   // outer loop tokenizes by newline
-  while (nsnull != (token = nsCRT::strtok(newline, kNewlines, &newline))) {
+  while (*newline) {
+    while (*newline && '\n' == *newline) {
+      ++newline;
+      ++line;
+    }
+    if (!*newline)
+      break;
+
+    token = newline;
+    while (*newline && '\n' != *newline)
+      ++newline;
+
+    if (*newline) {
+      *newline = '\0';
+      ++newline;
+    }
     ++line;
 
     if (*token == '#') // ignore lines that begin with # as comments
       continue;
 
     char *whitespace = token;
     token = nsCRT::strtok(whitespace, kWhitespace, &whitespace);
     if (!token) continue;
--- a/xpcom/components/ManifestParser.h
+++ b/xpcom/components/ManifestParser.h
@@ -41,9 +41,13 @@
 #include "nsComponentManager.h"
 #include "nsChromeRegistry.h"
 
 class nsILocalFile;
 
 void ParseManifest(NSLocationType type, nsILocalFile* file, char* buf,
                    bool aChromeOnly);
 
+void LogMessage(const char* aMsg, ...);
+
+void LogMessageWithContext(nsILocalFile* aFile, PRUint32 aLineNumber, const char* aMsg, ...);
+
 #endif // ManifestParser_h
--- a/xpcom/components/nsComponentManager.cpp
+++ b/xpcom/components/nsComponentManager.cpp
@@ -454,33 +454,53 @@ void
 nsComponentManagerImpl::RegisterCIDEntry(const mozilla::Module::CIDEntry* aEntry,
                                          KnownModule* aModule)
 {
     PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mMon);
 
     nsFactoryEntry* f = mFactories.Get(*aEntry->cid);
     if (f) {
         NS_ERROR("Re-registering a CID?");
-        // XXX REPORT ERROR, we don't override CIDs
+
+        char idstr[NSID_LENGTH];
+        aEntry->cid->ToProvidedString(idstr);
+
+        nsCString existing;
+        if (f->mModule)
+            existing = f->mModule->Description();
+        else
+            existing = "<unknown module>";
+
+        LogMessage("While registering XPCOM module, trying to re-register CID '%s' already registered by %s.",
+                   aModule->Description().get(),
+                   idstr,
+                   existing.get());
         return;
     }
 
     f = new nsFactoryEntry(aEntry, aModule);
     mFactories.Put(*aEntry->cid, f);
 }
 
 void
 nsComponentManagerImpl::RegisterContractID(const mozilla::Module::ContractIDEntry* aEntry)
 {
     PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mMon);
 
     nsFactoryEntry* f = mFactories.Get(*aEntry->cid);
     if (!f) {
         NS_ERROR("No CID found when attempting to map contract ID");
-        // XXX REPORT ERROR, CID isn't registered
+
+        char idstr[NSID_LENGTH];
+        aEntry->cid->ToProvidedString(idstr);
+
+        LogMessage("Could not map contract ID '%s' to CID %s because no implementation of the CID is registered.",
+                   aEntry->contractid,
+                   idstr);
+                   
         return;
     }
 
     mContractIDs.Put(nsDependentCString(aEntry->contractid), f);
 }
 
 void
 nsComponentManagerImpl::RegisterLocation(NSLocationType aType,
@@ -644,20 +664,19 @@ nsComponentManagerImpl::ManifestBinaryCo
 
     nsresult rv = clfile->AppendRelativeNativePath(nsDependentCString(file));
     if (NS_FAILED(rv)) {
         NS_WARNING("Couldn't append relative path?");
         return;
     }
 
     const mozilla::Module* m = mNativeModuleLoader.LoadModule(clfile);
-    if (!m) {
-        // XXX report load error
+    if (!m)
         return;
-    }
+
     RegisterModule(m, clfile);
 }
 
 void
 nsComponentManagerImpl::ManifestXPT(ManifestProcessingContext& cx, int lineno, char *const * argv)
 {
     char* file = argv[0];
 
@@ -686,24 +705,35 @@ nsComponentManagerImpl::ManifestComponen
     char* file = argv[1];
 
 #ifdef TRANSLATE_SLASHES
     TranslateSlashes(file);
 #endif
 
     nsID cid;
     if (!cid.Parse(id)) {
-        // XXX report parse error
+        LogMessageWithContext(cx.mFile, lineno, "Malformed CID: '%s'.", id);
         return;
     }
 
     nsAutoMonitor mon(mMon);
     nsFactoryEntry* f = mFactories.Get(cid);
     if (f) {
-        // XXX report double-register error
+        char idstr[NSID_LENGTH];
+        cid.ToProvidedString(idstr);
+
+        nsCString existing;
+        if (f->mModule)
+            existing = f->mModule->Description();
+        else
+            existing = "<unknown module>";
+
+        LogMessageWithContext(cx.mFile, lineno, "Trying to re-register CID '%s' already registered by %s.",
+                              idstr,
+                              existing.get());
         return;
     }
 
     nsCOMPtr<nsIFile> cfile;
     cx.mFile->GetParent(getter_AddRefs(cfile));
     nsCOMPtr<nsILocalFile> clfile = do_QueryInterface(cfile);
 
     nsresult rv = clfile->AppendRelativeNativePath(nsDependentCString(file));
@@ -736,24 +766,25 @@ nsComponentManagerImpl::ManifestComponen
 void
 nsComponentManagerImpl::ManifestContract(ManifestProcessingContext& cx, int lineno, char *const * argv)
 {
     char* contract = argv[0];
     char* id = argv[1];
 
     nsID cid;
     if (!cid.Parse(id)) {
-        // XXX report parse error
+        LogMessageWithContext(cx.mFile, lineno, "Malformed CID: '%s'.", id);
         return;
     }
 
     nsAutoMonitor mon(mMon);
     nsFactoryEntry* f = mFactories.Get(cid);
     if (!f) {
-        // XXX report unregistered CID
+        LogMessageWithContext(cx.mFile, lineno, "Could not map contract ID '%s' to CID %s because no implementation of the CID is registered.",
+                              contract, id);
         return;
     }
 
     mContractIDs.Put(nsDependentCString(contract), f);
 }
 
 void
 nsComponentManagerImpl::ManifestCategory(ManifestProcessingContext& cx, int lineno, char *const * argv)
@@ -809,16 +840,27 @@ nsComponentManagerImpl::KnownModule::Loa
                 return rv;
             }
         }
         mLoaded = true;
     }
     return true;
 }
 
+nsCString
+nsComponentManagerImpl::KnownModule::Description() const
+{
+    nsCString s;
+    if (mFile)
+        mFile->GetNativePath(s);
+    else
+        s = "<static module>";
+    return s;
+}
+
 nsresult nsComponentManagerImpl::Shutdown(void)
 {
     NS_TIME_FUNCTION;
 
     PR_ASSERT(NORMAL == mStatus);
 
     mStatus = SHUTDOWN_IN_PROGRESS;
 
--- a/xpcom/components/nsComponentManager.h
+++ b/xpcom/components/nsComponentManager.h
@@ -201,16 +201,22 @@ public:
         bool EnsureLoader();
         bool Load();
 
         const mozilla::Module* Module() const
         {
             return mModule;
         }
 
+        /**
+         * For error logging, get a description of this module, either the
+         * file path, or <static module>.
+         */
+        nsCString Description() const;
+
     private:
         const mozilla::Module* mModule;
         nsCOMPtr<nsILocalFile> mFile;
         nsCOMPtr<mozilla::ModuleLoader> mLoader;
         bool mLoaded;
         bool mFailed;
     };
 
new file mode 100644
--- /dev/null
+++ b/xpcom/tests/unit/compmgr_warnings.manifest
@@ -0,0 +1,9 @@
+# The following line is malformed (mismatched braces)
+component {94b346d7-0cde-4e6e-b819-95d6f200bbf6 MyComponent.js
+
+component 94b346d7-0cde-4e6e-b819-95d6f200bbf7 MyComponent.js
+# The following line re-registers an existing CID
+component {94b346d7-0cde-4e6e-b819-95d6f200bbf7} MyOtherComponent.js
+
+# The following line maps a contractID to a non-existent CID
+contract @testing/foo {0c07730f-f875-436b-8deb-90c4251920ec}
new file mode 100644
--- /dev/null
+++ b/xpcom/tests/unit/test_compmgr_warnings.js
@@ -0,0 +1,68 @@
+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+
+function info(s) {
+  dump("TEST-INFO | test_compmgr_warnings.js | " + s + "\n");
+}
+
+var gMessagesExpected = [
+  { line: 2, message: /Malformed CID/, found: false },
+  { line: 6, message: /re-register/, found: false },
+  { line: 9, message: /Could not/, found: false },
+];
+
+const kConsoleListener = {
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleListener]),
+
+  observe: function listener_observe(message) {
+    if (!(message instanceof Ci.nsIScriptError)) {
+      info("Not a script error: " + message.message);
+      return;
+    }
+
+    info("Script error... " + message.sourceName + ":" + message.lineNumber + ": " + message.errorMessage);
+    for each (let expected in gMessagesExpected) {
+      if (message.lineNumber != expected.line)
+        continue;
+
+      if (!expected.message.test(message.errorMessage))
+        continue;
+
+      info("Found expected message: " + expected.message);
+      do_check_false(expected.found);
+                
+      expected.found = true;
+    }
+  }
+};
+
+function run_deferred_event(fn) {
+  do_test_pending();
+  Components.classes["@mozilla.org/thread-manager;1"].
+    getService(Ci.nsIThreadManager).mainThread.dispatch(function() {
+      fn();
+      do_test_finished();
+    }, 0);
+}
+
+function run_test()
+{
+  let cs = Components.classes["@mozilla.org/consoleservice;1"].
+    getService(Ci.nsIConsoleService);
+  cs.registerListener(kConsoleListener);
+
+  var manifest = do_get_file('compmgr_warnings.manifest');
+  Components.manager.QueryInterface(Ci.nsIComponentRegistrar).
+    autoRegister(manifest);
+
+  run_deferred_event(function() {
+    cs.unregisterListener(kConsoleListener);
+
+    for each (let expected in gMessagesExpected) {
+      info("checking " + expected.message);
+      do_check_true(expected.found);
+    }
+  });
+}