Closed Bug 1423687 Opened 8 years ago Closed 8 years ago

Warn about pitfalls with temporary bootstrapped extensions

Categories

(DevTools :: about:debugging, defect)

defect
Not set
normal

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: bdanforth, Assigned: jdescottes)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:59.0) Gecko/20100101 Firefox/59.0 Build ID: 20171205123322 Steps to reproduce: In a bootstrapped addon, I load a frame script with a global message manager with a `resource://` URL and delayed loading, similar to https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/bootstrap.js#175 to load it into all existing and new tabs. I temporarily install the addon XPI into Firefox (via about:debugging) and open a bunch of different pages, including a new tab (about:newtab). Actual results: I observe that the frame script loads in main process pages, like "about:config", "about:preferences", but not in content processes pages, like "about:newtab" or "https://www.mozilla.org/en-US/". If I lower the "security.sandbox.content.level” about:config pref from 3 to 2, my frame scripts load in content processes like "about:newtab". If I disable e10s, "browser.tabs.remote.autostart", my frame scripts load in content pages like "about:newtab". When I run the following code in the Browser Content Toolbox of an "about:newtab" page with my addon temporarily installed: ``` Components.utils.import("resource://gre/modules/Services.jsm"); let myResourceURI = Services.io.newURI("resource://button-icon-preference/content/new-tab-variation.js"); let protocolHandler = Services.io.getProtocolHandler("resource"); protocolHandler.QueryInterface(Components.interfaces.nsISubstitutingProtocolHandler); protocolHandler.resolveURI(myResourceURI); ``` I find that the location for my addon's XPI is: "jar:file:///Users/bdanforth/Projects/tracking-protection-shield-study/dist/linked-addon.xpi!/content/new-tab-variation.js"; i.e. it is being loaded from the project directory on my computer; it is not being copied into the Firefox application user profile directory as is done for a permanent addon install. If I permanently install my addon XPI using about:addons, my frame script loads in content processes pages like about:newtab just fine. Expected results: With the help of Gijs, I can conclude that if my addon XPI could be copied (temporarily) into the user profile directory when an addon is temporarily installed, rather than wherever it happens to be on my computer, (as is the case for permanent addon installations) I would be able to work within the sandbox to load frame scripts in content processes while developing and testing my bootstrapped addon.
This is expected, content processes are sandboxed and cannot read from arbitrary filesystem locations. That sandboxing restriction was introduced in 57 when bootstrapped extensions became unsupported. You have a few options, which you mentioned above: you can relax the sandbox settings while you're developing or you can package and install your extension so that it is copied into the profile directory. But in the long run, you'll be better off using a WebExtension if possible.
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
(In reply to Andrew Swan [:aswan] from comment #1) > This is expected, content processes are sandboxed and cannot read from > arbitrary filesystem locations. That sandboxing restriction was introduced > in 57 when bootstrapped extensions became unsupported. How do webextensions work around this? Don't they get sandboxed? Either way, it's very unclear *why* this doesn't work for folks working on these add-ons. Bianca explained this upfront in comment #0, but this actually took a lot of time to figure out because there's 0 feedback about why things break (silently). Can we at least improve the messaging here, if we don't want to invest any time in fixing the add-ons manager so we load temporary add-ons in a way where the sandbox doesn't break them? (Why can't we just copy them to the profile dir - wouldn't that solve this issue and not be a big deal?) At least for the foreseeable future, I don't see bootstrapped add-ons going away completely. They're used for shield add-ons as well as webextension experiments, and having the development workflow for those not be terrible in the face of sandboxing would be nice.
Flags: needinfo?(aswan)
(In reply to :Gijs from comment #2) > How do webextensions work around this? Don't they get sandboxed? Yes, but they don't use resource: urls. They use moz-extension: which gets proxied to the parent process for this very reason. > Either way, it's very unclear *why* this doesn't work for folks working on > these add-ons. Bianca explained this upfront in comment #0, but this > actually took a lot of time to figure out because there's 0 feedback about > why things break (silently). Can we at least improve the messaging here, if > we don't want to invest any time in fixing the add-ons manager so we load > temporary add-ons in a way where the sandbox doesn't break them? (Why can't > we just copy them to the profile dir - wouldn't that solve this issue and > not be a big deal?) First, sorry, I didn't mean to dismiss the fact that you spent a bunch of time tracing this. I think we should do something to avoid having that happen to others. I'm not so sure about copying extensions though. That would take a modest amount of work, the first question that comes to mind for me is how/when that copy would get removed. Also its unnecessary for folks using about:debugging to develop webextensions, which I think is the majority of users. What if instead we added a banner or other notification in about:debugging when a bootstrapped addon is loaded? That notification could suggest (or link to a wiki page that suggests) setting the sandbox pref during development. > At least for the foreseeable future, I don't see bootstrapped add-ons going > away completely. They're used for shield add-ons as well as webextension > experiments, and having the development workflow for those not be terrible > in the face of sandboxing would be nice. Yes, this is a broader issue, AndyM has scheduled some time in Austin with folks from Shield (as well as Test Pilot, developers of other system addons, etc) to work out plans in this area.
Flags: needinfo?(aswan)
Yeah, this took me a long time to figure out. Gijs was the fourth person I asked. Any kind of notice about it would probably save other developers some time and frustration. Since there are a few options (some of which are less risky than others; i.e. I should think copying the XPI to the user profile directory would be favored over lowering the sandbox security level), a linked wiki page explaining why the addon might be broken and these options would be helpful.
(In reply to Andrew Swan [:aswan] from comment #3) > (In reply to :Gijs from comment #2) > > How do webextensions work around this? Don't they get sandboxed? > > Yes, but they don't use resource: urls. They use moz-extension: which gets > proxied to the parent process for this very reason. That's... interesting on its own, for other reasons. I kinda wish we didn't do that, but I'm not familiar with all the reasons why (I assume it's not *just* the temporary add-on case?). Anyhow. > > Either way, it's very unclear *why* this doesn't work for folks working on > > these add-ons. Bianca explained this upfront in comment #0, but this > > actually took a lot of time to figure out because there's 0 feedback about > > why things break (silently). Can we at least improve the messaging here, if > > we don't want to invest any time in fixing the add-ons manager so we load > > temporary add-ons in a way where the sandbox doesn't break them? (Why can't > > we just copy them to the profile dir - wouldn't that solve this issue and > > not be a big deal?) > > First, sorry, I didn't mean to dismiss the fact that you spent a bunch of > time tracing this. I think we should do something to avoid having that > happen to others. I'm not so sure about copying extensions though. That > would take a modest amount of work, the first question that comes to mind > for me is how/when that copy would get removed. Next startup of the add-on manager, off-main-thread? We should use a separate directory (ie not staging or main add-on dir). > Also its unnecessary for > folks using about:debugging to develop webextensions, which I think is the > majority of users. As noted above, I would kind of like it not to be unnecessary, if that makes sense... would be nice if we could do these loads inside the content process instead of passing through the parent, which would require a similar solution. If we're currently marshalling everything through the parent process and IPC, that doesn't sound good for either perf or sandboxing. > What if instead we added a banner or other notification > in about:debugging when a bootstrapped addon is loaded? That notification > could suggest (or link to a wiki page that suggests) setting the sandbox > pref during development. A notification would be a start, yeah. Should we reopen this, at least for the notification? (Ni for this) > > At least for the foreseeable future, I don't see bootstrapped add-ons going > > away completely. They're used for shield add-ons as well as webextension > > experiments, and having the development workflow for those not be terrible > > in the face of sandboxing would be nice. > > Yes, this is a broader issue, AndyM has scheduled some time in Austin with > folks from Shield (as well as Test Pilot, developers of other system addons, > etc) to work out plans in this area. OK. I mean, how would this work for webextension experiments? Don't they run into the same issue? And aren't we still suggesting to people that's the best way of prototyping new API support?
Flags: needinfo?(aswan)
(In reply to :Gijs from comment #5) > (In reply to Andrew Swan [:aswan] from comment #3) > > (In reply to :Gijs from comment #2) > > > How do webextensions work around this? Don't they get sandboxed? > > > > Yes, but they don't use resource: urls. They use moz-extension: which gets > > proxied to the parent process for this very reason. > > That's... interesting on its own, for other reasons. I kinda wish we didn't > do that, but I'm not familiar with all the reasons why (I assume it's not > *just* the temporary add-on case?). Anyhow. I believe this is for "regular" webextension use, so that content processes (including both web content processes and the extension process) don't require direct access to the filesystem, which lets them run with a tighter sandbox. The proxied connection still lets a content process read any file that is inside an active WebExtension, but that is more limited than if they had access to the profile directory (or even just the extensions subdirectory). If you're curious to read more, this work happened in bug 1334550. > > > Either way, it's very unclear *why* this doesn't work for folks working on > > > these add-ons. Bianca explained this upfront in comment #0, but this > > > actually took a lot of time to figure out because there's 0 feedback about > > > why things break (silently). Can we at least improve the messaging here, if > > > we don't want to invest any time in fixing the add-ons manager so we load > > > temporary add-ons in a way where the sandbox doesn't break them? (Why can't > > > we just copy them to the profile dir - wouldn't that solve this issue and > > > not be a big deal?) > > > > First, sorry, I didn't mean to dismiss the fact that you spent a bunch of > > time tracing this. I think we should do something to avoid having that > > happen to others. I'm not so sure about copying extensions though. That > > would take a modest amount of work, the first question that comes to mind > > for me is how/when that copy would get removed. > > Next startup of the add-on manager, off-main-thread? We should use a > separate directory (ie not staging or main add-on dir). This is doable, but if its for the narrow case of developing bootstrapped extensions, which we want to deprecate anyway, I'm not sure its worth writing and maintaining that code. The fact that this blindsided you and Bianca is bad, but I don't think this is the right solution. > > Also its unnecessary for > > folks using about:debugging to develop webextensions, which I think is the > > majority of users. > > As noted above, I would kind of like it not to be unnecessary, if that makes > sense... would be nice if we could do these loads inside the content process > instead of passing through the parent, which would require a similar > solution. If we're currently marshalling everything through the parent > process and IPC, that doesn't sound good for either perf or sandboxing. Sorry to pass the buck, but I think somebody from the sandboxing team would probably be better positioned to address these questions. I would assume they did consider these tradeoffs when originally deciding to implement it this way... > > What if instead we added a banner or other notification > > in about:debugging when a bootstrapped addon is loaded? That notification > > could suggest (or link to a wiki page that suggests) setting the sandbox > > pref during development. > > A notification would be a start, yeah. Should we reopen this, at least for > the notification? (Ni for this) Sure, I'll do that here. > > > At least for the foreseeable future, I don't see bootstrapped add-ons going > > > away completely. They're used for shield add-ons as well as webextension > > > experiments, and having the development workflow for those not be terrible > > > in the face of sandboxing would be nice. > > > > Yes, this is a broader issue, AndyM has scheduled some time in Austin with > > folks from Shield (as well as Test Pilot, developers of other system addons, > > etc) to work out plans in this area. > > OK. I mean, how would this work for webextension experiments? Don't they run > into the same issue? And aren't we still suggesting to people that's the > best way of prototyping new API support? Yes, experiments are one of the methods we would like to use to prototype new APIs. At the moment, experiments can only run in the main process. Kris is working on this in bug 1323845. The WebExtensions framework has its own system for loading API code in child processes as needed which is what I assume he's planning to use, so there shouldn't be any reason to use frame scripts in experiments. I suppose in a pinch you could use a data: url?
Assignee: nobody → aswan
Status: RESOLVED → REOPENED
Component: Add-ons Manager → Developer Tools: about:debugging
Ever confirmed: true
Flags: needinfo?(aswan)
Product: Toolkit → Firefox
Resolution: WONTFIX → ---
Summary: Frame scripts do not load in content processes for a temporarily installed addon → Warn about pitfalls with temporary bootstrapped extensions
Version: 59 Branch → unspecified
Attachment #8935570 - Flags: review?(jdescottes)
Comment on attachment 8935570 [details] Bug 1423687 Show a warning link when loading a bootstrapped extension temporarily https://reviewboard.mozilla.org/r/206444/#review212320 Thanks for the patch! I think I would prefer displaying this warning inside of the addon target. With the current approach, if the user reloads the page, or goes to another category and comes back to #addons, the information is lost. By making it part of the addon warnings, the info would be displayed as long as the addon is loaded. If you are fine with changing the approach, I have a modified version of your patch which I can push here. Let me know what you think.
Attachment #8935570 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #8) > Thanks for the patch! I think I would prefer displaying this warning inside > of the addon target. > With the current approach, if the user reloads the page, or goes to another > category and comes back to #addons, the information is lost. > > By making it part of the addon warnings, the info would be displayed as long > as the addon is loaded. > If you are fine with changing the approach, I have a modified version of > your patch which I can push here. Let me know what you think. Yes, that sounds like a much better idea, please do push the patch, and thanks!
Comment on attachment 8935794 [details] Bug 1423687 - show warning in about:debugging when loading a temporary legacy extension; https://reviewboard.mozilla.org/r/206668/#review212396 ::: devtools/client/aboutdebugging/modules/addon.js:87 (Diff revision 1) > + !addonForm.isWebExtension && > + !addonForm.isAPIExtension; How likely is this to happen, I don't know? But you're checking form properties that won't be defined when connected to older server. I guess that menas we'll be showing the warning even if the extension is in fact a valid web extension.
Attachment #8935794 - Flags: review?(pbrosset)
Comment on attachment 8935794 [details] Bug 1423687 - show warning in about:debugging when loading a temporary legacy extension; https://reviewboard.mozilla.org/r/206668/#review212388 The addons-related bits look good to me. I only looked at the front-end changes superficially. I don't have the context to evaluate them in depth, I assume that's why the other reviewer is flagged. If you'd like me to take a closer look at the front-end bits, please needinfo me and I can do it. Thanks!
Attachment #8935794 - Flags: review?(aswan) → review+
Assignee: aswan → jdescottes
Comment on attachment 8935794 [details] Bug 1423687 - show warning in about:debugging when loading a temporary legacy extension; https://reviewboard.mozilla.org/r/206668/#review212548
Attachment #8935794 - Flags: review?(pbrosset) → review+
Comment on attachment 8935794 [details] Bug 1423687 - show warning in about:debugging when loading a temporary legacy extension; https://reviewboard.mozilla.org/r/206668/#review212396 > How likely is this to happen, I don't know? But you're checking form properties that won't be defined when connected to older server. > I guess that menas we'll be showing the warning even if the extension is in fact a valid web extension. Thanks for catching that. Might happen now that we support host/port parameters for about:debugging.
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c0b8f7ae6e03 show warning in about:debugging when loading a temporary legacy extension;r=aswan,pbro
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Comment on attachment 8935794 [details] Bug 1423687 - show warning in about:debugging when loading a temporary legacy extension; https://reviewboard.mozilla.org/r/206668/#review213420 ::: devtools/client/locales/en-US/aboutdebugging.properties:113 (Diff revision 2) > > +# LOCALIZATION NOTE (legacyExtensionWarning): > +# This string is displayed as a warning message when loading a temporary legacy extension. > +legacyExtensionWarning = This is a legacy extension, be aware that these are no longer fully supported. Please read the linked documentation and then proceed with caution. > + > +# LOCALIZATION NOTE (temporaryID.learnMore): Please pay attention when copying over existing comment (that's not the ID you're looking for…).
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: