1224000 - browser/omni.ja doesn't contain startup cache anymore and resource://app/ doesn't point where it's supposed to

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Mike Hommey [:glandium]]

The root cause for this is that the startup cache population script resolves resource://app/ and iterates over components and modules in there. But bug 1073095 made resource://app/ mean something different from what it is supposed to mean, which I'm surprised didn't have more unintended consequences. Maybe that's because we're not using resource://app/ that much and use resource:/// instead, which is supposed to have the same meaning.

That being said, if addons are using resource://app/ they may be suffering.

Overriding resource://app/ is NOT fine, and I'm also tempted to make the resource protocol handler reject reassignments of substitutions for app, gre and ''. Benjamin, what do you think?

Comment 1

[Mike Hommey [:glandium]]

Attachment: Install defaults/permissions file under browser/ instead of under browser/chrome/browser

And restore resource://app/ to its true meaning.

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8686293 [details] [diff] [review]
Install defaults/permissions file under browser/ instead of under browser/chrome/browser

Looks good to me. I don't know what potential consequences there might be, though.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8686293 [details] [diff] [review]
Install defaults/permissions file under browser/ instead of under browser/chrome/browser

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

Let's run this through a permission manager peer, then.

Comment 4

[Benjamin Smedberg]

Holy crap, did I review that? So sorry.

I agree that we should disallow remapping of the core resource mappings.

Comment 5

[Josh Matthews [:jdm]]

Honestly I've never seen these things before and I have no idea what the implications are. My review would just be a rubber stamp.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8686293 [details] [diff] [review]
Install defaults/permissions file under browser/ instead of under browser/chrome/browser

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

(In reply to Josh Matthews [:jdm] from comment #5)
> Honestly I've never seen these things before and I have no idea what the
> implications are. My review would just be a rubber stamp.

Let's try Mark, then, since, while he's not a peer, he wrote the original code reading the default permission file through resources.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8686293 [details] [diff] [review]
Install defaults/permissions file under browser/ instead of under browser/chrome/browser

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

Sorry, but I'm not on top of this. I'd also be expecting nsPermissionManager.cpp to have its reference to "resource://app/defaults/permissions" change, but that might just be a reflection of that fact I don't know enough about this stuff.

MattN, are you able to have a look at this?

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8686293 [details] [diff] [review]
Install defaults/permissions file under browser/ instead of under browser/chrome/browser

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

Manual spot-check was good

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking since this made bsmedberg almost swear. Serious business. 
Please request uplift if this lands and if that seems like a good idea.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef9720309ef

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ef9720309ef

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8686293 [details] [diff] [review]
Install defaults/permissions file under browser/ instead of under browser/chrome/browser

Approval Request Comment
[Feature/regressing bug #]: Regression from bug 1073095
[User impact if declined]: This may have an impact on startup times under some conditions, but more importantly, this bug broke several addons in possibly subtle ways. See https://groups.google.com/d/msg/mozilla.dev.platform/KatkCgbD0FU/5E5PVOOGCAAJ
[Describe test coverage new/current, TreeHerder]:
- Without the change, opening resource://app/ and resource:/// list different things. With the change, they list the same things.
- Without the change, resource:///jsloader/ doesn't exist, with the change, it does.
- With and without the change, resource://app/defaults/permissions show the same thing.
[Risks and why]: Low. It doesn't touch anything else than what resource://app/ means, and only fixes what it means to match resource:/// which is what it is supposed to mean, while ensuring resource://app/defaults/permissions is still valid.
[String/UUID change made/needed]: None

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Sorry but it doesn't match the ESR criteria (only important security issues or very important top crashes).

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686293 [details] [diff] [review]
Install defaults/permissions file under browser/ instead of under browser/chrome/browser

Not a recent regression but let's uplift it to aurora and beta. Worth the risk to fix addon regressions and improve startup time.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/786c011b2594

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0038529143f2

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/786c011b2594