Closed Bug 691551 Opened 8 years ago Closed 8 years ago

Suppress warnings when an addon's chrome.manifest is unregistered

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 3 obsolete files)

Calls to CheckForNewChrome causes warnings on the console, such as:

> Warning: Duplicate resource declaration for '...' ignored.

> Could not read chrome manifest file '.../chrome.manifest'.

We should suppress them (see also bug 564667 comment 15).
Attached patch patch (obsolete) — Splinter Review
Dave, is this a reasonable approach?
Attachment #564409 - Flags: feedback?(dtownsend)
Comment on attachment 564409 [details] [diff] [review]
patch

I think I'm generally against this. I don't really care about re-logging registration errors (we should fix the default theme case though). I think we should be able to suppress duplicate registration errors by seeing if they are just re-registering the same thing and if so ignoring it rather than logging about it. Bsmedberg ought to take a look at this though so feel free to pass it past him for a look see first.
Attachment #564409 - Flags: feedback?(dtownsend) → feedback-
I don't understand yet: the "could not read chrome manifest" warning is already filed and isn't specific to this case, so let's ignore it.

The "Duplicate resource declaration" looks like a bug in CheckForNewChrome: we should be clearing all the resource mappings here: http://mxr.mozilla.org/mozilla-central/source/chrome/src/nsChromeRegistryChrome.cpp#410 so that there aren't any leftovers. But I suspect that since resource mappings were added more recently to the chrome registry that was never implemented.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> The "Duplicate resource declaration" looks like a bug in CheckForNewChrome:
> we should be clearing all the resource mappings here:
> http://mxr.mozilla.org/mozilla-central/source/chrome/src/
> nsChromeRegistryChrome.cpp#410 so that there aren't any leftovers. But I
> suspect that since resource mappings were added more recently to the chrome
> registry that was never implemented.

Unfortunately it isn't quite as simple as that. Because resource mappings can be defined both by chrome.manifest entries and directory on nsIResProtocolHandler we can't just clear them all. We only want to clear those defined in chrome.manifest.
I'm not sure if this is a different bug, but ideally we should skip registering manifests for bootstrapped add-ons that don't provide them, which should cut down on a lot of this noise. I'm now getting an Error Console full of "Could not read chrome manifest file" messages every time I enable/disable/install a bootstrapped add-on in my test profile.

Am I to understand by comment 3 that there's already a bug for this (I can't find it)?
(In reply to Kris Maglione [:kmag] from comment #5)
> I'm not sure if this is a different bug, but ideally we should skip
> registering manifests for bootstrapped add-ons that don't provide them,
> which should cut down on a lot of this noise. I'm now getting an Error
> Console full of "Could not read chrome manifest file" messages every time I
> enable/disable/install a bootstrapped add-on in my test profile.
> 
> Am I to understand by comment 3 that there's already a bug for this (I can't
> find it)?

Pretty sure there isn't a bug on this already
(In reply to Kris Maglione [:kmag] from comment #5)
> I'm not sure if this is a different bug, but ideally we should skip
> registering manifests for bootstrapped add-ons that don't provide them,
> which should cut down on a lot of this noise. I'm now getting an Error
> Console full of "Could not read chrome manifest file" messages every time I
> enable/disable/install a bootstrapped add-on in my test profile.

This seems as good a place as any for this patch, which does as Kris describes.
Attachment #569298 - Flags: review?(dtownsend)
Comment on attachment 569298 [details] [diff] [review]
stop loading and unloading where chrome.manifest doesn't exist

Review of attachment 569298 [details] [diff] [review]:
-----------------------------------------------------------------

Slightly concerned over the timing here. I wonder if we used the zipreader cache if that'd give us some wins. Might break tests though.
Attachment #569298 - Flags: review?(dtownsend) → review+
Attached patch patch v3 (obsolete) — Splinter Review
A third approach: only store the manifest location in sModuleLocations if the manifest actually exists. This achieves basically the same result without the zip reader overhead.
Attachment #571476 - Flags: review?(dtownsend)
Comment on attachment 571476 [details] [diff] [review]
patch v3

Review of attachment 571476 [details] [diff] [review]:
-----------------------------------------------------------------

I don't get it, the code that tests whether the manifest exists or not still logs an error when it doesn't so how does this solve the problem?

::: xpcom/components/nsComponentManager.cpp
@@ +2117,3 @@
>      if (!nsComponentManagerImpl::gComponentManager ||
>          nsComponentManagerImpl::NORMAL != nsComponentManagerImpl::gComponentManager->mStatus)
>          return NS_OK;

This is going to mean it fails to add the jar manifest to sModuleLocations if called during startup
(In reply to Dave Townsend (:Mossop) from comment #10)
> I don't get it, the code that tests whether the manifest exists or not still
> logs an error when it doesn't so how does this solve the problem?

Oops, missed it. I'll check aType for bootstrapped add-ons and skip the warning.
I'll chase down the duplicate resource messages in another bug and leave it at that.
Assignee: nobody → geoff
Attachment #564409 - Attachment is obsolete: true
Attachment #569298 - Attachment is obsolete: true
Attachment #571476 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #571476 - Flags: review?(dtownsend)
Attachment #571793 - Flags: review?(dtownsend)
Attachment #571793 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef0684ac019
Flags: in-testsuite-
Flags: in-litmus-
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/aef0684ac019
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.