Closed
Bug 1385057
Opened 8 years ago
Closed 7 years ago
Remove support for unpacked side-loaded extensions
Categories
(Toolkit :: Add-ons Manager, enhancement, P2)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | verified |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug, )
Details
(Keywords: addon-compat, dev-doc-complete)
Attachments
(2 files)
No description provided.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•8 years ago
|
||
The longer we wait to do this, the harder it's going to become.
Priority: P3 → --
Updated•8 years ago
|
Keywords: addon-compat,
dev-doc-needed
Comment 3•8 years ago
|
||
Assuming Kris is wanting to move this up in priority, going for a P1.
Flags: needinfo?(aswan)
Comment 5•8 years ago
|
||
In a meeting that I think you were in, we assigned it to P1. In comment 1 Kris removed the P3 suggesting it should be a higher priority, but not setting it to one. I set it to P1 in comment 3 and since its on add-ons manager and a P1 I think your input would be helpful on if it is a P1.
Flags: needinfo?(amckay)
Comment 6•8 years ago
|
||
(In reply to Andy McKay [:andym] from comment #5)
> In a meeting that I think you were in, we assigned it to P1
s/P1/P3/
Comment 7•8 years ago
|
||
Ah. I think its an important thing to do but not more important than all the other stuff on my plate for 57. If Kris has the cycles to do it now, that's great otherwise, realistically, its going to have to wait.
Updated•8 years ago
|
Priority: -- → P2
Comment 8•7 years ago
|
||
How are we going to handle compatibility for this? I just updated MDN but until a few minutes ago the documented method for sideloading on Windows involved unpacking. Is there any practical way to notify all the authors of extensions that are installed in this way?
Assignee: nobody → aswan
Flags: needinfo?(jorge)
Comment 9•7 years ago
|
||
I'm asking just out of curiosity, but why shouldn't unpacked extensions be allowed to install via global install locations? I failed to find a concrete reason looking through the related issues.
Comment 10•7 years ago
|
||
I don't think there's good way to identify those developers.
Flags: needinfo?(jorge)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Lakatos.Isti from comment #9)
> I'm asking just out of curiosity, but why shouldn't unpacked extensions be
> allowed to install via global install locations? I failed to find a concrete
> reason looking through the related issues.
We don't support unpacked extensions in production environments anymore. We allow them in development environments, mainly as temporary extensions, but that comes with a performance penalty, and involves code paths we don't want to have to support in production.
Comment 12•7 years ago
|
||
Unassigning myself for now until we have a strategy for how to deploy this.
Assignee: aswan → nobody
Flags: needinfo?(mconca)
Comment 13•7 years ago
|
||
The plan is to deprecate support for unpacked extensions loaded via the registry with the release of Firefox 62 (currently scheduled for 21-Aug-2018). A communications plan is being formed to give notice to as many developers as possible during the six-month sunset window.
Flags: needinfo?(mconca)
Assignee | ||
Comment 14•7 years ago
|
||
These extensions will no longer work after bug 1460350 lands, but that still leaves behind a bunch of dead code for verifying unpacked extensions, which can now go away.
Assignee: nobody → kmaglione+bmo
Blocks: aom-code-quality
Summary: Don't allow unpacked WebExtensions in global (e.g., registry) install locations → Remove support for unpacked side-loaded extensions
Comment hidden (mozreview-request) |
![]() |
||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review248762
Cool - security/ parts lgtm.
Attachment #8974573 -
Flags: review?(dkeeler) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review248792
::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:458
(Diff revision 1)
> * then the uri to the root of the contained files will be returned
> * @returns {nsIURI}
> * An nsIURI pointing at the resource
> */
> function getURIForResourceInFile(aFile, aPath) {
> - if (aFile.exists() && aFile.isDirectory()) {
> + if (!aFile.leafName.toLowerCase().endsWith(".xpi")) {
What's the purpose of this change? It is not unheard of for people to install .zip files (which I think you can only do from about:debugging, but people do do that...)
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review248792
> What's the purpose of this change? It is not unheard of for people to install .zip files (which I think you can only do from about:debugging, but people do do that...)
Calling .isFile() is expensive. Pretty sure we don't support installing .zip files. If we do, I expect we'd have tests. We use `.endsWith(".xpi")` or some variant thereof all over the place. But if we do support them, we probably shouldn't.
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review248792
> Calling .isFile() is expensive. Pretty sure we don't support installing .zip files. If we do, I expect we'd have tests. We use `.endsWith(".xpi")` or some variant thereof all over the place. But if we do support them, we probably shouldn't.
Like I said it only works from about:debugging (see bug 1353861).
It would be nice to not break it but if isFile()/isDirectory() does a main thread stat() every time it is called then that sucks. Could we do something like always stick a trailing / on directories and just check that?
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review248792
> Like I said it only works from about:debugging (see bug 1353861).
> It would be nice to not break it but if isFile()/isDirectory() does a main thread stat() every time it is called then that sucks. Could we do something like always stick a trailing / on directories and just check that?
Not really.
Honestly, supporting .zip files as extensions is probably a misfeature that I could not care less about.
That said, this probably doesn't break them. Or, at least, it probably doesn't break WebExtensions. It may break bootstrapped extensions, but I care even less about supporting .zip bootstrapped extensions.
If it *does* break them, then there should be tests that fail.
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review248928
::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:44
(Diff revision 1)
> minVersion: "1",
> maxVersion: "1"
> }],
> name: "Test Delay Update Ignore",
> - }, profileDir, IGNORE_ID, "bootstrap.js");
> + }, profileDir, IGNORE_ID, {
> + "bootstrap.js": String.raw`
Looks like `data/test_delay_*/bootstrap.js` can be removed now
::: toolkit/mozapps/extensions/test/xpcshell/test_proxy.js:13
(Diff revision 1)
>
> BootstrapMonitor.init();
>
> // Ensure that a proxy file to an add-on with a valid manifest works.
> add_task(async function() {
> + if (AppConstants.MOZ_REQUIRE_SIGNING) {
Looks like this can just be done from xpcshell.ini?
::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:174
(Diff revision 1)
> }),
> "bootstrap.js": bootstrapJS,
> };
>
> let target;
> - if (packed) {
> + if (!packed) {
ugh
::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
(Diff revision 1)
> tags = blocklist
> [test_gmpProvider.js]
> skip-if = appname != "firefox"
> [test_harness.js]
> [test_install.js]
> -[test_install_from_sources.js]
I'm in favor of just removing that function altogether. But not a fan or removing the test and leaving untested code sitting around...
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review248928
> Looks like `data/test_delay_*/bootstrap.js` can be removed now
Yeah, sorry, I meant to remove them.
> Looks like this can just be done from xpcshell.ini?
Oh, yeah. I couldn't find any tests that used it, but we have mozconfig.require_signing.
> I'm in favor of just removing that function altogether. But not a fan or removing the test and leaving untested code sitting around...
Oh. I thought it was still used internally, but I guess not. Yay. More code to delete.
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.
https://reviewboard.mozilla.org/r/242912/#review249372
Attachment #8974573 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 25•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6caf1d4d23bc139562493ffbb8c0f199ed0b30
Bug 1385057: Remove most code for handling unpacked side-loaded extensions. r=aswan,keeler
![]() |
||
Comment 26•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 27•7 years ago
|
||
Tested and reproduced issue on Firefox 61.0a1 (20180329222832).
Retested and verified in Firefox 62.0a1 (20180522100110) on Windows 10 x64 and Ubuntu 16.04 LTS.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 28•7 years ago
|
||
:aswan, I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Alternative_distribution_options/Add-ons_in_the_enterprise#Installation_using_the_Windows_registry to try to clarify that the old way isn't supported, but AFAICT the docs only talk about packaged extensions already.
Let me know if there's anything else needed here.
Flags: needinfo?(aswan)
Comment 29•7 years ago
|
||
Looks good, thanks!
Flags: needinfo?(aswan)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•