1430751 - Apply unified behavior model for about: pages to about:privatebrowsing

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Boris, as discussed at the AllHands we are about to rewrite about:privatebrowsing to load with content privileges instead of system privileges. Overall that seems to work pretty well but I wanted to make sure you are onboard with the proposed changes and ultimately I have some implementation questions. As previously discussed, we would like to get away from custom events and instead use webidl bindings. In the end we would like to load all JS from external resources and also apply a CSP to about: pages.

I attached a WIP patch, and while there is still a lot of work to do, it already illustrates the proposed changes. I was wondering if you could take the time to take a look at my questions: 
(a) We extend Document.webidl by a partial interface which uses [Func="nsDocument::CallerIsAboutPrivateBrowsing"] where CallerIsAboutPrivateBrowsing acts as a gatekeeper to only expose attributes to about:privatebrowsing. I think it makes sense to use [Func=""] for that, or would you prefer to burry something within the webidl bindings similar to [ChromeOnly]?

(b) I think it makes sense to do localization within C++ by adding a new *.properties file and transfer information in form of a JSON string (which is realized through the dictionary in webidl) back to about:prviatebrowsing. We then use that JSON to dynamically fill containers with the localized data. I already discussed this change with other folks like e.g. Gijs, but also wanted to check if you are onboard with that change as well. Agreed?

(c) The page about:privatebrowsing allows to toggle tracking protection, which I realized through TogglePrefForTrackingProtectionForPB() which is also exposed via the document. Now, that runs in the content process and hence we have to do an IPC call to actually toggle the tracking protection pref. Does that also make sense to you?

(d) We use *.html files instead of *.xhtml. Gijs told me that in the past we had some problmes with localization that broke the stricter xhtml rules. Using *.html instead would provide a little more flexibility.

(e) I am not entirely sure about the difference between URI_CAN_LOAD_IN_CHILD and URI_MUST_LOAD_IN_CHILD and when to use which one. Now that we are lowering about:privatebrowsing to load with content privileges would it make more sense to use URI_CAN_LOAD_IN_CHILD like e.g. about:blocked, or would it make more sense to actually change pages like about:blocked to also use URI_MUST_LOAD_IN_CHILD?

(f) If not in private browsing mode, then about:privatebrowsing has a button to open a new window in private browsing mode, which I realized through openWinInPBMode(). Now it gets tricky. What is the best way to open a new window in private browsing mode in C++ land. I now how to do that in JS land, but not in C++ land. Can you provide some pointers?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

I just realized there is one major flaw in the new design. In the new design we set the 'Tracking Protection' toggle to on/off only when we open about:prviatebrowsing. The old design used an observer on the actual pref and dynamically toggles the switch to  on/off.

In more detail:
* Open about:privatebrowsing in one tab
* Open about:preferences in a second tab and switch tracking protection on/off
* Observe that the pref change does not get picked up by about:privatebrowsing tab

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I think it makes sense to use [Func=""] for that

I agree.  ChromeOnly was added because it was a common case (well, and it predates Func) and is a bit more efficient than Func.  But for this case, Func is fine.

That said, the implementation of this Func, as all Func annotations should be considered performance-sensitive, because it will run at setup on every pageload.  GetSpec on a URI is _expensive_ in some cases (e.g. large data: URIs).  We should probably at least check that the scheme is "about" before doing GetSpec.  I assume we will never have "about:privatebrowsing?stuff", so the EqualsLiteral is OK?

Also, GetSpec is fallible.  Should probably return false if it fails.

> (b) I think it makes sense to do localization within C++ by adding a new *.properties file

I have heard rumors that we may want to make localization async.  Might be worth returning a Promise with the data?

Why are we converting the data to JSON just to deserialize it on the other end?  Why not just return the dictionary we built up?  It'd require a method call in JS, not an attribute, but I think it would be more usable, and clearer about the work really being done...

> hence we have to do an IPC call to actually toggle the tracking protection pref.

Looks reasonable, I think, modulo comment 2.

> (d) We use *.html files instead of *.xhtml.

Seems fine, if we're not doing DTD-based localization.

> I am not entirely sure about the difference between URI_CAN_LOAD_IN_CHILD and URI_MUST_LOAD_IN_CHILD

The "can" case can be loaded in parent or child.  The "must" case must be loaded in the child.

The main question here is what should happen when the user types "about:privatebrowsing" in the URL bar.  If the tab is currently showing a parent-process thing, should it switch to content process or stay where it is?  

about:blocked shouldn't be URI_MUST_LOAD_IN_CHILD, because you can be trying to do an http load in the parent in an iframe and then have to deal with it being blocked, no?

In general, it makes sense to me to use URI_MUST_LOAD_IN_CHILD for about:privatebrowsing.

> What is the best way to open a new window in private browsing mode in C++ land.

I don't know, offhand.  How would you do it in JS?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Thanks Boris for your feedback. I incorporated your suggestions and mconley helped me with opening a new window in private browsing mode. I guess the next step is to clean up testcases. I preliminary did a first try run. It seems there are a few tests that need to fixed, but overall not too bad:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=421565e1c9e65f96d7da0f238e8506898347cf38

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Getting closer, a few more things that need to be fixed though:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a75d5d6ea0cf10f178abeef51d7f0f70f681a2b1

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Android is its own story, but other than that I think we are almost there:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3b5c4a223368dde27b19d3704faea0377f9249d

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Gijs, within this patch is a complete replica of about:privatebrowsing but instead of loading with system privileges, we load it with content privileges. We have discussed several things already, if something is unclear I am happy to hop on vidyo call. Please note that this is rather a first round of a review requests, the only reason I didn't set r? is because I am not sure if we need to replicate dontShowIntroPanelAgain(). If we do, then we need some workaround for that, because I think there is no easy way to actually call from C++ land into browser-trackingprotection.js.

Android will be handled separately, I am not sure if we should incorporate changes for that within this bug or should an Android developers handle that for us. I am adding a WIP patch to this bug anyway, but I think we should rather file a separate bug for android.

What do you think?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_update_about_privatebrowsing_tests.patch

@Gijs: I am updating the title 'Open a private window?' to use upper case, which also matches the test in the page itself then. Allows us to use only one localization sentence instead of two.

@whimboo: Can you check the puppeteer changes please!

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing_android.patch

A WIP patch what would need to be done in android, but I guess we should move that to a separate bug.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8949687 [details] [diff] [review]
bug_1430751_update_about_privatebrowsing_tests.patch

Review of attachment 8949687 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/firefox-ui/tests/functional/private_browsing/test_about_private_browsing.py
@@ +35,5 @@
>              self.marionette.navigate('about:privatebrowsing')
>  
>              status_node = self.marionette.find_element(By.CSS_SELECTOR, 'p.showNormal')
>              self.assertEqual(status_node.text,
> +                             "You are currently not in a private window.",

You have to retrieve the localized value here via `self.browser.localize_property()`. Otherwise this will break the test for other locales than en-US.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_update_about_privatebrowsing_tests.patch

Thanks whimboo for pointing that out - here we go.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8949686 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8949686 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, we should probably have a separate bug for android.

I think the general idea here is good, but looking at the implementation and the question about dontShowIntroPanelAgain makes me think we need a forward-compatible lightweight way of communicating from in-content about: pages into privileged JS in the parent, for very specific hole-punching (like "toggle the TP pref" or "don't show this intro panel again" or, in this case, "show a private window"). Right now you're writing C++ code for every case and that will be hard to scale, and hard to maintain for frontend people who don't generally have to mess with the IPC internals we have. See below for a concrete proposal (essentially, exposing a small subset of the frame message sending interfaces only to about: page DOMs). It might be worth getting feedback on that idea from someone else, like bholley or bz.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.html
@@ +1,1 @@
> +<html>

For the real patch, please include a doctype (<!DOCTYPE html>), license header (can be preprocessed) and make this a `hg cp` of the original (if not just modifying the original, which might be easier).

@@ +1,3 @@
> +<html>
> +  <head>
> +    <link id="favicon" rel="icon" type="image/png"/>

If not using xhtml, <link> and <meta> tag don't need to self-close in HTML.

@@ +34,5 @@
> +      </section>
> +
> +      <h2 id="tpSubHeader" class="about-subheader">
> +        <span class="tpTitle" id="tpTitle"></span>
> +        <input id="tpToggle" class="toggle toggle-input" type="checkbox"/>

Same for <input>

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +79,4 @@
>  }
> +
> +// TODOs:
> +// * don't show intro panel again (grep for dontShowIntroPanelAgain);

The tricky thing is that this needs messaging to the parent anyway. We can either do it in the JS code or in the C++ code but it'll need to happen because we need to write to prefs which we can't do in the content process. If we're trying to minimize the amount of listener JS goop we need, I guess we can put the messaging on the C++ side.

Then to talk to the JS, I guess the simplest thing is to either add a custom event listener to browser.xul and fire a chromeonly event from C++, or add a method to nsIXULBrowserWindow.idl.

Alternatively, we could move the logic entirely into C++, but I expect that will be frowned upon for maintenance reasons... OTOH, it's kind of weird to have some frontend code in the page, and then some C++ in the middle, and then some more frontend code in the parent...

This is going to be a recurring theme though (in fact, you've already solved this twice for the "open a new private browsing window" code as well as setting the main TP pref!), so perhaps we can come up with a more abstracted solution?

Thinking about it more, how about this: add a webidl method exposed on all about: pages that takes a DOMString message name, and a JSON dictionary of parameters, and then forward that through the C++ side of the webidl to the parent, using the message manager APIs? It would essentially expose a subset of the messageManager API to unprivileged about: pages. As long as we ensure that this subset is adequately namespaced away from the 'general' privileged message manager, that seems OK to me... So we could for instance always use the same message name ("AboutPageMessage"), and pass the principal of the page, and pass all the message name / data parameters we were given along with that in the usual 'data' part of the messageManager APIs.

In browser.js we could then have a single messageManager listener for that specific message, and have that just have a switch() block to deal with all the different messages. Then this would be in 1 place instead of spread into a gazillion different objects for each about page that all have to deal with their own communication.

Essentially, I think our original requirements were:
1) avoid having to have content-scripts-per-page that we load for each frame global (content.js / tab-content.js which we have now)
2) avoid requiring system principal for pages
3) ideally ensure pages can live in the sandbox (ie live remotely)

So based on that, having some subset of the message manager exposed through webidl only for about: pages seems ideal to me, as long as we're confident in our restrictions that avoid web content accessing these globals. It'd be interesting if we could ensure that all JS on the stack belonged to the about: page generally (so that even if you found a way to access the original page (bypassing wrappers), the semi-privileged API wouldn't be available), but I guess that is a secondary line of defence anyway.

::: browser/components/privatebrowsing/jar.mn
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  browser.jar:
> +    content/browser/aboutPrivateBrowsing.html             (content/aboutPrivateBrowsing.html)

Nitty nit: please keep this sorted.

::: dom/base/nsDocument.cpp
@@ +3217,5 @@
> +  if (formatter) {
> +    formatter->FormatURLPref(NS_LITERAL_STRING("app.support.baseURL"),
> +      aAboutPBReport.mAppSupportBaseURL);
> +    formatter->FormatURLPref(NS_LITERAL_STRING("privacy.trackingprotection.introURL"),
> +      aAboutPBReport.mPrivacyTrackingprotectionIntroURL); 

Nit: whitespace.

Someone other than me probably needs to review this...

::: dom/webidl/Document.webidl
@@ +480,5 @@
> +// Partial inferace for the various flavors of about: pages
> +partial interface Document {
> +  [Func="nsDocument::CallerIsAboutPrivateBrowsing"] AboutPrivateBrowsingReport getAboutPrivateBrowsingData();
> +  [Func="nsDocument::CallerIsAboutPrivateBrowsing"] void openWinInPBMode();
> +  [Func="nsDocument::CallerIsAboutPrivateBrowsing"] attribute boolean prefForTrackingProtectionInPB;

With the messaging thing we wouldn't need the openWinInPBMode() anymore, and the attribute would be readonly (because we can read the pref without talking to the parent).

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8950151 [details] [diff] [review]
bug_1430751_update_about_privatebrowsing_tests.patch

Review of attachment 8950151 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the puppeteer changes.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Summarizing my discussion with mrbkap on IRC:

* The document can and should only be the entry point to the about page API, something like
  [Func="nsDocument::CallerIsAboutPage"] readonly attribute AboutCapabilities aboutCapabilities();
* That AboutCapabilities is a new webidl that exposes functionality like
  <> UpdatePref()
  <> openWindowInPrivateBrowsingMode()
  <> ...
* AboutCapabilites is then implement in JS but we would still be able to use the description/vetting/etc from webidl.
  In addition, implementing AboutCapabilities has the benefit that
  <> JS has helpers around a lot of needed functionality, and
  <> most of the about: page maintainers are frontend engineers which probably prefer to write JS rather than C++.
* Addtionally AboutCapabilities would have something like:
  <> void sendAsyncMessage(messageName, data)
  to send some message manager message to browser.js (basically what Gijs already suggested in comment 12).
  Ultimately, we could use some specific prefix to make sure it can not accidentaly communicate outside of the about: world.

I am willing to flip all we got so far around because that sounds like the better plan in the end.

Gijs, Boris: What do you think?

The remaining question is, do we still support webidl to be implemented in JS? And if so, can someone provide a link to such code?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I haven't really been following this bug in detail.  The plan proposed in comment 14 is certainly feasible; I can't comment on whether it's the best plan or addresses whatever problems we're trying to address without doing more reading to get context.

> do we still support webidl to be implemented in JS?

Yes, though not everyone is happy about that.  That said, we have ~20 JS-implemented things for the moment, some of them fairly complex.  So I don't think it's going anywhere in the next few months.

> can someone provide a link to such code?

The best thing to crib for this purpose is probably External.webidl and nsGlobalWindowInner::GetExternal.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Thanks for all your input. One thing that occured to me last night is, that we also want to update e.g. about:blocked which currently passes URL queries which are then parsed within the actual about:blocked page. We want to get away from that as well. All that query information is available on the docshell (which is query-able from nsDocument.cpp). I am not sure how we can access that if we implement the webidl in JS and if that might be another obstacle in our architecture. Just one thing to keep in mind.

Comment 17

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> I am willing to flip all we got so far around because that sounds like the
> better plan in the end.
> 
> Gijs, Boris: What do you think?

I think this sounds OK.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> Thanks for all your input. One thing that occured to me last night is, that
> we also want to update e.g. about:blocked which currently passes URL queries
> which are then parsed within the actual about:blocked page. We want to get
> away from that as well. All that query information is available on the
> docshell (which is query-able from nsDocument.cpp). I am not sure how we can
> access that if we implement the webidl in JS and if that might be another
> obstacle in our architecture. Just one thing to keep in mind.

The JS webidl implementation would be privileged, and would be able to talk to the docshell for the window it was created for. If this docshell data isn't currently exposed, we might need to make it be so via webidl and/or xpcom.

Alternatively, you could create another separate compiled-code-implemented webidl thing for this kind of purpose, that isn't docshell, but ultimately that's just moving the same-ish implementation out of docshell (which would still potentially be good because docshell is huge and could do with not having more only-semi-related-stuff added into it).

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Blake and/or Boris, the attached patch is still a little messy which I will clean up soon. Anyway, the patch compiles but for some reason the JS implementation for AboutCapabilities can not be quried. I have no idea what's wrong and don't even know where to debug. In particular, within the about: page I do:
> document.aboutCapabilities
which then correctly dispatches to nsIDocument::GetAboutCapabilities, but then when calling
> ConstructJSImplementation("@mozilla.org/aboutcapabilities;1", sgo, &jsImplObj, aRv);
I get the following error:

[Child 31729, Main Thread] WARNING: Failed to get JS implementation for contract "@mozilla.org/aboutcapabilities;1": file /home/ckerschb/source/mc/dom/bindings/BindingUtils.cpp, line 2622


Is there anything obvious wrong within this patch? Any comments and suggestions welcome - thanks!

Comment 19

[Boris Zbarsky [:bzbarsky]]

You're trying to get the thing with contract "@mozilla.org/aboutcapabilities;1", but you never registered that contract.  So it doesn't work.

Again, see how window.external works.  The C++ implementation gets the JS impl by contract.  toolkitsearch.manifest maps the contract to the "22117140-9c6e-11d3-aaf1-00805f8a4905" CID and maps that CID to the right implementation .js file.  The JS impl claims to implement that CID.

Comment 20

[Boris Zbarsky [:bzbarsky]]

So in particular, you would need to add a manifest file that registers your contract and add that manifest file to browser/installer/package-manifest.in and mobile/android/installer/package-manifest.in (and file a comm-cental bug to add it to their installer manifests).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Oh, and you will need to add your component .js file to the installer manifests too.

Comment 22

[Boris Zbarsky [:bzbarsky]]

And I dunno whether you want to add your stuff to EXTRA_PP_COMPONENTS or EXTRA_COMPONENTS in moz.build; I'm not sure what the difference is....  PeerConnection.js just uses EXTRA_COMPONENTS for itself and PeerConnection.manifest, for example.

Comment 23

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #22)
> And I dunno whether you want to add your stuff to EXTRA_PP_COMPONENTS or
> EXTRA_COMPONENTS in moz.build; I'm not sure what the difference is.... 
> PeerConnection.js just uses EXTRA_COMPONENTS for itself and
> PeerConnection.manifest, for example.

I think the "PP" is for "preprocessed" - ie whether the preprocessor for #ifdef etc. runs as part of the build. You shouldn't need this. If you have OS-specific or other per-#define-like things for JS components we now prefer using AppConstants.jsm, rather than the preprocessor. This has build speed benefits (build system can just symlink rather than reading + processing files) and thus development benefits, as well as making the file more easily lintable by standard tools etc. etc.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Sorry, if I don't understand, but everything you mentioned in comment 19 to 21, I did as far as I can tell. In particular:

* I created aboutcapabilities.manifest
* which I added to browser/installer/package-manifest.in
* the manifest registers the component with {4c2b1f46-e637-4a91-8108-8a9fb7aab91d}
* and then nsAboutCapabilities.js implements {4c2b1f46-e637-4a91-8108-8a9fb7aab91d}

I precisely looked how toolkitsearch.manifest and nsSideBar.js as far as I can tell. Maybe I am misunderstanding what you mentioned in those comments.

FWIW, the EXTRA_PP_COMPONENTS just is used for preprocessing, which is not needed in our case.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8949686 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

marking this one as obsolete to avoid any kind of misommunication.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Hmm.  I see it now, but find-in-page was not finding the "capabilities;" in the manifest when I looked at the patch...

Reading through the patch more carefully, the one obvious remaining issue I see is that nsAboutCapabilities.js is not listed in EXTRA_COMPONENTS.  It should be.  And you probably need to add things to the other installer manifests, but that's not an issue for local testing, of course.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Yes, facepalm, that was exactly the problem.

+++ b/browser/components/about/moz.build
+EXTRA_COMPONENTS += [
+    'aboutcapabilities.manifest',
+    'nsAboutCapabilities.js',
+]

Sorry for all the confusion, I can take it from here.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Gijs, Blake, thanks for the discussions and your insightful comments. This new version is not only a complete replica of about:privatebrowsing running with content privileges (instead of system privileges), it also uses the newly created plumbing. In particular:
* document.webidl does not get polluted much because it only exposes aboutcapabilities
* aboutcapabilities then exposes all the required functionality for the various flavours of about: pages and can easily be extended without having to write C++ code.

Additional benefit: GetAboutPrivateBrowsingData() returns AboutPrivateBrowsingReport which is a dictionary (other about pages can add similar dictionaries). If GetAboutPrivateBrowsingData() would simply return an 'object' one would get the following error:

 WARNING: Silently denied access to property "inPrivateBrowsingMode": Access to privileged JS object not permitted (@chrome://browser/content/aboutPrivateBrowsing.js:81:0): file /home/ckerschb/source/mc/js/xpconnect/wrappers/XrayWrapper.cpp, line 263

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8949688 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing_android.patch

I created a new bug for converting the android version -> See Bug 1438624.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8951374 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8951374 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good overall. I have a few comments that need addressing and I'm going to lean on Gijs for the .html/.js portions of the patch, but I much prefer this to the previous patch.

::: browser/base/content/browser.js
@@ +8929,3 @@
>    init() {
>      window.messageManager.addMessageListener(
> +      "AboutCapabilities:SetBoolPref",

Gijs would know better than me, but I think it's nicer to have some sort of array of message names so the API (such as it exists) is easier to pick out of the code.

@@ +8929,5 @@
>    init() {
>      window.messageManager.addMessageListener(
> +      "AboutCapabilities:SetBoolPref",
> +      msg => {
> +        Services.prefs.setBoolPref(msg.data.pref, msg.data.value);

We've avoided adding message manager APIs like this in the past because of the sensitive nature of prefs. While we still don't make many guarantees about a compromised content process, having APIs to set arbitrary prefs will only make that eventual effort harder. This should probably piggy-back on the AsyncPrefs.jsm API, which was added as an easier way to set prefs from the content process from JS (and only allows setting a whitelisted set of prefs).

::: browser/components/about/nsAboutCapabilities.js
@@ +57,5 @@
> +  GetAboutPrivateBrowsingData() {
> +  	let bundle = Services.strings.createBundle("chrome://global/locale/about/privatebrowsing.properties");
> +
> +  	let report = {
> +      openPrivateWindowButtonLabel : getStrFromBundle(bundle, "AboutPrivateBrowsingOpenPrivateWindowLabel"),

Nit: indentation.

::: dom/base/nsDocument.cpp
@@ +3377,5 @@
>  
> +already_AddRefed<mozilla::dom::AboutCapabilities>
> +nsIDocument::GetAboutCapabilities(ErrorResult& aRv) const
> +{
> +  static nsCOMPtr<nsISupports> sAboutCapabilities;

I think static nsCOMPtrs aren't allowed because they incur a startup cost (will eventually show up as shutdown leaks on treeherder). This should probably be |static StaticRefPtr<AboutCapabilities>| and on initialization this should use a ClearOnShutdown in order to not leak it on shutdown.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Thanks Blake for your feedback!

(In reply to Blake Kaplan (:mrbkap) from comment #30)
> ::: browser/base/content/browser.js
> > +      "AboutCapabilities:SetBoolPref",
> Gijs would know better than me, but I think it's nicer to have some sort of
> array of message names so the API (such as it exists) is easier to pick out
> of the code.

I followed what browser.js is doing, but I am happy to incorporate whatever suggestions you you and Gijs have here. Please guide me to such an array and I am happy to incorporate that change if really needed.

> > +      msg => {
> > +        Services.prefs.setBoolPref(msg.data.pref, msg.data.value);
> 
> We've avoided adding message manager APIs like this in the past because of
> the sensitive nature of prefs. While we still don't make many guarantees
> about a compromised content process, having APIs to set arbitrary prefs will
> only make that eventual effort harder. This should probably piggy-back on
> the AsyncPrefs.jsm API, which was added as an easier way to set prefs from
> the content process from JS (and only allows setting a whitelisted set of
> prefs).

I tried that, but using AsyncPrefs.jsm doesn't work for our use case. I am getting:
> Setting pref privacy.trackingprotection.pbmode.enabled from content is not allowed

Within browser.js I saw a bunch of Services.prefs.SetBoolPref(). Not saying it's good, just saying AsyncPrefs does not work for our use case. Again, happy to incorporate such requested changes if needed.

Comment 32

[:Gijs (he/him)]

Comment on attachment 8951545 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8951545 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +8929,2 @@
>    init() {
>      window.messageManager.addMessageListener(

The way this is generally implemented is:


var AboutCapabilitiesListener = {
  _topics: [...],
  init() {
    for (let topic of kTopics) {
      window.messageManager.addMessageListener(topic, this);
    }
  },
  receiveMessage({name, data, target}) {
    switch (name) {
      case "msg1":
        ...;
        break;
      case ...
    }
  }
};

etc.

You could potentially reuse the list of topics in uninit(), which some of the other code in browser.js seems to do. I'm not sure if that's necessary if you're registering to the message manager in the window, I'd kind of expect cycle collection to take care of it, but if not then this setup makes that easier too because you can reuse the _topics property to unregister all the listeners.

@@ +8929,5 @@
>    init() {
>      window.messageManager.addMessageListener(
> +      "AboutCapabilities:SetBoolPref",
> +      msg => {
> +        Services.prefs.setBoolPref(msg.data.pref, msg.data.value);

Yeah, so, as :mrbkap said, this is basically sand-box-escaping because you can turn off the sandbox completely like this once you compromise the sandboxed profile (by setting the sandboxing pref). So we can't do this, unfortunately.

He's also right that AsyncPrefs is the right thing to use here.

> > Setting pref privacy.trackingprotection.pbmode.enabled from content is not allowed

The fix here is to modify AsyncPrefs.jsm and add this pref to the list of allowed prefs. Ditto for other prefs you need to modify.

::: browser/components/about/nsAboutCapabilities.js
@@ +47,5 @@
> +    this.mm.sendAsyncMessage("AboutCapabilities:SetCharPref", {
> +      pref: aPref,
> +      value: aValue
> +    });
> +  },

So I'd personally go for exposing specifics here (so having a method called setTrackingProtectionPB(bool) or whatever). If you expose this generically and use asyncprefs (or this manual forwarding) then the content-privileged about page can set any asyncprefs-listed pref they like, which we probably don't want to allow.

@@ +54,5 @@
> +    this.mm.sendAsyncMessage("AboutCapabilities:" + aMessage, aParams);
> +  },
> +
> +  GetAboutPrivateBrowsingData() {
> +    let bundle = Services.strings.createBundle("chrome://global/locale/about/privatebrowsing.properties");

You can use XPCOMUtils to easily define this as a lazy property so we don't need to fetch the strings every time.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +67,5 @@
> +  tpSubHeader.classList.toggle("tp-off", !tpEnabled);
> +
> +  tpButton.addEventListener("click", () => {
> +    let newValue = !tpButton.value;
> +    aboutCapabilities.SetBoolPref("privacy.trackingprotection.pbmode.enabled", newValue);

JS methods should use camelCase, so no leading capitalized character. webidl etc. should take care of exposing UpperCase things in C++.

::: dom/base/nsDocument.cpp
@@ +3411,5 @@
> +  // getSpec is an expensive operation, hence we first check to
> +  // see if the caller is an about: page.
> +  bool isAboutScheme = false;
> +  uri->SchemeIs("about", &isAboutScheme);
> +  return isAboutScheme;

This doesn't check anything else, and looks like it would also pass on about:blank. We definitely don't want this on about:blank (or about:srcdoc, or any other content-linkable about: pages, tbh).

::: dom/webidl/AboutPagesReports.webidl
@@ +6,5 @@
> + * Dictionary to allow about:privatebrowsing query data
> + */
> +
> +dictionary AboutPrivateBrowsingReport {
> +  boolean inPrivateBrowsingMode = false;

Having to have IDL properties for all of these seems like a nuisance for JS/frontend work. Could we just expose a method that allows fetching a named localized string?

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Gijs, Blake, I incorporated all of your suggestions to lock down exposed functionality for about: pages even more. Initially I thought more about usability, but in the end I think it makes sense to really only expose specific functionality. While this non generic solution involves a little more work for each about: page to be converted, I agree it's the way to go.

Anyway, I further added AboutUtils.cpp which maintains a whitelist of about: pages that have access to aboutCapabilities. I added this because I don't think we should pollute nsDocument.cpp with that information. Now nsAboutCapabilities.js as well as AboutUtils.cpp are maintained within the same place within the codebase.


> > +dictionary AboutPrivateBrowsingReport {
> > +  boolean inPrivateBrowsingMode = false;
> 
> Having to have IDL properties for all of these seems like a nuisance for
> JS/frontend work. Could we just expose a method that allows fetching a named
> localized string?

I slightly disagree. Please note that AboutPrivateBrowsingReport not only holds localized strings but also some URLS, booleans, etc. Basically all the relevant information to display about:privatebrowsing. Keeping all that information within one dictionary allows to only expose that exact dictionary without having something generic that might be abused. I don't think it creates that much more work for frontend engineers, given the benefit of not exposing something generic.

Comment 34

[:Gijs (he/him)]

Comment on attachment 8951966 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8951966 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +8933,5 @@
>          TrackingProtection.dontShowIntroPanelAgain();
> +        break;
> +
> +      case "AboutCapabilities:SetPrefPrivacyTrackingProtectionPbmodeEnabled":
> +        AsyncPrefs.set("privacy.trackingprotection.pbmode.enabled", aMsg.data.value);

OK, so, confusingly, the parent process doesn't need async prefs. They only exist for the content process. browser.js runs in the parent. It can just use `Services.prefs.setBoolPref(...);` directly.

::: browser/components/about/AboutUtils.cpp
@@ +10,5 @@
> +
> +namespace mozilla {
> +namespace browser {
> +
> +/* 

Nit: trailing whitespace.

::: browser/components/about/nsAboutCapabilities.js
@@ +25,5 @@
> +    try {
> +      this.mm = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDocShell)
> +                      .QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIContentFrameMessageManager);

Out of curiosity, can someone enlighten me on how this stuff gets destroyed? That is, this component takes a strong ref to the window and the message manager. I believe that the window has a ref to this component (via its document) because otherwise the instance wouldn't even exist. Is this just the magic of cycle collection?

@@ +36,5 @@
> +    return Services.prefs.getBoolPref(aPref);
> +  },
> +
> +  getCharPref(aPref) {
> +    return Services.prefs.getCharPref(aPref);

So personally I would argue that strings are much less sensitive than "retrieve all the prefs". I guess this is more reusable... but it's an interesting decision in terms of which things we expose generically and which we expose specifically.

@@ +45,5 @@
> +  },
> +
> +  getAboutPrivateBrowsingData() {
> +    let report = {
> +      openPrivateWindowButtonLabel : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingOpenPrivateWindowLabel"),

All of these strings are namespaced with "AboutPrivateBrowsing", which seems superfluous because they're in "aboutPrivateBrowsing.properties". Unlike DTDs (where most of these came from) there's no namespace pollution between different files for .properties things, so I think we should just remove the prefix to make all of this easier to read.

@@ +46,5 @@
> +
> +  getAboutPrivateBrowsingData() {
> +    let report = {
> +      openPrivateWindowButtonLabel : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingOpenPrivateWindowLabel"),
> +      notPrivateText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingNotPrivate"),

I love abstractions, but `getStrFromBundle(gPBundle, ` saves -1 characters over just `gPBundle.GetStringFromName(` -- `getStrFromBundle` is 1 character shorter than `GetStringFromName`, but the comma and space then promptly take 2 chars back.

So let's just use `gPBBundle.GetStringFromName()` directly. If you want to remove the repetition, perhaps create an object:

```js
let reportStrings = {
  titleText: "Title",
  ...
};
```

and process:

```js
for (let [prop, val] of Object.entries(reportStrings)) {
  report[prop] = gPBundle.GetStringFromName(val);
}
```

@@ +52,5 @@
> +      titleTrackingText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingTitleTracking"),
> +      privateBrowsingInfoNotSavedText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingInfoNotSaved"),
> +      visitedText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingInfoVisted"),
> +      cookiesText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingInfoSearches"),
> +      searchesText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingInfoCookies"),

cookies: searches
searches: cookies.

Doesn't look right to me...

@@ +64,5 @@
> +      privateBrowsingTPDescriptionText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingTPDescription"),
> +      privateBrowsingLearnMoreText : getStrFromBundle(gPBBundle, "AboutPrivateBrowsingLearnMore"),
> +      inPrivateBrowsingMode : PrivateBrowsingUtils.isContentWindowPrivate(this.window),
> +      appSupportBaseURL : Services.urlFormatter.formatURLPref("app.support.baseURL"),
> +      privacyTrackingprotectionIntroURL : Services.urlFormatter.formatURLPref("privacy.trackingprotection.introURL"),

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #33)
> > > +dictionary AboutPrivateBrowsingReport {
> > > +  boolean inPrivateBrowsingMode = false;
> > 
> > Having to have IDL properties for all of these seems like a nuisance for
> > JS/frontend work. Could we just expose a method that allows fetching a named
> > localized string?
> 
> I slightly disagree. Please note that AboutPrivateBrowsingReport not only
> holds localized strings but also some URLS, booleans, etc. Basically all the
> relevant information to display about:privatebrowsing. Keeping all that
> information within one dictionary allows to only expose that exact
> dictionary without having something generic that might be abused. I don't
> think it creates that much more work for frontend engineers, given the
> benefit of not exposing something generic.

Well, it requires changing (and thus rebuilding) WebIDL files to add/change strings. Which I believe isn't possible on artifact builds. Which means frontend folks who modify about:privatebrowsing can't use artifact builds for their work, which is a pretty significant barrier.

It also creates a bunch of tight coupling and duplication across strings -> webidl -> js component -> js frontend bits. Which then leads to mistakes (see above).

Generally I wouldn't expect there to be a problem with exposing these strings to these specific about: pages. We can limit the bundle from which we use strings if that makes things better...

The formatted URLs and private browsing mode state would indeed still need exposing separately.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +35,5 @@
> +  document.getElementById("tempFiles").textContent = pbDataObj.tempFilesText;
> +
> +  document.getElementById("privateBrowsingSaved").textContent = pbDataObj.privateBrowsingInfoSavedText;
> +  document.getElementById("bookmarks").textContent = pbDataObj.bookmarksText;
> +  document.getElementById("downloads").textContent = pbDataObj.downloadsText;

Where the string and/or properties names match, I'd be inclined to try to reduce some of the repetition here. E.g.:

let textElementsToLocalize = [
  "title", "titleTracking", "privateBrowsingNotSaved", "visited", "cookies",
  ...
];
for (let elementID of textElementsToLocalize) {
  document.getElementById(elementID).textContent = pbDataObj[elementID + "Text"];
}

@@ +65,5 @@
> +  title.classList.toggle("hide", tpEnabled);
> +  titleTracking.classList.toggle("hide", !tpEnabled);
> +  tpSubHeader.classList.toggle("tp-off", !tpEnabled);
> +
> +  tpButton.addEventListener("click", () => {

The way this was previously set up was through having a change listener on the actual checkbox (which is visually styled into oblivion), and then having clicking the button trigger a click on the actual checkbox. In theory this helps with ensuring the page still works correctly when e.g. viewed without stylesheets or with aggressive alternative styling (like high contrast mode which may remove some of the imagery we use here). Can we keep doing this, or was there a particular reason you removed this?

::: dom/base/nsDocument.cpp
@@ +3410,5 @@
> +
> +bool
> +nsDocument::CallerIsTrustedAboutPage(JSContext* aCx, JSObject* aObject)
> +{
> +  return mozilla::browser::AboutUtils::CallerIsTrustedAboutPage(aCx, aObject);

I don't really understand how this works on mobile. I'd assume this wouldn't compile? It'd be kind of odd if mobile/ depended on stuff in browser/ ...

This is a problem we will need to come up with a solution for. Traditionally, the solution was IDL and having a different implementation of the service/interface provided by different client applications that ran on toolkit. I don't know what the best solution is now. :mrbkap is probably the better-qualified person to suggest something.

::: toolkit/modules/AsyncPrefs.jsm
@@ +19,5 @@
>  
>    "narrate.rate",
>    "narrate.voice",
>  
> +  "privacy.trackingprotection.pbmode.enabled",

So this you also don't need if we're using browser.js to set the pref.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8951966 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8951966 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/about/AboutUtils.h
@@ +17,5 @@
> +  static bool CallerIsTrustedAboutPage(JSContext* aCx, JSObject* aObject);
> +
> +private:
> +  AboutUtils() {}
> +  virtual ~AboutUtils() {}

Why is this virtual? Why bother declaring these at all?

::: dom/base/nsDocument.cpp
@@ +3410,5 @@
> +
> +bool
> +nsDocument::CallerIsTrustedAboutPage(JSContext* aCx, JSObject* aObject)
> +{
> +  return mozilla::browser::AboutUtils::CallerIsTrustedAboutPage(aCx, aObject);

It seems like for the time being we can kind of ignore this problem and simply return "false" on mobile (to be honest, I've forgotten why the implementation of CallerIsTrustedAboutPage is in its own class in browser/ instead of just implemented here). The Fennec code can have its own implementation of @mozilla.org/aboutcapabilities;1 too.

Am I missing anything about this?

::: dom/base/nsIDocument.h
@@ +2875,5 @@
>    void GetReferrer(nsAString& aReferrer) const;
>    void GetLastModified(nsAString& aLastModified) const;
>    void GetReadyState(nsAString& aReadyState) const;
> +
> +  already_AddRefed<mozilla::dom::AboutCapabilities>GetAboutCapabilities(

Nit: missing space after '>'.

Comment 36

[Johann Hofmann [:johannh]]

Hi Christoph,

thank you for working on this! These pages have so many problems and while exposing WebIDL interfaces for privileged tasks sounds a little painful to me, I grant it that it certainly solves a lot of them.

I'd like to echo Gijs' concern about string handling in this bug, in particular I do not understand the need to move away from DTD based string injection. What threat are you trying to protect against? Localizers can still inject malicious content into other (chrome privileged) documents, there's no point doing it in about: pages. The webidl dictionary and required boilerplate code is already quite large for the rather small about:privatebrowsing page. It seems like a huge burden for the people who maintain these pages (like me) to manage this.

Also, did anyone check this patch with the folks working on the Fluent transition? They might be able to provide input, alternatives and gotchas. AFAIU Fluent works indeed asynchronously by looking at data attributes on elements. It automatically sanitizes its input and it should be sufficient to link the .ftl file in the document, without additional privileges.

It sounds like forcing ourselves to deliver strings through webidl interfaces might be slightly incompatible with Fluent and not necessary given the upcoming improvements it brings.

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

From the transition to Fluent perspective, this patch makes things slightly harder (but not by much), because it's possible to semi-automate DTD->Fluent, but not StringBundle->Fluent.

I also don't fully understand what would be the increased security of using .properties over DTD here, but I do see the value of transitioning it to Fluent.

If those pages are going to be unprivileged, we'd need to wait for bug 1407418 (hopefully this quarter) to get Fluent to support it. If it's privileged, you may be able to use it already (with my teams oversight since it's still quite early for Fluent in Firefox :)).

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Regarding comment 36 and 37. My plan was to clean up all of our about: pages not only in terms of adding a new security layer, but also unifying them in terms of how they are build. To that end I wanted to have either *.dtd or *.properties, where I didn't have a strong opinion for one or the other. Johann and I just had a chat and he filled me in that *.dtd is for static content vs *.properties is for dynamic content. To be honest I was not aware of the precise differentiation and it to me it makes sense, at least for about:privatebrowsing to keep that separation and keep the *.dtd and the *.properties file. Ultimately I hope that the efforts for fluent will clean that up.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Hey Gijs, as discussed over IRC we should use AsyncPrefs.jsm within nsAboutCapabilities.js. I incoroporated that change, but it seems it's not working because of:
> JavaScript error: chrome://browser/content/aboutPrivateBrowsing.js, line 47: Error: Permission denied to access property "then"

I am not sure how we can resolve this or if we should go back to implementing the pref switching within browser.js. Any suggestions?

Comment 40

[:Gijs (he/him)]

Comment on attachment 8953965 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8953965 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/about/nsAboutCapabilities.js
@@ +37,5 @@
> +      AsyncPrefs.set(aPref, aValue).then(function() {
> +        dump("\n\n setBoolPref resolved\n\n");
> +        resolve();
> +      });
> +    });

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39)
> Created attachment 8953965 [details] [diff] [review]
> bug_1430751_convert_about_privatebrowsing.patch
> 
> Hey Gijs, as discussed over IRC we should use AsyncPrefs.jsm within
> nsAboutCapabilities.js. I incoroporated that change, but it seems it's not
> working because of:
> > JavaScript error: chrome://browser/content/aboutPrivateBrowsing.js, line 47: Error: Permission denied to access property "then"
> 
> I am not sure how we can resolve this or if we should go back to
> implementing the pref switching within browser.js. Any suggestions?

The reason you're seeing issues here is that you're creating a promise in the privileged context of nsAboutCapabilities and passing it to the unprivileged context of about:privatebrowsing, which is then not allowed to access it . I believe you could create a content-privileged promise by using `new this.window.Promise`, which about:privatebrowsing *would* then be able to access.

I'm not entirely sure why webidl and our wrappers code doesn't clone the content here such that the content gets a cloned object. Perhaps it only does that when going from less- to more-privileged compartments or something. If that is true, it's also possible you could explicitly do this by using `Cu.cloneInto` to clone the promise into the window context (assuming that Promise is compatible with Cu.cloneInto(), which I don't know for sure off-hand). Try the this.window route first. :-)

Comment 41

[:Gijs (he/him)]

(Though, more abstractly, does the content really need to know when the pref has been set? I think we can just optimistically update the checkbox visually and not care about when we hear from the parent that the change has taken effect. Any navigations / requests will have to go through the parent anyway so it's not possible for things to get out of sync given the process messaging is fifo.)

Comment 42

[Boris Zbarsky [:bzbarsky]]

Wrappers code never clones anything automatically.

cloneInto is not going to work for Promise.  It uses structured cloning, and Promise is not structured-clonable last I checked.

Creating the Promise in the right global to start with is the right answer.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

Gijs, two things worth noting abut this patch:
a) We should wait till the Promise from setBoolPref resolves, otherwise we run into a race condition where getBoolPref resolves faster than the updating.
b) The original version of about:privatebrowsing relied on an observer pattern. Lowering the privileges of about:privatebrowsing does not allow to register such an observer. In order no to get out of sync with the toggle I think it makes sense to call setBoolPref with the current Value of the toggle. Please note that about:preferences e.g. openend in a another tab could potentially overwrite the tracking protection flag which does not get picked up by the about:prviatebrowsing tab.

Comment 44

[:Gijs (he/him)]

Comment on attachment 8954006 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8954006 [details] [diff] [review]:
-----------------------------------------------------------------

Almost done, just a bunch of nits and a few questions here and there. Thanks!

::: browser/components/about/nsAboutCapabilities.js
@@ +14,5 @@
> +function nsAboutCapabilities() {
> +}
> +
> +nsAboutCapabilities.prototype = {
> +

Nit: no blank line between the opening of the proto object and the first method/property definition please.

@@ +17,5 @@
> +nsAboutCapabilities.prototype = {
> +
> +  init(window) {
> +    this.window = window;
> +    this.isPrivateWindow = PrivateBrowsingUtils.isContentWindowPrivate(window);

Can you use XPCOMUtils.defineLazyGetter() for this at the bottom of the file? While about:privatebrowsing needs this, the other modules won't, so we probably don't need to do this on init but only when someone asks.

@@ +22,5 @@
> +    try {
> +      this.mm = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDocShell)
> +                      .QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIContentFrameMessageManager);

Nit: you can get the docshell like so:

window.document.docShell

which somewhat reduces the XPCOM goopiness here. I can't see a way of getting the message manager right now, though I expect bug 888600 will fix that. So you'd still need the second QI + gI calls.

@@ +29,5 @@
> +    }
> +  },
> +
> +  getBoolPref(aPref) {
> +    return Services.prefs.getBoolPref(aPref);

Nit: please define the optional second argument here (and in the WebIDL) and forward it. This allows specifying a default value.

@@ +57,5 @@
> +    let bundle = Services.strings.createBundle(aStrBundle);
> +    return bundle.GetStringFromName(aStr);
> +  },
> +
> +  getFormatURL(aFormatURL) {

Can we name this the same as the method to which it forwards (`formatURLPref`) ?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +4,5 @@
>  
> +const FAVICON_QUESTION = "chrome://global/skin/icons/question-32.png";
> +const STRING_BUNDLE = "chrome://browser/locale/aboutPrivateBrowsing.properties";
> +const TP_ENALBED_PREF = "privacy.trackingprotection.enabled";
> +const TP_PB_ENALBED_PREF = "privacy.trackingprotection.pbmode.enabled";

Nit: please fix spelling on these consts, 'ENABLED' instead of 'ENALBED'.

@@ +22,5 @@
> +  // if tracking protection is enabled globally we don't even give the user
> +  // a choice here by hiding the toggle completely.
> +  tpButton.classList.toggle("hide", globalTrackingEnabled);
> +  tpToggle.checked = trackingEnabled;
> +  tpButton.value = trackingEnabled;

We didn't used to assign to `tpButton.value` - is this an oversight or is it fixing something?

@@ +53,4 @@
>  
> +  let tpButton = document.getElementById("tpButton");
> +  tpButton.addEventListener("click", () => {
> +    let newValue = !tpButton.value;

This is still doing something subtly different from before, as I noted previously:

> The way this was previously set up was through having a change listener on
> the actual checkbox (which is visually styled into oblivion), and then
> having clicking the button trigger a click on the actual checkbox. In theory
> this helps with ensuring the page still works correctly when e.g. viewed
> without stylesheets or with aggressive alternative styling (like high
> contrast mode which may remove some of the imagery we use here). Can we keep
> doing this, or was there a particular reason you removed this?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
@@ +26,5 @@
>    </head>
>  
>    <body dir="&locale.dir;">
>      <p class="showNormal">&aboutPrivateBrowsing.notPrivate;</p>
> +    <button id="startPrivateBrowsing" class="showNormal">&privatebrowsingpage.openPrivateWindow.label;</button>

This still wants an accesskey attribute.

@@ +70,5 @@
>          <p class="about-info">&aboutPrivateBrowsing.learnMore3.before;<a id="learnMore" target="_blank">&aboutPrivateBrowsing.learnMore3.title;</a>&aboutPrivateBrowsing.learnMore3.after;</p>
>        </section>
>  
>      </div>
> +    <script type="application/javascript" src="chrome://browser/content/aboutPrivateBrowsing.js"></script>

Could probably leave this where it was? Or is there a significant reason not to do this?

::: dom/base/nsDocument.cpp
@@ +3389,5 @@
>  
>  bool
> +nsDocument::CallerIsTrustedAboutPage(JSContext* aCx, JSObject* aObject)
> +{
> +  /* 

Nit: whitespace

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

(In reply to :Gijs from comment #44)
> @@ +22,5 @@
> > +  // if tracking protection is enabled globally we don't even give the user
> > +  // a choice here by hiding the toggle completely.
> > +  tpButton.classList.toggle("hide", globalTrackingEnabled);
> > +  tpToggle.checked = trackingEnabled;
> > +  tpButton.value = trackingEnabled;
> 
> We didn't used to assign to `tpButton.value` - is this an oversight or is it
> fixing something?

I think there was no need because the old implementation was based on the observer pattern of the actual pref, so the toggle was never out of sync.
 
> @@ +53,4 @@
> >  
> > +  let tpButton = document.getElementById("tpButton");
> > +  tpButton.addEventListener("click", () => {
> > +    let newValue = !tpButton.value;
> 
> This is still doing something subtly different from before, as I noted
> previously:

I thought I answered that within comment 43, but maybe it was not obvious. Anyway, I am worried that the toggle gets out of sync, e.g. if you open a about:preferences in a second tab. If you update the tracking protections pref within that second tab, then about:privatebrowsing would not pick up that change. To that end I think it makes sense to actually pull the current value out of the toggle. This way, at least the value is set as expected when modyfing it within about privatebrowsing. I hope you agree.


@mrbkap: I had to move away from using a static variable within nsDocument as in my previous patch since we were leaking memory. I updated that to use mAboutCapabilities within nsIDocument.h. Similar to the other code we already have for similar scenarios.

Comment 46

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #45)
> > @@ +53,4 @@
> > >  
> > > +  let tpButton = document.getElementById("tpButton");
> > > +  tpButton.addEventListener("click", () => {
> > > +    let newValue = !tpButton.value;
> > 
> > This is still doing something subtly different from before, as I noted
> > previously:
> 
> I thought I answered that within comment 43, but maybe it was not obvious.
> Anyway, I am worried that the toggle gets out of sync, e.g. if you open a
> about:preferences in a second tab. If you update the tracking protections
> pref within that second tab, then about:privatebrowsing would not pick up
> that change. To that end I think it makes sense to actually pull the current
> value out of the toggle. This way, at least the value is set as expected
> when modyfing it within about privatebrowsing. I hope you agree.

I'm not objecting against taking the value out of the toggle. I'm objecting that the value is being stored on the button, instead of the checkbox, and that we ignore events on the checkbox and use only events on the button. :-)

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1430751_convert_about_privatebrowsing.patch

(In reply to :Gijs from comment #46)
> I'm not objecting against taking the value out of the toggle. I'm objecting
> that the value is being stored on the button, instead of the checkbox, and
> that we ignore events on the checkbox and use only events on the button. :-)

Now I get what you mean :-) That also answers the question that you raised earlier why we need to set the value on tpbutton. We don't need that anymore now either :-)
Thanks for your help with this part!

Comment 48

[:Gijs (he/him)]

Comment on attachment 8955063 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8955063 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!

::: browser/components/about/nsAboutCapabilities.js
@@ +8,5 @@
> +ChromeUtils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  AsyncPrefs: "resource://gre/modules/AsyncPrefs.jsm",
> +});

Nit: Because there's just one, we can just use:

ChromeUtils.defineModuleGetter(this, "AsyncPrefs", "resource://gre/modules/AsyncPrefs.jsm");

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8955063 [details] [diff] [review]
bug_1430751_convert_about_privatebrowsing.patch

Review of attachment 8955063 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good to me.

::: dom/base/nsIDocument.h
@@ +2868,5 @@
>    void GetReferrer(nsAString& aReferrer) const;
>    void GetLastModified(nsAString& aLastModified) const;
>    void GetReadyState(nsAString& aReadyState) const;
> +
> +  already_AddRefed<mozilla::dom::AboutCapabilities>GetAboutCapabilities(

Nit: this is missing a space after the >.

::: dom/webidl/AboutCapabilities.webidl
@@ +10,5 @@
> +  boolean getBoolPref(DOMString aPref, boolean? aDefaultValue);
> +  Promise<void> setBoolPref(DOMString aPref, boolean aValue);
> +
> +  DOMString getCharPref(DOMString aPref, DOMString? aDefaultValue);
> +  Promise<void> setCharPref(DOMString aPref, DOMString aValue);

The 'set' APIs should have a comment mentioning that AsyncPrefs.jsm has to know about the prefs to be set through this API.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9129a61155
Apply unified behavior model for about: pages to about:privatebrowsing. r=gijs,mrbkap

Comment 51

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ed9129a61155

Comment 52

[Dave Townsend [:mossop]]

I'm interested to know how this approach differs from that offered by RemotePageManager. Any thoughts?

Comment 53

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dave Townsend [:mossop] from comment #52)
> I'm interested to know how this approach differs from that offered by
> RemotePageManager. Any thoughts?

Dave, without diving into too much detail: the main benefit is that the new model only provides one entry point into the about page API which is guarded by the interface description and vetting of webidl. I agree, the actual implementation of AboutCapabilities is quite similar to RemotePageManager.jsm. I also understand the downside that the new model loses the ability to develop with artifact builds and also that webidl changes require a dom-peer review. The big advantage however is that the new model uses proven webidl based principal vetting where all the different consumers have to register within a single place, namely within CallerIsTrustedAboutPage() to get access to the API.

Ultimately we would like to unify the model for all of our about pages. Having said that I am wide open to discussions for improving the current model and resolve potential issues. Probably this bug is not the right forum though. In the end we would like to have a unified model that allows easy adoption.