Based on the fact that we are deprecating JS-implemented WebIDL (See Bug 1450827) and also after quite so many sessions with front end engineers I think we should not rewrite AboutCapabilities in C++ (which will definitely hurt adoption) but rather have about pages rely on RPM (probably with some improvements as we go along). FWIW, this bug will render Bug 1457458 to become a WONTFIX.

Dave, thanks for all the valuable discussions and feedback. I managed to have about:privatebrowsing rely on RPM instead of AboutCapabilities which allows us to completely deprecate AboutCapabilities. As you can see in the patch, there is quite so much boiler plate code on the consumer end though. E.g. when you take a look at aboutPrivatebrowsing.js I had to implement setBoolPref, getBoolPref, etc. As far as I can tell RemotePageManager basically only allows async message passing, or am I missing something? Would it make sense to incorporate something like 'queryOnce' within RPM which would allow to e.g. query a Pref? Going forward, I guess we could incorporate some minor improvements to RPM. E.g. I still like the idea of having a whitelist of the consumers of that API. Currently one registers a new consumer simply by performing |new RemotePages(URI)|. I guess we could add a whitelist within RPM that checks that any consumers are actually allowed to use it and something there like. What do you think?

Full support from my side for some helper library that reduces the message passing boilerplate going on in content land. Something like let isPrivate = await RPMContentUtils.queryOnce("IsWindowPrivate") would get us much closer to the ergonomics that the webidl solution admittedly provided to content code.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1) > Created attachment 8973221 [details] [diff] [review] > bug_1459204_about_privatebrowsing_rpm.patch > > Dave, > > thanks for all the valuable discussions and feedback. I managed to have > about:privatebrowsing rely on RPM instead of AboutCapabilities which allows > us to completely deprecate AboutCapabilities. As you can see in the patch, > there is quite so much boiler plate code on the consumer end though. E.g. > when you take a look at aboutPrivatebrowsing.js I had to implement > setBoolPref, getBoolPref, etc. As far as I can tell RemotePageManager > basically only allows async message passing, or am I missing something? > Would it make sense to incorporate something like 'queryOnce' within RPM > which would allow to e.g. query a Pref? I think there's a lot of scope for improvements. Certainly a common case is querying something and getting a response. We could easily build a queryMessage API that returns a promise that resolves when the parent responds to it. > Going forward, I guess we could incorporate some minor improvements to RPM. > E.g. I still like the idea of having a whitelist of the consumers of that > API. Currently one registers a new consumer simply by performing |new > RemotePages(URI)|. I guess we could add a whitelist within RPM that checks > that any consumers are actually allowed to use it and something there like. I'm not sure why that's necessary. Isn't declaring it in the parent essentially whitelisting it? Other pages shouldn't get the API injected into them.

(In reply to Dave Townsend [:mossop] from comment #3) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #1) > > Going forward, I guess we could incorporate some minor improvements to RPM. > > E.g. I still like the idea of having a whitelist of the consumers of that > > API. Currently one registers a new consumer simply by performing |new > > RemotePages(URI)|. I guess we could add a whitelist within RPM that checks > > that any consumers are actually allowed to use it and something there like. > > I'm not sure why that's necessary. Isn't declaring it in the parent > essentially whitelisting it? Other pages shouldn't get the API injected into > them. The point as I see it is that we want to have a central list of "thingies that punch through our sandbox". What I had imagined when we talked about whitelisting was a central list of all outgoing and incoming messages that RPM will allow through, e.g. // browser/base/content/RPMMessages.js RemotePageManager.setGlobalWhitelist({ "about:privatebrowsing": { incoming: [ "DontShowIntroPanelAgain", "OpenPrivateWindow", "GetIsWindowPrivate", ], outgoing: [ "ReceiveIsWindowPrivate", ], }, ... }); This way we have a nice list in a central place (that's not hard to modify and doesn't require any special peer review). In theory we could even use this save all the boilerplate that's used to add message listeners in all the places that use RPM. This would be based on the guarantee that RPM only injects into the right pages, of course.

I think there are a bunch of opportunities to improve RPM, but probably we should do that in separate bug. I filed Bug 1459909 to do that. Let's skip that discussion for now within this Bug. @mossop: I'll flag you for review once Gijs is fine with the changes. @Gijs: The attached patch deprecates AboutCapabilities and have about:privatebrowsing rely on RPM. For now that patch includes some boilerplate like e.g. getBoolPref which ideally we should hide/incorporate within RPM. To keep this patch simple and also to just have it rely on RPM, let's do those follow ups as one of the dependencies for Bug 1459909. Last question: Is |let win = BrowserWindowTracker.getTopWindow();| the way to query the window within AboutPrivateBrowsingHandler.jsm or is there a better way to do that?

Comment on attachment 8973996 [details] [diff] [review] bug_1459204_about_privatebrowsing_rpm.patch Review of attachment 8973996 [details] [diff] [review]: ----------------------------------------------------------------- There's a few more simplifications we can apply with this approach, see comments - but those are relatively easy/minor adjustments of the patch. A more major concern is the lack of sync access to same-process APIs ("is this window private", pref reads, format URL prefs) in the content process. All the async-ness is likely to introduce flicker where the page doesn't finish initializing before we do the first paint. We should do our best to avoid that. Hopefully Mossop has suggestions - I don't see any good way for RPM to allow same-process privileged lambdas or something like that. Perhaps we can add the ones we need. ::: browser/components/about/AboutPrivateBrowsingHandler.jsm @@ +43,5 @@ > + Services.obs.notifyObservers(null, "about-privatebrowsing-open-window"); > + break; > + } > + case "DontShowIntroPanelAgain": { > + Services.obs.notifyObservers(null, "about-privatebrowsing-do-not-show-intro-panel"); You should be able to collapse the code from the AboutPageListener in browser.js into here (and remove that listener). @@ +53,5 @@ > + break; > + } > + case "SetBoolPref": { > + AsyncPrefs.set(aMessage.data.aPref, aMessage.data.aVal).then(function() { > + this.pageListener.sendAsyncMessage("ReceiveGetBoolPref"); You're in the parent process, so you don't need to use AsyncPrefs anymore. Please remove the pref from the whitelist in AsyncPrefs, and instead create your own whitelist here. We shouldn't allow a message from the content process to set arbitrary prefs - only the ones that we intend to be settable from content (in this case, the TP one from the checkbox in about:privatebrowsing). @@ +64,5 @@ > + break; > + } > + case "GetIsWindowPrivate": { > + let win = BrowserWindowTracker.getTopWindow(); > + let isPrivate = PrivateBrowsingUtils.isWindowPrivate(win); So rather than doing this, I believe you can use `aMessage.port` which has a browser object, which is the browser in which the about:privatebrowsing instance lives (which might not be the topmost browser window). You probably also want to use this for the `OpenBrowserWindow({private: true});` call. ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js @@ +60,2 @@ > let trackingEnabled = globalTrackingEnabled || > + await getBoolPref(TP_PB_ENABLED_PREF); It's pretty unfortunate that this (and the formatURLPref stuff, and `isWindowPrivate`) now requires a cross-process roundtrip, thus potentially causing a flash of unstyled/incomplete content while we wait for the IPC. I take it with RPM we don't have a good way of getting this data from within the content process? Maybe Mossop has ideas on how that's supposed to work in cases like this. @@ +86,5 @@ > }); > + let startTourURLPref = await getFormatURLPref("privacy.trackingprotection.introURL"); > + document.getElementById("startTour").setAttribute("href", startTourURLPref); > + > + let learnMoreURLPref = await getFormatURLPref("privacy.trackingprotection.introURL"); Copy-paste issue? Looks like this should use `app.support.baseURL`.

Gijs, thanks for your speedy response; I incorporated your suggestions, however I still have some questions: a) I see we can remove 'about-privatebrowsing-do-not-show-intro-panel' but we still need to call into browser.js for opening the private window, right? b) FWIW, I was relying on AsyncPrefs.jsm because of the whitelist. Anyway, maintaining our own whitelist makes sense to me. Should we log something for developers in case they forgot to whitelist their pref? I would say yes, but I don't know what's the right way of logging is. c) Unfortunately 'aMessage.port' is undefined. I also tried |aMessage.target.ownerGlobal| which is also undefined. Any other suggestion how I can query the window? d) The linter complains within aboutPrivateBrowsing.js because e.g. addMessageListener(), which is injected through RPM is not defined. What is the right way of handling that? Thanks!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8) > Created attachment 8974059 [details] [diff] [review] > bug_1459204_about_privatebrowsing_rpm.patch > > Gijs, > thanks for your speedy response; I incorporated your suggestions, however I > still have some questions: > a) I see we can remove 'about-privatebrowsing-do-not-show-intro-panel' but > we still need to call into browser.js for opening the private window, right? No, once you get a browser window you can just call myBrowserWindowRef.OpenBrowserWindow({private: true}) directly. > b) FWIW, I was relying on AsyncPrefs.jsm because of the whitelist. Anyway, > maintaining our own whitelist makes sense to me. Should we log something for > developers in case they forgot to whitelist their pref? I would say yes, but > I don't know what's the right way of logging is. Cu.reportError would be fine. > c) Unfortunately 'aMessage.port' is undefined. I also tried > |aMessage.target.ownerGlobal| which is also undefined. Any other suggestion > how I can query the window? Sorry, from re-reading code + docs, I guess you need `aMessage.target.browser` to get the browser, and then `aMessage.target.browser.ownerGlobal` to get the window -- but I don't have a setup right now to verify that works. If that doesn't work, I'd recommend using the browser toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox ), setting a breakpoint in its debugger in your listener, and inspecting `aMessage`, to see how to get the browser and its window. > d) The linter complains within aboutPrivateBrowsing.js because e.g. > addMessageListener(), which is injected through RPM is not defined. What is > the right way of handling that? Adding the comment: /* eslint-env mozilla/frame-script */ at the top of the file should fix this. That seems to be what https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutTabCrashed.js does, anyway. I'm not entirely convinced that's better than marking the things RPM exports into the scope of the window explicitly, but it'll work for now and be consistent, so let's go with that.

@Gijs: Thanks, that all seems to work now. @Dave: Please also see questions from Gijs in comment 7. Thanks!

Comment on attachment 8974125 [details] [diff] [review] bug_1459204_about_privatebrowsing_rpm.patch Review of attachment 8974125 [details] [diff] [review]: ----------------------------------------------------------------- I think we need a solution to get some of this stuff sync within the content process. Hopefully Mossop has some ideas there. If this isn't possible, we should probably make some of the load handling not block on getting certain bits of information, and make sure page rendering is consistent and doesn't flicker too much as a result. ::: browser/components/about/AboutPrivateBrowsingHandler.jsm @@ +49,5 @@ > + win.OpenBrowserWindow({private: true}); > + break; > + } > + case "DontShowIntroPanelAgain": { > + TrackingProtection.dontShowIntroPanelAgain(); // eslint-disable-line Err, I think you need a `win` reference for this, too, and that's why eslint complains... :-) @@ +59,5 @@ > + break; > + } > + case "SetBoolPref": { > + let pref = aMessage.data.aPref; > + if (this._allowedPrefs.indexOf(pref) > -1) { Use `.includes(pref)` Did you run eslint? I thought we had a rule for this now... ( bug 1339461). If the rule is not working maybe we need to fix it. Or maybe the eslint configuration for this directory isn't great, or something?

(In reply to :Gijs (he/him) from comment #11) > I think we need a solution to get some of this stuff sync within the content > process. Hopefully Mossop has some ideas there. If this isn't possible, we > should probably make some of the load handling not block on getting certain > bits of information, and make sure page rendering is consistent and doesn't > flicker too much as a result. Mossop, do you have any suggestions regarding make this stuff sync? > ::: browser/components/about/AboutPrivateBrowsingHandler.jsm > > + TrackingProtection.dontShowIntroPanelAgain(); // eslint-disable-line > > Err, I think you need a `win` reference for this, too, and that's why eslint > complains... :-) Indeed, that also made eslint shut up. > @@ +59,5 @@ > > + if (this._allowedPrefs.indexOf(pref) > -1) { > > Use `.includes(pref)` > > Did you run eslint? Eslint didn't complain; either way, now I am using includes(pref), which seems to work fine. But we need to do something about the flickerness. I guess that's also why the test browser_privatebrowsing_about.js is still failing, see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75da292efa93fa851aa9ebacb8e7dbc0da70d58b

That might be a problem. RPM was never designed to allow for sync access to anything. I guess we could expose some APIs by sending down functions ala ContentTask but that seems a little hacky.

(In reply to Dave Townsend [:mossop] from comment #13) > That might be a problem. RPM was never designed to allow for sync access to > anything. I guess we could expose some APIs by sending down functions ala > ContentTask but that seems a little hacky. Gijs, any suggestions on how to move forward in that case? Should we try to get the patch landed as is and then start iterating on improvements to RPM?

If Mossop's OK with it, I think I would suggest: 1. add an option to the remotepagemanager constructor that passes an object with properties for the following: a) an optional array of prefs the page should be able to read b) an optional bool indicating whether the page should be able to determine if it's in private browsing mode. 2. expose 3 helper functions to the page in addition to the frame message managing ones that RemotePageChannel currently exposes - getting a bool pref. Check against the list of allowed prefs - getting a formatted string from a pref (again, check against list of allowed prefs) - whether the page is in PB mode (only if the parent gave permission for this) You can do this by using `exportFunction` as it's done here: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/RemotePageManager.jsm#409-420 . To implement this, AFAICT you could pass the relevant information in the RemotePage:Register message (and store in the initialProcessData blob), and change `registeredUrls` to a Map instead of Set, storing the config options in there per-url, then passing them to the ChildMessagePort constructor when that's called. The pref writes / window opening can stay as async messages, AFAICT. Dave, how does that sound?

(In reply to :Gijs (he/him) from comment #15) > If Mossop's OK with it, I think I would suggest: > > 1. add an option to the remotepagemanager constructor that passes an object > with properties for the following: > a) an optional array of prefs the page should be able to read > b) an optional bool indicating whether the page should be able to determine > if it's in private browsing mode. > > 2. expose 3 helper functions to the page in addition to the frame message > managing ones that RemotePageChannel currently exposes > > - getting a bool pref. Check against the list of allowed prefs > - getting a formatted string from a pref (again, check against list of > allowed prefs) > - whether the page is in PB mode (only if the parent gave permission for > this) My worry is that this sort of API call is going to balloon, but maybe this one is likely useful in a few places so let's go with it and see where it gets us. > Dave, how does that sound? Sounds good!

Gijs, thanks for your suggestions. I thought about this a little more and I think what we really should try to end up with is a more declarative security model! Let me propose the following which I also incorporated in the attached patch (in no particular order): a) I added an RPMAccessManager which holds a whitelist of what URI has what permissions. I would prefer that over extending the RemotePages() constructor by a second argument which holds that list; for two reasons: First, if someone manages to be allowed to call RemotePages() that someone could give that URI all the permissions through the constructor. Second, we have a whitelist baked into RPM which requires explicit whitelisting of what URI has what permissions which translates into security by default. b) I added helper functions for getBoolPref, setBoolPref and getFormatURLPref. All of the three helpers first check if the requested feature is whitelisted within RPMAccessManager. Doing so I think it also makes sense to keep setBoolPref generic and within RPM instead of remoting it within AboutPrivateBrowsingHandler.jsm. First, it's a very generic function that most likely will be used but quite so many about pages and second it falls back to security checks of RPMAccessManager which denies access by default. In comparision, someone adding a new AboutForManager.jsm file (like we did for aboutPrivateBrowsing) would have to explicitly add a check to make sure the pref is allowed to be flipped. c) The exported functions are preprepended by "RPM", e.g. RMPSetBoolPref() which simplifies grepping for it within our codebase. Ultimately I think we should also do that for sendAsyncMessage and other imported functions which will simplify security audits. In contrast grepping for just 'sendAsyncMessage' returns quite so many results which makes such audits cumbersome. d) I am not sure if the declarative model should also incorporate all the messages that asyncMessage is allowed to receive. I think that's not necessary, but I could be convinced otherwise. Possible downsides: *) I am relying on AsyncPrefs.jsm which requires whitelisting of prefs not only within RPM but also within AsyncPrefs.jsm. A tradeoff I am willing to take. *) Declaring all the permission within RPMAccessManager might ballon, but I could imagine us moving that part into a different file to adhere to the guidelines of separation of concerns once it gets too big. Notes about the patch: *) You suggested to also incorporate the bit about whether a window is private or not within RPM. I think that part is too specific to about:privatebrowsing and hence I kept that computation as is within aboutPrivateBrowising.js. I know it's not that great to have that sync needed information to rely on async message passing, but in my opinion it's too specific for that use case to add it to RPM. What do you think?

Dave, I forgot to flag you for ni? on comment 17. What's your take on my suggestions?

Comment on attachment 8979654 [details] [diff] [review] bug_1459204_about_privatebrowsing_rpm.patch Review of attachment 8979654 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/RemotePageManager.jsm @@ +17,5 @@ > + * > + * Please note that prefs whitelisted within setBoolPref need to be additionally > + * whitelisted within AsyncPrefs.jsm which handles the actual pref flip for RPM. > + */ > +function RPMAccessManager() {} Why is this a function as opposed to a plain object?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18) > > +function RPMAccessManager() {} > > Why is this a function as opposed to a plain object? No particular reason. This is more of a strawman patch and I am happy to work out such details once we all agree to go down that route. What's important is the declarative model I am proposing. I am more than happy to incorporate suggestions to that patch and proposal.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17) > Let me propose the following which I also incorporated in the attached patch > (in no particular order): > > a) I added an RPMAccessManager which holds a whitelist of what URI has what > permissions. I would prefer that over extending the RemotePages() > constructor by a second argument which holds that list; for two reasons: > First, if someone manages to be allowed to call RemotePages() that someone > could give that URI all the permissions through the constructor. Surely we only allow RemotePages() to be constructed in the parent (if not, we can add a check for this, unless I'm misunderstanding something...) and so the ability to call RemotePages() implies RCE in the parent process, at which point we've pretty much already lost... In fact, even if RemotePages were constructable in the content process, again, being able to call the constructor implies chrome-level RCE in the content process so then they could just call the exposed APIs (getting prefs etc.) directly anyway. > Second, we > have a whitelist baked into RPM which requires explicit whitelisting of what > URI has what permissions which translates into security by default. I think this makes sense, but I have to admit I don't see much difference with passing this to the RemotePages constructor - presumably any allowed things would have to be passed explicitly to the constructor, which would still be security by default, no? The only real benefit I see (which may be large enough to justify doing what you're doing anyway!) is avoiding passing the permissions state from the parent to the child. It's much easier if the child has access to it to begin with... > b) I added helper functions for getBoolPref, setBoolPref and > getFormatURLPref. All of the three helpers first check if the requested > feature is whitelisted within RPMAccessManager. Doing so I think it also > makes sense to keep setBoolPref generic and within RPM instead of remoting > it within AboutPrivateBrowsingHandler.jsm. First, it's a very generic > function that most likely will be used but quite so many about pages and > second it falls back to security checks of RPMAccessManager which denies > access by default. In comparision, someone adding a new AboutForManager.jsm > file (like we did for aboutPrivateBrowsing) would have to explicitly add a > check to make sure the pref is allowed to be flipped. Yeah, there'll be other consumers that will want set*Pref, like the about:neterror/about:certerror "fix my prefs that disable too many TLS algorithms" thingy. > c) The exported functions are preprepended by "RPM", e.g. RMPSetBoolPref() > which simplifies grepping for it within our codebase. Ultimately I think we > should also do that for sendAsyncMessage and other imported functions which > will simplify security audits. In contrast grepping for just > 'sendAsyncMessage' returns quite so many results which makes such audits > cumbersome. This makes sense to me. > d) I am not sure if the declarative model should also incorporate all the > messages that asyncMessage is allowed to receive. I think that's not > necessary, but I could be convinced otherwise. I think it's fine not to do this, as AFAIK they're restricted to the message channel stuff set up via RPM anyway, right? That is, the page can't use sendAsyncMessage to send any messages that then call "real" message manager listeners in the parent, as it's all via a different channel/port. > Possible downsides: > *) I am relying on AsyncPrefs.jsm which requires whitelisting of prefs not > only within RPM but also within AsyncPrefs.jsm. A tradeoff I am willing to > take. The threat model for the checks in AsyncPrefs is a compromised content process. If we think it's worth having a separate layer of checks for a compromised about page (without process-level compromise in the child) then I guess we can add checks, but tbh I'm not sure it's worth it at the moment, given how much of our exposure to web content is C++ and the number of bugs I'm aware of that try to escalate from JS execution in a content-privileged about: page to anything else (ie 0)... If the number of prefs we access this way balloons, I could be convinced to reconsider. One thing we could consider is adding an API to AsyncPrefs so that in the parent we can add new prefs, so the list of allowed prefs doesn't need to be in 1 place. Then RPM code in the parent could just add the relevant pref names and you wouldn't need to whitelist them twice. I'm not sure this is worth the effort, but it seems possible. I think the reason I didn't do this originally was that I was paranoid that people would use it as a way to then have the content process send the parent a message "please add these prefs to the whitelist" etc. But "paranoia" is probably an accurate description for that thought. :-) > *) Declaring all the permission within RPMAccessManager might ballon, but I > could imagine us moving that part into a different file to adhere to the > guidelines of separation of concerns once it gets too big. I'm not super worried about this so far. > Notes about the patch: > *) You suggested to also incorporate the bit about whether a window is > private or not within RPM. I think that part is too specific to > about:privatebrowsing and hence I kept that computation as is within > aboutPrivateBrowising.js. I know it's not that great to have that sync > needed information to rely on async message passing, but in my opinion it's > too specific for that use case to add it to RPM. I think that "are we in private browsing" is likely to be generically useful for other in-content pages as well, tbh (e.g. to make activity stream, which loads in the private window if you press the home button and haven't changed your homepage, behave differently in PB). It's a simple enough helper call, and there are already "unscientific" ways of determining this from web content, see e.g. the codepen linked from bug 1366318, so the risk of exposing it seems very minimal. If it's easier, we could expose it without the permissions management, and just expose it to every content-privileged about: page (that isn't about:blank/srcdoc, of course).

(In reply to :Gijs (no reviews; away 25/5-5/6 inclusive; he/him) from comment #21) > > have a whitelist baked into RPM which requires explicit whitelisting of what > > URI has what permissions which translates into security by default. > > I think this makes sense, but I have to admit I don't see much difference > with passing this to the RemotePages constructor - presumably any allowed > things would have to be passed explicitly to the constructor, which would > still be security by default, no? Sure. The other benefit is that we would have one matrix with permissions in one central point instead of having to browse the codebase for security audits searching for the constructor of each consumer of RemotePages. > The only real benefit I see (which may be large enough to justify doing what > you're doing anyway!) is avoiding passing the permissions state from the > parent to the child. It's much easier if the child has access to it to begin > with... I am not entirely sure what you mean with that, but if that argument makes you lean towards my approach, then I am happy that you found it :-) > Yeah, there'll be other consumers that will want set*Pref, like the > about:neterror/about:certerror "fix my prefs that disable too many TLS > algorithms" thingy. So, I guess we agree here and we keep the helper function for setBoolPref within RPM. Probably we then also want setCharPref in RPM, but I think we should only add that when needed. > > c) The exported functions are preprepended by "RPM", e.g. RMPSetBoolPref() > > which simplifies grepping for it within our codebase. Ultimately I think we > > should also do that for sendAsyncMessage and other imported functions which > > will simplify security audits. In contrast grepping for just > > 'sendAsyncMessage' returns quite so many results which makes such audits > > cumbersome. > > This makes sense to me. FWIW, I filed Bug 1463663 to get that done for the exported functions we are not touching within this bug. > > Possible downsides: > > *) I am relying on AsyncPrefs.jsm which requires whitelisting of prefs not > > only within RPM but also within AsyncPrefs.jsm. A tradeoff I am willing to > > take. > > The threat model for the checks in AsyncPrefs is a compromised content > process. If we think it's worth having a separate layer of checks for a > compromised about page (without process-level compromise in the child) then > I guess we can add checks, but tbh I'm not sure it's worth it at the moment, I agree with that. So what I'll do is add a comment on top of the file explaining how to use RPM and what needs to be whitelisted and where. E.g. no need to whitelist prefs to be flipped within RPM and AsyncPrefs.jsm, but only in AsyncPrefs.jsm. > I think that "are we in private browsing" is likely to be generically useful > for other in-content pages as well, tbh (e.g. to make activity stream, which > loads in the private window if you press the home button and haven't changed > your homepage, behave differently in PB). I don't have a strong opinion tbh. I will add a helper function to RPM to check if a window is in private browsing mode or not.

Dave, I chatted with Gijs and he is fine with the approach in this patch. He is going to be on leave for a while so he was wondering if you could review this patch. Please note that I filed a follow up to prepend other exported functions of RPM with 'RPM', see Bug 1463663. Thanks!

Comment on attachment 8979909 [details] [diff] [review] bug_1459204_about_privatebrowsing_rpm.patch Review of attachment 8979909 [details] [diff] [review]: ----------------------------------------------------------------- I have two initial comments here that are worth thinking about before I review further. They are unfortunately at odds with each other so we can't really satisfy both but... If we're concerned about being able to see the capabilities of about: pages in one central place then it may make sense to go one step further and just list all the RPM urls in one place along with any boilerplate necessary to get the main process side ready. This would mean we wouldn't need to load AboutPrivateBrowsingHandler.jsm until it was actually needed for example. How do system add-ons, test pilot and shield study add-ons use RPM in this world where every page's capabilities must be whitelisted in a central place?

(In reply to Dave Townsend [:mossop] from comment #24) > If we're concerned about being able to see the capabilities of about: pages > in one central place then it may make sense to go one step further and just > list all the RPM urls in one place along with any boilerplate necessary to > get the main process side ready. This would mean we wouldn't need to load > AboutPrivateBrowsingHandler.jsm until it was actually needed for example. This sounds beneficial to me and I think we should aspire such a model. > How do system add-ons, test pilot and shield study add-ons use RPM in this > world where every page's capabilities must be whitelisted in a central place? That I am not entirely sure. I guess the problem is that the entire code is hosted outside mozilla-central, right? Do we need input from folks working on those projects to move forward? Two possible ways forward: * We land that patch basically as is (including RPMAccessManager) and follow up in a new bug addressing your concerns. * We remove RPMAccessManager from this patch, use this bug to solely have about:privatebrowsing rely on RPM and deal with all further security improvements over in Bug 1459909 and dependencies.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25) > (In reply to Dave Townsend [:mossop] from comment #24) > > If we're concerned about being able to see the capabilities of about: pages > > in one central place then it may make sense to go one step further and just > > list all the RPM urls in one place along with any boilerplate necessary to > > get the main process side ready. This would mean we wouldn't need to load > > AboutPrivateBrowsingHandler.jsm until it was actually needed for example. > > This sounds beneficial to me and I think we should aspire such a model. > > > How do system add-ons, test pilot and shield study add-ons use RPM in this > > world where every page's capabilities must be whitelisted in a central place? > > That I am not entirely sure. I guess the problem is that the entire code is > hosted outside mozilla-central, right? Do we need input from folks working > on those projects to move forward? I'll chat with some of them and see how we can do something useful for them too. > Two possible ways forward: > * We land that patch basically as is (including RPMAccessManager) and follow > up in a new bug addressing your concerns. Let's do this. Maybe RPMAccessManager changes significantly there but we might as well move forwards here in the meantime. I'll get onto the review today or tomorrow.

(In reply to Dave Townsend [:mossop] from comment #26) > > That I am not entirely sure. I guess the problem is that the entire code is > > hosted outside mozilla-central, right? Do we need input from folks working > > on those projects to move forward? > > I'll chat with some of them and see how we can do something useful for them > too. Thanks! > > Two possible ways forward: > > * We land that patch basically as is (including RPMAccessManager) and follow > > up in a new bug addressing your concerns. > > Let's do this. Maybe RPMAccessManager changes significantly there but we > might as well move forwards here in the meantime. I'll get onto the review > today or tomorrow. That sounds like a good plan to me too - thanks!

Comment on attachment 8979909 [details] [diff] [review] bug_1459204_about_privatebrowsing_rpm.patch Hey Baku, I would need review from a DOM Peer since I am removing AboutCapabilities.webidl as well as removing the entry point to it within Document.webidl.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb4f880a8aa4 Have about:privatebrowsing rely on RemotePageManager instead of AboutCapabilities. r=mossop,baku