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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Geoff Lankow (:darktrojan)]

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).

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: patch

Dave, is this a reasonable approach?

Comment 2

[Dave Townsend [:mossop]]

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.

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Dave Townsend [:mossop]]

(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.

Comment 5

[Kris Maglione [:kmag]]

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)?

Comment 6

[Dave Townsend [:mossop]]

(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

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: stop loading and unloading where chrome.manifest doesn't exist

(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.

Comment 8

[Dave Townsend [:mossop]]

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.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: patch v3

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.

Comment 10

[Dave Townsend [:mossop]]

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

Comment 11

[Geoff Lankow (:darktrojan)]

(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.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: suppress 'no chrome.manifest' messages on enable

I'll chase down the duplicate resource messages in another bug and leave it at that.

Comment 13

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef0684ac019

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/aef0684ac019