Closed
Bug 1224000
Opened 9 years ago
Closed 9 years ago
browser/omni.ja doesn't contain startup cache anymore and resource://app/ doesn't point where it's supposed to
Categories
(Core :: Permission Manager, defect)
Core
Permission Manager
Tracking
()
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file)
2.48 KB,
patch
|
mshal
:
review+
MattN
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr38-
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
And restore resource://app/ to its true meaning.
Attachment #8686293 -
Flags: review?(mshal)
Assignee | ||
Updated•9 years ago
|
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox-esr38:
--- → affected
tracking-firefox43:
--- → ?
tracking-firefox44:
--- → ?
tracking-firefox-esr38:
--- → ?
Assignee | ||
Updated•9 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 2•9 years ago
|
||
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•9 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•9 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)
Comment 5•9 years ago
|
||
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•9 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 7•9 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]:
-----------------------------------------------------------------
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 8•9 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]:
-----------------------------------------------------------------
Manual spot-check was good
Attachment #8686293 -
Flags: review?(MattN+bmo) → review+
Comment 9•9 years ago
|
||
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•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Comment 12•9 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?
Comment 13•9 years ago
|
||
Sorry but it doesn't match the ESR criteria (only important security issues or very important top crashes).
Updated•9 years ago
|
Attachment #8686293 -
Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
Comment 14•9 years ago
|
||
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+
Comment 15•9 years ago
|
||
bugherder uplift |
Comment 16•9 years ago
|
||
bugherder uplift |
Comment 17•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•