1072152 - Change chrome.manifest read order

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Hommey [:glandium]]

As per bug 1063052 comment 81:
- It seems to me the main part of the failure in this bug comes from the fact that chrome manifests outside the omni.ja are treated after the chrome manifests inside the omni.ja. What about doing the opposite? If something is registered from an extracted omni.ja, it will be overridden by the contents of omni.ja. If people really want to use the contents of the extracted omni.ja, they'll have to remove omni.ja.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Change chrome.manifest read order

In case of multiple "resource" manifest entries for the same keyword, the
last registered one now takes precedence, like any other chrome manifest
entry.

The latter is required, well, first because it's awkward that the last
chrome manifest entry takes precedence for all chrome types except
resources. But more importantly, in the case of bug 1063052, one part of
the failure, at least one I got by upgrading 31 to nightly after unzipping
omni.ja, is that resource://gre-resource resolves to the unzipped tree and
doesn't contain some of the new, required files, while omni.ja registers
resource://gre-resources/.

So to summarize, without the component manager changes:
  chrome://foo/ points to (old) unzipped
  resource://foo/ points to (old) unzipped
  layout code loads resource://gre-resources/counterstyles.css which doesn't exist
with the component manager changes:
  chrome://foo/ points to omni.ja
  resource://foo/ points to (old) unzipped
  layout code loads resource://gre-resources/counterstyles.css which doesn't exist
Neither of the above are desirable. With the complete changes from this patch:
  chrome://foo/ points to omni.ja
  resource://foo/ points to omni.ja
  layout code properly loads resource://gre-resources/counterstyles.css.


Note that, alternatively, we could just have actual chrome.manifest files near each
omni.ja with a manifest entry pointing to the chrome.manifest inside the omni.ja. So
 in practice, we'd always have a chrome.manifest and always be overwritting whatever
people would be doing to their chrome.manifest. That's slightly more work, as I'm not
sure "manifest" entries support jar urls so maybe support for that would be needed
(although if that's needed, that shouldn't be hard), and that would also require
packager changes.

Comment 2

[Benjamin Smedberg]

Comment on attachment 8494406 [details] [diff] [review]
Change chrome.manifest read order

This is correct, but I still feel like we're doing this in a weird order within this method. Is there a reason we can't reorder this method so that things are registered in the order they will be processed? so:

RegisterModule(&kXPCOMModule); // needed for mozilla::Monijar to work, right?
greDir
greOmnijar
appDir
appOmnijar

I'm going to mark r+ on this version but I'd really prefer that form better!

Comment 3

[Mike Hommey [:glandium]]

Attachment: Change chrome.manifest read order

In case of multiple "resource" manifest entries for the same keyword, the
last registered one now takes precedence, like any other chrome manifest
entry.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8495014 [details] [diff] [review]
Change chrome.manifest read order

This unfortunately breaks m-e10s bc3 and xpcshell.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Change chrome.manifest read order

So, the xpcshell problem was just a test dans was explicitly testing the current behavior that registering resources has.

But the best of all is the e10s test that somehow changes an existing error from a warning to an error because the test loads the superpowers addon that registers the superpowers resource that already is registered by marionette. So before, that registration wasn't happening and the error about AboutProtocolParent.tryUnregisterFactory not existing ends up a warning. Now, the registration happens and that error ends up an actual error that turns the test red.

Comment 6

[Mike Hommey [:glandium]]

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

Comment 7

[Carsten Book [:Tomcat]]

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