Since we don't want code running in a compromised web content process to be able to access privileged extension APIs, we need to run all privileged extension code in a separate type of content process before we can turn on OOP extensions in production.

Comment on attachment 8814733 [details] Bug 1320395: Part 1 - Allow keeping non-default process types alive. https://reviewboard.mozilla.org/r/95830/#review95928 Gabor is probably the best person to review this, as I think he is still working on getting tests working for multi-process, which is why this was added originally. I don't see how I redirect the review to him though. ::: dom/ipc/ContentParent.cpp:1628 (Diff revision 1) > + nsAutoCString keepAlivePref("dom.ipc.keepProcessesAlive."); > + keepAlivePref.Append(NS_ConvertUTF16toUTF8(mRemoteType)); > + if (NS_FAILED(Preferences::GetInt(keepAlivePref.get(), &processesToKeepAlive))) { > + // Otherwise, only keep processes for the default remote type alive. > + if (!mRemoteType.EqualsLiteral(DEFAULT_REMOTE_TYPE)) { > + return false; > + } > + > + processesToKeepAlive = Preferences::GetInt("dom.ipc.keepProcessesAlive", 0); > + } Given that this is a fairly new pref, I wonder if we shouldn't just have the specific forms, for example dom.ipc.keepProcessesAlive.web You would need to update: http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/testing/mochitest/browser-test.js#112 I only put in code to continue to support dom.ipc.processCount in my changes, because the pref had been around for quite a while.

Comment on attachment 8814735 [details] Bug 1320395: Part 3 - Run WebExtensions in their own process type. f?pauljt https://reviewboard.mozilla.org/r/95834/#review95930 I don't really know the extension code at all, but billm is reviewing as well, so that should be fine. ::: browser/components/extensions/ext-utils.js:235 (Diff revision 1) > let browser = document.createElementNS(XUL_NS, "browser"); > browser.setAttribute("type", "content"); > browser.setAttribute("disableglobalhistory", "true"); > browser.setAttribute("transparent", "true"); > browser.setAttribute("class", "webextension-popup-browser"); > + browser.setAttribute("remoteType", "extension"); Shouldn't this be inside the |if (this.extension.remote) {| below? Also wouldn't it be better to be using E10SUtils to decide the remote type? So that we don't have that logic spread out over different files. Something like: let remoteType = E10SUtils.getRemoteTypeForURI(popupURL, gMultiProcessBrowser, E10SUtils.EXTENSION_REMOTE_TYPE); (Not sure if gMultiProcessBrowser is defined so might just have to pass true.) ::: dom/ipc/ContentParent.h:40 (Diff revision 1) > #define NO_REMOTE_TYPE "" > > // These must match the similar ones in E10SUtils.jsm. > #define DEFAULT_REMOTE_TYPE "web" > #define FILE_REMOTE_TYPE "file" > +#define EXTENSION_REMOTE_TYPE "extension" Not sure we need this, unless it's actually going to be used in C++ code. ::: toolkit/components/extensions/ExtensionManagement.jsm:309 (Diff revision 1) > } > > ExtensionManagement = { > get isExtensionProcess() { > - return (this.useRemoteWebExtensions || > - Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT); > + if (this.useRemoteWebExtensions) { > + return Services.appinfo.remoteType === "extension"; It would be nice if we could use E10SUtils.EXTENSION_REMOTE_TYPE here and in other places instead of just a string. ::: toolkit/components/extensions/ExtensionParent.jsm:460 (Diff revision 1) > let processMessageManager = (target.messageManager.processMessageManager || > Services.ppmm.getChildAt(0)); > > if (!extension.parentMessageManager) { > + let expectedRemoteType = extension.remote ? "extension" : null; > + if (target.remoteType === expectedRemoteType) { Should we be throwing some sort of error if this isn't true? ::: toolkit/components/extensions/ext-backgroundPage.js:55 (Diff revision 1) > let chromeDoc = yield this.getParentDocument(); > > let browser = chromeDoc.createElement("browser"); > browser.setAttribute("type", "content"); > browser.setAttribute("disableglobalhistory", "true"); > + browser.setAttribute("remoteType", "extension"); Should be inside the this.extension.remote if again I think and same thing over using E10SUtils. ::: toolkit/mozapps/extensions/content/extensions.js:3538 (Diff revision 1) > let browser = document.createElement("browser"); > browser.setAttribute("type", "content"); > browser.setAttribute("disableglobalhistory", "true"); > browser.setAttribute("class", "inline-options-browser"); > + browser.setAttribute("remoteType", "extension"); > Same here. As an aside, I don't like the fact that lots of different places have to know the correct attribute to set. It would be nice if browser had a method for it, but as I understand it we don't have the XBL bindings until the element has been added into the DOM.

Comment on attachment 8814735 [details] Bug 1320395: Part 3 - Run WebExtensions in their own process type. f?pauljt https://reviewboard.mozilla.org/r/95834/#review95930 > Shouldn't this be inside the |if (this.extension.remote) {| below? > > Also wouldn't it be better to be using E10SUtils to decide the remote type? > So that we don't have that logic spread out over different files. > Something like: > > let remoteType = E10SUtils.getRemoteTypeForURI(popupURL, gMultiProcessBrowser, E10SUtils.EXTENSION_REMOTE_TYPE); > > (Not sure if gMultiProcessBrowser is defined so might just have to pass true.) It occurred to me to put it in the `if` when I was looking over the patch, but the attribute doesn't actually have any effect if the browser isn't remote, so it shouldn't really make a difference. I'm happy to move it if you think I should, though. As for using E10SUtils, it did occur to me, but that adds a lot of complications. For instance, we'd have to support redirecting remote loads in these browsers. Since they're only actually intended to load extension URLs, it didn't seem worth the added complexity. > Not sure we need this, unless it's actually going to be used in C++ code. I mainly just added it for the sake of completeness, so anyone reading this code knows that the type exists. Eventually, though, we're also going to need separate sandboxing rules for extension processes, so it will be used at that point. > Should we be throwing some sort of error if this isn't true? The check a few lines below will throw if this winds up being true, since the message manager won't wind up being set, and therefore won't match.

Comment on attachment 8814733 [details] Bug 1320395: Part 1 - Allow keeping non-default process types alive. https://reviewboard.mozilla.org/r/95830/#review95928 I think that you just have to click the pencil in the reviewers column and change the list there. > Given that this is a fairly new pref, I wonder if we shouldn't just have the specific forms, for example dom.ipc.keepProcessesAlive.web > > You would need to update: > http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/testing/mochitest/browser-test.js#112 > > I only put in code to continue to support dom.ipc.processCount in my changes, because the pref had been around for quite a while. Sounds good to me. I actually considered that, but decided on trying to keep the changes minimal.

Comment on attachment 8814735 [details] Bug 1320395: Part 3 - Run WebExtensions in their own process type. f?pauljt https://reviewboard.mozilla.org/r/95834/#review96462

Comment on attachment 8814735 [details] Bug 1320395: Part 3 - Run WebExtensions in their own process type. f?pauljt https://reviewboard.mozilla.org/r/95834/#review95930 > It occurred to me to put it in the `if` when I was looking over the patch, but the attribute doesn't actually have any effect if the browser isn't remote, so it shouldn't really make a difference. I'm happy to move it if you think I should, though. > > As for using E10SUtils, it did occur to me, but that adds a lot of complications. For instance, we'd have to support redirecting remote loads in these browsers. Since they're only actually intended to load extension URLs, it didn't seem worth the added complexity. It may not in this case, but it should only really be set when we are remote. As for E10SUtils, you're probably right that it's not worth the effort. It would just be using the one method and it should always return E10SUtils.EXTENSION_REMOTE_TYPE or NOT_REMOTE anyway. Not sure you'd have to support redirecting remote loads because of that though. As I said, it would be nice if we could use E10SUtils.EXTENSION_REMOTE_TYPE instead of the string, but I'm not sure how much overhead including the jsm is just for that. So not a big deal if we can't. > I mainly just added it for the sake of completeness, so anyone reading this code knows that the type exists. Eventually, though, we're also going to need separate sandboxing rules for extension processes, so it will be used at that point. Good point, CC me on the bug once we're ready to think about sandboxing this differently. (Or I guess that might happen when we tighten the standard rules to a point where they break extensions in this process.) > The check a few lines below will throw if this winds up being true, since the message manager won't wind up being set, and therefore won't match. Ah right, sorry I missed that.

Comment on attachment 8814733 [details] Bug 1320395: Part 1 - Allow keeping non-default process types alive. https://reviewboard.mozilla.org/r/95830/#review97964 I'm sorry for this review taking so long... and thanks for keeping me in the loop. I have only one request, this whole keep process alive thing is just a hack to avoid test timeouts for the debug builds. If I get it right for extensions we will use it for the same purpose, could you please add a short comment that makes it explicit that these flags are for test only?

Comment on attachment 8814733 [details] Bug 1320395: Part 1 - Allow keeping non-default process types alive. https://reviewboard.mozilla.org/r/95830/#review97964 Yes, they're only for the sake of tests here, too. In this case, it's not so much for the sake of timeouts in debug builds as overall test run time in general, since without it we wind up spawining and destroying a new content process every time we load and unload a test extension (which we do a lot). I'll update the comment to reflect that.

Comment on attachment 8814734 [details] Bug 1320395: Part 2 - Assign extension's process message manager based on first view load. https://reviewboard.mozilla.org/r/95832/#review98434 ::: toolkit/components/extensions/Extension.jsm:933 (Diff revision 1) > > this.cleanupGeneratedFile(); > } > > observe(subject, topic, data) { > - if (topic == "xpcom-shutdown") { > + if (topic === "xpcom-shutdown") { Won't this always be a string? ::: toolkit/components/extensions/ExtensionParent.jsm:458 (Diff revision 1) > let context; > if (envType == "addon_parent" || envType == "devtools_parent") { > + let processMessageManager = (target.messageManager.processMessageManager || > + Services.ppmm.getChildAt(0)); > + > + if (!extension.parentMessageManager) { This seems scary to me since it seems like a rogue content process could win a race and get access to extension APIs. I would rather we try to enumerate all <browser> elements that might be associated with the extension and check their processMessageManager. Is there a reason that wouldn't work?

Comment on attachment 8814735 [details] Bug 1320395: Part 3 - Run WebExtensions in their own process type. f?pauljt https://reviewboard.mozilla.org/r/95834/#review98436

Comment on attachment 8814734 [details] Bug 1320395: Part 2 - Assign extension's process message manager based on first view load. https://reviewboard.mozilla.org/r/95832/#review98434 > Won't this always be a string? Yes, but most of our code has been moving to using === by default lately. I only changed this because I originally added another observer to this object, and I apparently forgot to revert this change when I removed it. > This seems scary to me since it seems like a rogue content process could win a race and get access to extension APIs. I would rather we try to enumerate all <browser> elements that might be associated with the extension and check their processMessageManager. Is there a reason that wouldn't work? There's currently only ever one process that can host extension content. As of this patch, it's the (single) web content process. As of the next one, it's a separate extension process (which we verify). We can't enumerate all extension browsers because, 1) there aren't always going to be browsers with extension content, and 2) extension content can be loaded into ordinary tabs. But we do ensure that all extension content is in the right process, and the right process type. If/when we start supporting multiple extension content processes, this is going to get more complicated, but for now, all we need to be sure of is the process type.

(In reply to Kris Maglione [:kmag] from comment #15) > > This seems scary to me since it seems like a rogue content process could win a race and get access to extension APIs. I would rather we try to enumerate all <browser> elements that might be associated with the extension and check their processMessageManager. Is there a reason that wouldn't work? > > There's currently only ever one process that can host extension content. As > of this patch, it's the (single) web content process. As of the next one, > it's a separate extension process (which we verify). We can't enumerate all > extension browsers because, 1) there aren't always going to be browsers with > extension content, If we're asked to create a proxy context, then there should be a <browser>, right? > and 2) extension content can be loaded into ordinary > tabs. That's annoying, but could we listen for new <browser>s added to the DOM, link them to extensions via the UUID, and set the parent process message manager that way? > But we do ensure that all extension content is in the right process, > and the right process type. > > If/when we start supporting multiple extension content processes, this is > going to get more complicated, but for now, all we need to be sure of is the > process type. I feel like this is something we should get right from the start.

(In reply to Bill McCloskey (:billm) from comment #16) > (In reply to Kris Maglione [:kmag] from comment #15) > > > This seems scary to me since it seems like a rogue content process could win a race and get access to extension APIs. I would rather we try to enumerate all <browser> elements that might be associated with the extension and check their processMessageManager. Is there a reason that wouldn't work? > > > > There's currently only ever one process that can host extension content. As > > of this patch, it's the (single) web content process. As of the next one, > > it's a separate extension process (which we verify). We can't enumerate all > > extension browsers because, 1) there aren't always going to be browsers with > > extension content, > > If we're asked to create a proxy context, then there should be a <browser>, > right? Yes, but at that point, either this is the first browser, and we're setting process message manager based on it, or the process message manager has already been set, and we check this one against it. > > and 2) extension content can be loaded into ordinary > > tabs. > > That's annoying, but could we listen for new <browser>s added to the DOM, > link them to extensions via the UUID, and set the parent process message > manager that way? Well, those browsers aren't actually related to the extension until an extension URL gets loaded into them, and they stop being related once they navigate away. > > But we do ensure that all extension content is in the right process, > > and the right process type. > > > > If/when we start supporting multiple extension content processes, this is > > going to get more complicated, but for now, all we need to be sure of is the > > process type. > > I feel like this is something we should get right from the start. I think we are getting it right from the start. As long as there's only one extension process, that process's message manager is the right one for any extension. Until we figure out how we're going to group all browsers for a given extension into a single process, there isn't really anything more to be done.

(In reply to Kris Maglione [:kmag] from comment #17) > > If we're asked to create a proxy context, then there should be a <browser>, > > right? > > Yes, but at that point, either this is the first browser, and we're setting > process message manager based on it, or the process message manager has > already been set, and we check this one against it. OK, I agree. > > That's annoying, but could we listen for new <browser>s added to the DOM, > > link them to extensions via the UUID, and set the parent process message > > manager that way? > > Well, those browsers aren't actually related to the extension until an > extension URL gets loaded into them, and they stop being related once they > navigate away. When that happens, the browser will be removed from the DOM and added back, so we'll get a different frameloader and message manager. Thinking about it, though, the URL might not be available right away. > > I feel like this is something we should get right from the start. > > I think we are getting it right from the start. As long as there's only one > extension process, that process's message manager is the right one for any > extension. Until we figure out how we're going to group all browsers for a > given extension into a single process, there isn't really anything more to be > done. All right, I guess it can stay the way it is. It seems hokey to me, but I guess we're not likely to add more extension processes soon.

Bill, does part 2 need any other changes or did you mean to r+ it?

Comment on attachment 8814734 [details] Bug 1320395: Part 2 - Assign extension's process message manager based on first view load. https://reviewboard.mozilla.org/r/95832/#review105382 Sorry.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c59832caf8dbfb88f48e7a336702891cbae0b913 Bug 1320395: Part 1 - Allow keeping non-default process types alive. r=bobowen,gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/a1eb055f4caf5cb16c9630ce7fcb4704be4cb74b Bug 1320395: Part 2 - Assign extension's process message manager based on first view load. r=billm https://hg.mozilla.org/integration/mozilla-inbound/rev/353d75e97b1e3e98795512fe9e783b01324eb6c1 Bug 1320395: Part 3 - Run WebExtensions in their own process type. r=billm,bobowen

Kris, what's the targeted version for OOP extensions release?

https://hg.mozilla.org/mozilla-central/rev/c59832caf8db https://hg.mozilla.org/mozilla-central/rev/a1eb055f4caf https://hg.mozilla.org/mozilla-central/rev/353d75e97b1e

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22) > Kris, what's the targeted version for OOP extensions release? I'm unofficially aiming for 55, but that requires dependencies completed by other people at mozilla.

Ideally, I'd like to turn it on in nightly 44, but we're currently blocked on bug 1287004 and bug 1302702.

Obviously, I meant 54.

Comment on attachment 8814735 [details] Bug 1320395: Part 3 - Run WebExtensions in their own process type. f?pauljt Clearing flag since I dont have anything to add. Thanks for heads up though.