285063 - No visible error messages for malformed/invalid chrome.manifest in extension

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Nickolay_Ponomarev]

If you make a typo in chrome.manifest (introduced in bug 278534), there are no
messages shown in JS Console or anywhere (only NS_WARNINGs). The manifest (or
parts of it) is just ignored.

Previous code shown a "Chrome registration failure" window and opened JS Console
with details.

Please reimplement this behaviour for chrome.manifest files. As someone who
spends time helping out extension dev. newcomers, I can tell you it's pretty
important.

If you need a testcase XPI, I'll attach it.

Comment 1

[Benjamin Smedberg]

As I said in bug 278534, this was part of my original plan. But I'm surprised to
hear that there were JS console errors reported from the old chrome registry
code. What kind of errors were they? (Text and/or screenshots from the old
errors would be helpful to me.)

Comment 2

[Nickolay_Ponomarev]

It gave this message at startup:
http://mozilla.doslash.org/stuff/chrome-reg-error.png

When you click on View Details JS Console opened with this message:
--
Chrome Registration failed for Extension '..guid..' when calling
nsIXULChromeRegistry::installPackage with this chrome path:
jar:file:///...profile.../extensions/...guid.../chrome/infolister.jar!/content/infolister/
(profile extension = true). Perhaps this path does not exist within the chrome
JAR file, or the contents.rdf file at that location is malformed?
--

It's not perfect, but at least it didn't fail silently.

Sorry I didn't notice that you said you were going to fix this (in bug 278534
comment 17). btw, I think EM should show an error message like I posted, not
just print a message to JS Console.

Comment 3

[Benjamin Smedberg]

Attachment: Add console logging for errors possibly encountered during app/extension development

Comment 4

[Darin Fisher]

Comment on attachment 178186 [details] [diff] [review]
Add console logging for errors possibly encountered during app/extension development

>Index: chrome/src/nsChromeRegistry.cpp

>+static void
>+LogMessage(const char* aMsg, ...)

I think you need to mark this function __cdecl under Windows in order
for the vararg to work as expected.


>+  va_list args;
>+  char* formatted = PR_vsmprintf(aMsg, args);
>+  if (!formatted)

Where are the calls to va_start and va_end?  How does this work?



>+static void
>+LogMessageWithContext(nsIURI* aURL, PRUint32 aLineNumber, PRUint32 flags,
>+                      const char* aMsg, ...)

same problems with this guy.


Could we get into a situation where the developer isn't even able to bring
up the JS console?  Should we maybe support a NSPR log module where these
messages are dumped to in addition to the JS console?

Comment 5

[Benjamin Smedberg]

> >+static void
> >+LogMessage(const char* aMsg, ...)
> 
> I think you need to mark this function __cdecl under Windows in order
> for the vararg to work as expected.

Is there a macro I should use for this (NS_METHOD)?

> Where are the calls to va_start and va_end?  How does this work?

I think that the va_start macro by itself should be sufficient (and it works on
my linux gcc3.2)... the vprintf calls take a va_args param, not individual
pointers, just for this situation if I understand it correctly.

> Could we get into a situation where the developer isn't even able to bring
> up the JS console?  Should we maybe support a NSPR log module where these

We can get into that situation, and I intend to support a console dump-to-file
function (which is why I filed the scripterror.message bug, so that the
dump-to-file function can be useful. No bug yet, but I'll file one soon.

Comment 6

[Darin Fisher]

See usage of va_list, va_start, and va_end in this existing code:

http://lxr.mozilla.org/mozilla/source/nsprpub/pr/src/io/prprf.c#1092

It seems to me that you have to initialize the va_list object or else how can it
find the parameters on the stack?  Even if you don't intend to call va_arg, you
still need to initialize the va_list, no?

Comment 7

[Benjamin Smedberg]

Attachment: Add console logging for chrome registry errors, rev. 1.1

Comment 8

[Darin Fisher]

Comment on attachment 178266 [details] [diff] [review]
Add console logging for chrome registry errors, rev. 1.1

> #define NS_METHOD_(type) type __stdcall

NS_METHOD is certainly wrong.  It may be the case that __cdecl is the default
when compiling under MSVC.  I know that NSPR_API assumes that it is, so perhaps
there is no need to add it manually.  The code as is should crash on Windows.

My apologies for not commenting on that previously.

r=darin assuming you remove the NS_METHOD_ macros.

Comment 9

[Benjamin Smedberg]

Comment on attachment 178266 [details] [diff] [review]
Add console logging for chrome registry errors, rev. 1.1

shaver, you're not allowed to complain if this doesn't log enough :-P