The changes in bug 1332190 broke the followonsearch system add-on: https://github.com/mozilla/followonsearch/blob/master/add-on/content/followonsearch-fs.js In particular, the frame script loads, but we no longer seem to be getting any events and there are no errors on the console. The only guess we had was: Cu.importGlobalProperties(["URLSearchParams"]); Any ideas appreciated.

>The only guess we had was: >Cu.importGlobalProperties(["URLSearchParams"]); removed that and it didn't fix the problem.

I don't immediately recognize what the cause is, to start debugging I'd recommend: * set `security.sandbox.logging.enabled` to `true` * Close Firefox * Open "Console" and in the search field enter "plugin-container" * Open firefox and run the addon * Paste whatever you see in console I should warn you, we were aware that the level three restrictions may break some legacy addons, so this may be a "wontfix" from our side -- but let's try to understand what happened first.

I see no references to plugin-container in the Mac console at all.

Discussed on IRC, a) Console.app vs the temrinal is a horribly confusing thing, b) the root problem is that the addon was being installed as a temporary addon, and temporary addon framescripts are loaded from "wherever they are on disk" by the content process, so that fails. :haik, do you remember what we conclude about this and if we documented anywhere? (I assume the conclusion was "WONTFIX" because legacy addons are going away :-))

System add-ons will continue to be bootstrap, so this needs to be documented somewhere because we'll need to flip the pref to 2 when testing locally. I imagine extension developers have probably been hitting this on developer edition without knowing what is going wrong. Couldn't we have waited to flip this pref until 57? And we really need to publicize this for add-on developers.

(In reply to Mike Kaply [:mkaply] from comment #5) > System add-ons will continue to be bootstrap, so this needs to be documented > somewhere because we'll need to flip the pref to 2 when testing locally. Mike, as a workaround, could you try placing your xpi in ~/Library/Application Support/Mozilla/Extensions and then load it? (~/Library is a hidden folder by default. In the "Load Temporary Extension" open dialog, you can press shift-command-. to toggle display of ~/Library. Or turn on showing Library in Finder->View->Show View Options.) Assuming the workaround works for you, that will be an option in 57 as well. We plan to disable reading from this directory some time after 57 with bug 1356167. > I imagine extension developers have probably been hitting this on developer > edition without knowing what is going wrong. > > Couldn't we have waited to flip this pref until 57? Turning this on allows us to ship a much stronger security sandbox on Windows/Mac/Linux so getting it out earlier has a big benefit to users. (Linux builds are not affected by this because, per current plan, file system read access sandboxing will be in 57.) > And we really need to publicize this for add-on developers. It's been documented that Firefox 54-56 will impact file system access from the content process: https://blog.mozilla.org/addons/2017/02/16/the-road-to-firefox-57-compatibility-milestones/ That said, we weren't aware of the impact to legacy temporary installs until later and we could add something specifically about installing temporary legacy extensions. Sorry you had to run into this problem.

(In reply to Haik Aftandilian [:haik] from comment #6) > (In reply to Mike Kaply [:mkaply] from comment #5) > > System add-ons will continue to be bootstrap, so this needs to be documented > > somewhere because we'll need to flip the pref to 2 when testing locally. > > Mike, as a workaround, could you try placing your xpi in > ~/Library/Application Support/Mozilla/Extensions and then load it? > (~/Library is a hidden folder by default. In the "Load Temporary Extension" > open dialog, you can press shift-command-. to toggle display of ~/Library. > Or turn on showing Library in Finder->View->Show View Options.) Assuming the > workaround works for you, that will be an option in 57 as well. We plan to > disable reading from this directory some time after 57 with bug 1356167. I'm not sure this is the place to discuss it, but we really do need a way of supporting development of legacy add-ons for System Add-on work - including post 57. WebExtensions aren't viable alternatives for most system add-ons, and my previous understanding was that System add-ons could continue to be legacy add-ons - and that means we need a viable debugging environment. I understand we need to put security first, but we also need to be able to develop Firefox as well.

(In reply to Mark Banner (:standard8) from comment #7) > (In reply to Haik Aftandilian [:haik] from comment #6) > > (In reply to Mike Kaply [:mkaply] from comment #5) > > > System add-ons will continue to be bootstrap, so this needs to be documented > > > somewhere because we'll need to flip the pref to 2 when testing locally. > I'm not sure this is the place to discuss it, but we really do need a way of > supporting development of legacy add-ons for System Add-on work - including > post 57. WebExtensions aren't viable alternatives for most system add-ons, > and my previous understanding was that System add-ons could continue to be > legacy add-ons - and that means we need a viable debugging environment. > > I understand we need to put security first, but we also need to be able to > develop Firefox as well. From the sandboxing perspective, we can add exceptions to ease the transition (by whitelisting a particular directory where the extension could be installed from), but I'd worry any legacy extension is going to be dependent on functionality being removed in 57+ (bug 1347507). That's better addressed by someone more knowledgeable about extension API's.

In theory any developer could just drop security.sandbox.content.level to 2, right? I'm fine with that workaround, we just need to document it somewhere.

(In reply to Mike Kaply [:mkaply] from comment #9) > In theory any developer could just drop security.sandbox.content.level to 2, > right? That's right, but it's better if we have a workaround that doesn't require lowering the level. Developers might end up writing new code that works with level 2, but breaks on higher levels we are shipping.

Bob, gcp, before we try to document a workaround, what are your thoughts on a Windows and Linux workaround for this problem? Specifically, loading a legacy extension via about:debugging needs to be done from a directory that content can read from. In comment 7, Mark mentioned we'll need to allow this in some form even after 57. The workaround I proposed above for Mac was to put the .XPI file in the per-user extensions directory. I'm thinking a new specific "LegacyExtensions" directory such as "~/Library/Application Support/Mozilla/LegacyExtensions" would be better. Do we have a per-user extensions dir on Linux?

(In reply to Haik Aftandilian [:haik] from comment #11) > Bob, gcp, before we try to document a workaround, what are your thoughts on > a Windows and Linux workaround for this problem? Specifically, loading a > legacy extension via about:debugging needs to be done from a directory that > content can read from. In comment 7, Mark mentioned we'll need to allow this > in some form even after 57. > > The workaround I proposed above for Mac was to put the .XPI file in the > per-user extensions directory. I'm thinking a new specific > "LegacyExtensions" directory such as "~/Library/Application > Support/Mozilla/LegacyExtensions" would be better. Do we have a per-user > extensions dir on Linux? We already have a dir (XRE_USER_SYS_EXTENSION_DIR "XREUSysExt"), on Windows this is: <user_home>\AppData\Roaming\Mozilla\Extensions Could we just use that? If we have a new dir maybe it should be called SystemExtensions (maybe SystemExtensionsDev) not Legacy, otherwise it will add confusion over whether we are continuing support somehow.

Whitelisting XRE_USER_SYS_EXTENSION_DIR should work on Linux.

> Specifically, loading a legacy extension via about:debugging needs to be done from a directory that content can read from. Could about:debugging take care of the workaround? i.e. copy the XPI when initially selected, then copy from the original location again when reloaded That, or we'll have to work that into our dev tooling to make sure the output of the build ends up in the right place. Beyond documentation, a reliable way to programmatically identify that directory outside of Firefox (e.g. in a node.js script) on every platform would be great to avoid manual steps or a per-developer custom config.

From a sandboxing perspective, that solution is fine; I'll defer to the addons folks as to whether that copies the data at the right times to achieve the same effect as in-place was trying to.

Changing the browser so that about:debugging copied the legacy extension to another directory before loading should work, but it requires development effort for what's considered a deprecated use case that we don't want to encourage anyone to use that isn't doing system addons. The copying would only be needed for legacy extensions, not webextensions. If we can instead invest that time in the developer tooling, that would be better. It wouldn't require changes/risk to the browser. The web-ext tool used for webextensions development supports auto-reloading when files change so maybe it's possible to automatically trigger reloading of a legacy extension added via about:debugging. The directory that the XPI needs to be copied to would be different for Mac, Windows, and Linux, but it should never change.

Andy, could you weigh in on the feasibility of changing about:debugging temporary installs to support legacy extensions as described in comment 14. The alternative is for the legacy extension XPI to need to be copied to a sandbox-whitelisted directory before it can be installed with about:debugging.

It's possible, but I don't think its something we should be doing. Post 57 our goal is make the whole add-ons loading path simpler and not more complicated. It's currently quite complicated with a large number of different ways of loading code. Adding in more code for a very small number of developers that we want to transition over to WebExtensions does not sound good. It's a pain for a small number of people to have to copy those files around, but it will temporary and can be tooled outside of Firefox.

(In reply to Haik Aftandilian [:haik] from comment #16) > If we can instead invest that time in the developer tooling, that would be > better. It wouldn't require changes/risk to the browser. The web-ext tool > used for webextensions development supports auto-reloading when files change > so maybe it's possible to automatically trigger reloading of a legacy > extension added via about:debugging. > > The directory that the XPI needs to be copied to would be different for Mac, > Windows, and Linux, but it should never change. (In reply to Andy McKay [:andym] from comment #18) > It's a pain for a small number of people to have to copy those files around, > but it will temporary and can be tooled outside of Firefox. Fair enough! I guess the only piece then would be to come up with a way to determine that directory for tooling. Sounds like it changes by platform (mac/win/linux) but also user profile location depending on platform. Can we get a definite list of what those are? I wouldn't mind writing up a patch for web-ext if it can work there.

The(In reply to Bob Owen (:bobowen) from comment #12) > We already have a dir (XRE_USER_SYS_EXTENSION_DIR "XREUSysExt"), on Windows > this is: > <user_home>\AppData\Roaming\Mozilla\Extensions > > Could we just use that? The conclusion was that it would be better to not use the existing per-user extensions dir. Extensions there get loaded automatically if they are named in a certain way and we might end up with naming collisions and strange behavior. > If we have a new dir maybe it should be called SystemExtensions (maybe > SystemExtensionsDev) not Legacy, otherwise it will add confusion over > whether we are continuing support somehow. Good point.

The patch posted for review adds a new sandbox-whitelisted directory to be used by developers for loading system extensions until they can be transitioned to webextensions. The directory is Windows: <user_home>\AppData\Roaming\Mozilla\SystemExtensionsDev Mac: ~/Library/Application Support/Mozilla/SystemExtensionsDev Linux: ~/.mozilla/systemextensionsdev

Comment on attachment 8914986 [details] Bug 1393805 - Part 1 - Add XRE_USER_SYS_EXTENSION_DEV_DIR XRESysExtDev Key. https://reviewboard.mozilla.org/r/186244/#review191324 This code is ripe for clean-up/de-duplication, but as I assume we want to uplift probably best to keep it simple. ::: toolkit/xre/nsXREDirProvider.cpp:1655 (Diff revision 1) > } > > +nsresult > +nsXREDirProvider::AppendSysUserExtensionsDevPath(nsIFile* aFile) > +{ > + NS_ASSERTION(aFile, "Null pointer!"); I think we normally MOZ_ASSERT without description now.

Comment on attachment 8914988 [details] Bug 1393805 - Part 3 - Add Windows whitelisted directory for system extensions development. https://reviewboard.mozilla.org/r/186248/#review191328 Looks like this will need a rebase. One other thing, could this be Nightly (and possibly Beta) only? Perhaps they would want to test against release as well I suppose.

Comment on attachment 8914990 [details] Bug 1393805 - Part 5 - Test that the system extensions dev dir is readable from content. https://reviewboard.mozilla.org/r/186252/#review191332

Comment on attachment 8914987 [details] Bug 1393805 - Part 2 - Add Mac whitelisted directory for system extensions development. https://reviewboard.mozilla.org/r/186246/#review191408

Comment on attachment 8914986 [details] Bug 1393805 - Part 1 - Add XRE_USER_SYS_EXTENSION_DEV_DIR XRESysExtDev Key. https://reviewboard.mozilla.org/r/186244/#review191324 That was my thinking too regarding keeping changes minimal and not refactoring.

Comment on attachment 8914986 [details] Bug 1393805 - Part 1 - Add XRE_USER_SYS_EXTENSION_DEV_DIR XRESysExtDev Key. https://reviewboard.mozilla.org/r/186244/#review191498

Comment on attachment 8914989 [details] Bug 1393805 - Part 4 - Add Linux whitelisted directory for system extensions development. https://reviewboard.mozilla.org/r/186250/#review191500 ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:216 (Diff revision 1) > } > } > } > } > > + // ~/.mozilla/systemextensionsdev (bug 1393805) Does this actually work? See the comment immediately following this code!

Comment on attachment 8914989 [details] Bug 1393805 - Part 4 - Add Linux whitelisted directory for system extensions development. https://reviewboard.mozilla.org/r/186250/#review191502

Comment on attachment 8914989 [details] Bug 1393805 - Part 4 - Add Linux whitelisted directory for system extensions development. https://reviewboard.mozilla.org/r/186250/#review191518 ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:216 (Diff revision 1) > } > } > } > } > > + // ~/.mozilla/systemextensionsdev (bug 1393805) I should have mentioned I tested this on Release/56 on Linux/Windows/Mac by installing an extension in the directory and making sure it worked. This was the test extension posted on bug 1402325 as a test case. <https://bugzilla.mozilla.org/attachment.cgi?id=8911179>. And the patches include a test case added to browser_content_sandbox_fs.js. I just did a check on Nightly Linux where I disabled the file content process and then made sure that a web content process could read an HTML file from the directory. (I'm pretty sure I already verified that on Windows and Mac, but I'll make sure.) And I'll rearrange the code here so that the comment's mention of "the former" still makes sense.

Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/98092d164d57 Part 1 - Add XRE_USER_SYS_EXTENSION_DEV_DIR XRESysExtDev Key. r=bobowen,gcp https://hg.mozilla.org/integration/autoland/rev/8198bc4c7e3c Part 2 - Add Mac whitelisted directory for system extensions development. r=Alex_Gaynor https://hg.mozilla.org/integration/autoland/rev/45695eda1c1c Part 3 - Add Windows whitelisted directory for system extensions development. r=bobowen https://hg.mozilla.org/integration/autoland/rev/1ba3220d84fa Part 4 - Add Linux whitelisted directory for system extensions development. r=gcp https://hg.mozilla.org/integration/autoland/rev/4fe99f70e199 Part 5 - Test that the system extensions dev dir is readable from content. r=bobowen

Backed out for mass test failures, at least on OS X: https://hg.mozilla.org/integration/autoland/rev/768533298b5cf6e2dbe1d1f37fcdd515a354009d https://hg.mozilla.org/integration/autoland/rev/248ab0b65852195f58c8035ae9f46f79bd2ac490 https://hg.mozilla.org/integration/autoland/rev/35aa314f212747bbceb4be2b49672d5a1b945b7e https://hg.mozilla.org/integration/autoland/rev/531f3dcfbbfc4d7d6f36c0489ded0af79e7a13b8 https://hg.mozilla.org/integration/autoland/rev/31d7cf3009c9e5ce7b50d5ffb07752671fbbd002 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4fe99f70e199cacff46a87deaa0ed55c4308dabe&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable Take a look at this crashtest run: https://treeherder.mozilla.org/logviewer.html#?job_id=135007236&repo=autoland It seems to be stuck in the sandbox code: > application crashed [@ mozilla::dom::ContentChild::RecvSetProcessSandbox(mozilla::dom::MaybeFileDesc const&)]

The backout failures are due to the Mac sandbox change having a trailing slash in the subpath string which causes sandbox_init_with_parameters() to fail. Will fix that and re-land on central.

Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2f59f47baabc Part 1 - Add XRE_USER_SYS_EXTENSION_DEV_DIR XRESysExtDev Key. r=bobowen,gcp https://hg.mozilla.org/integration/autoland/rev/4d06927fff29 Part 2 - Add Mac whitelisted directory for system extensions development. r=Alex_Gaynor https://hg.mozilla.org/integration/autoland/rev/9fa8e68af088 Part 3 - Add Windows whitelisted directory for system extensions development. r=bobowen https://hg.mozilla.org/integration/autoland/rev/2e2d6d3b8421 Part 4 - Add Linux whitelisted directory for system extensions development. r=gcp https://hg.mozilla.org/integration/autoland/rev/2cfbfa84c880 Part 5 - Test that the system extensions dev dir is readable from content. r=bobowen

Tracking this for 56 though I'm not sure this will end up fixed on 56. We could aim for uplift to 57. How widely do we think this may affect release users?

https://hg.mozilla.org/mozilla-central/rev/2f59f47baabc https://hg.mozilla.org/mozilla-central/rev/4d06927fff29 https://hg.mozilla.org/mozilla-central/rev/9fa8e68af088 https://hg.mozilla.org/mozilla-central/rev/2e2d6d3b8421 https://hg.mozilla.org/mozilla-central/rev/2cfbfa84c880

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #51) > Tracking this for 56 though I'm not sure this will end up fixed on 56. We > could aim for uplift to 57. > > How widely do we think this may affect release users? This just affects developers. Developers of legacy extensions (that use frame scripts) working with build 56 will run into issues loading their extension via about:debugging. This provides a workaround so that developers can copy the extension to a particular directory and then load it with about:debugging. See comment 26 for the directory. The motivation for getting this in 57 is that system addons haven't fully transitioned to WebExtensions yet and will need this directory for 57+ work. See comment 7.

Does this need further testing? Do you want to request uplift to beta 57? Mike, from your point of view is this crucial to ship to 56 as well? Or can the fix wait till 57?

From conversation with mkaply, this can ride with 57.

Comment on attachment 8914986 [details] Bug 1393805 - Part 1 - Add XRE_USER_SYS_EXTENSION_DEV_DIR XRESysExtDev Key. Approval Request Comment [Feature/Bug causing the regression]: Filesystem read-access sandboxing restrictions on Windows/Mac in build 56 and Linux in 57. [User impact if declined]: For 57, developers working on system addons with frame scripts won't be able to load their addons using about:debugging. [Is this code covered by automated tests?]: The fix includes an automated test to make sure the new directory is readable. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: All the patches on the bug (patch 1 - 5). [Is the change risky?]: No [Why is the change risky/not risky?]: The changes adds read access to a new subdirectory for use by system addon developers and adds whitelist entries to the sandbox rules for each platform. The code added to for this is very similar to what is already used for similar situations where we need to whitelist a directory. [String changes made/needed]: None

Comment on attachment 8914986 [details] Bug 1393805 - Part 1 - Add XRE_USER_SYS_EXTENSION_DEV_DIR XRESysExtDev Key. Must fix, beta57+

https://hg.mozilla.org/releases/mozilla-beta/rev/2e90942f919f https://hg.mozilla.org/releases/mozilla-beta/rev/895ccb3cf659 https://hg.mozilla.org/releases/mozilla-beta/rev/6c62f6086493 https://hg.mozilla.org/releases/mozilla-beta/rev/031933e4a8dc https://hg.mozilla.org/releases/mozilla-beta/rev/9040197e9047

(In reply to Haik Aftandilian [:haik] from comment #56) > [Is this code covered by automated tests?]: > The fix includes an automated test to make sure the new directory is > readable. > > [Has the fix been verified in Nightly?]: > Yes > > [Needs manual test from QE? If yes, steps to reproduce]: > No Setting qe-verify- based on Haik's assessment on manual testing needs and the fact that this fix has automated coverage.