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

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43+ fixed, firefox44+ fixed, firefox45+ fixed, firefox-esr38- wontfix, b2g-v2.5 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
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?
Flags: needinfo?(benjamin)
Assignee

Comment 1

4 years ago
And restore resource://app/ to its true meaning.
Attachment #8686293 - Flags: review?(mshal)
Assignee

Updated

4 years ago
Summary: browser/omni.ja doesn't contain startup cache anymore → browser/omni.ja doesn't contain startup cache anymore and resource://app/ doesn't point where it's supposed to
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.
Attachment #8686293 - Flags: review?(mshal) → review+
Assignee

Comment 3

4 years ago
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.
Attachment #8686293 - Flags: review?(josh)

Comment 4

4 years ago
Holy crap, did I review that? So sorry.

I agree that we should disallow remapping of the core resource mappings.
Flags: needinfo?(benjamin)
Assignee

Updated

4 years ago
Blocks: 1225384
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.
Assignee

Comment 6

4 years ago
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.
Attachment #8686293 - Flags: review?(josh) → review?(markh)
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?
Attachment #8686293 - Flags: review?(markh) → review?(MattN+bmo)
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
Attachment #8686293 - Flags: review?(MattN+bmo) → review+
Tracking since this made bsmedberg almost swear. Serious business. 
Please request uplift if this lands and if that seems like a good idea.
Assignee

Updated

4 years ago
Depends on: 1226119

Comment 11

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ef9720309ef
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee

Comment 12

4 years ago
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
Attachment #8686293 - Flags: approval-mozilla-esr38?
Attachment #8686293 - Flags: approval-mozilla-beta?
Attachment #8686293 - Flags: approval-mozilla-aurora?
Sorry but it doesn't match the ESR criteria (only important security issues or very important top crashes).
Attachment #8686293 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
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.
Attachment #8686293 - Flags: approval-mozilla-beta?
Attachment #8686293 - Flags: approval-mozilla-beta+
Attachment #8686293 - Flags: approval-mozilla-aurora?
Attachment #8686293 - Flags: approval-mozilla-aurora+

Updated

2 years ago
Depends on: 1366180
You need to log in before you can comment on or make changes to this bug.