956482 - Rewrite inline script in /browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml (about:privatebrowsing)

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[desiradaniel2007]

With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript. Since their amount is fairly limited, we start this by rewriting them bit by bit.

Comment 1

[desiradaniel2007]

Attachment: aboutPrivateBrowsingCSSJS.patch

I am not sure as to whether I got the URLs right and cannot test since I haven't configured the build system yet.

As commented in the markup, I have found platform-specific styles for about:privatebrowsing so I decided to create a new file with the inline style.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8361861 [details] [diff] [review]
aboutPrivateBrowsingCSSJS.patch

Thanks for the patch, Daniel. I asked Freddy for feedback on it.

Comment 3

[Frederik Braun [:freddy]]

Comment on attachment 8361861 [details] [diff] [review]
aboutPrivateBrowsingCSSJS.patch

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

This looks good in general. There are some minor questions I have, though:

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +3,5 @@
> +
> +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +
> +if (!PrivateBrowsingUtils.isWindowPrivate(window)) {
> +  document.title = "]]>&privatebrowsingpage.title.normal;<![CDATA[";

It think we had problems with these custom entities in similar bugs. Have you confirmed this works?

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
@@ +29,3 @@
>  
> +    <!-- Style shared between platforms -->
> +    <link rel="stylesheet" href="chrome://content/browser/aboutPrivateBrowsing.css" type="text/css" media="all"/>

There's already another aboutPrivateBrowsing.css referenced above. What's the reason behind having two separate files?

::: browser/components/privatebrowsing/jar.mn
@@ +4,3 @@
>  
>  browser.jar:
>  *   content/browser/aboutPrivateBrowsing.xhtml            (content/aboutPrivateBrowsing.xhtml) 

While you're at it, please remove the trailing whitespace (this is what we call un-needed space characters after an ending line).

Comment 4

[desiradaniel2007]

Snippet 1: Are referring to the string assigned to document.title? I will build on my system after reformatting HDD and fix.

Snippet 2: I believe we need that, since there seems to be different styles for Linux, OS X and Windows as commented. Would you confirm on this please?

Snippet 3: Will do.

Thanks for the feedback! :)

Comment 5

[desiradaniel2007]

Attachment: aboutprivatebrowsing.patch

Here are some other fixes I believe you missed. However, those are coming as I am having trouble with "./mach build". Shall file a build system bug on that. If you would like to/can help me with it, I will send you the output I get.

Comment 6

[Frederik Braun [:freddy]]

Sorry for not reaching out again, Daniel.
Do you still have build system issues? Feel free to try MDN and Google since some of these problems are common things that other people have dealt with already. If this doesnt help you can file a bug and CC me. I will be happy to help you moving things forward.

Can you also please refresh the patch to make sure it still applies? :)

Comment 7

[desiradaniel2007]

Hello again Fredric,

I've been passing through tough personal problems and have had some serious coursework to finish up. That's why it took me another 3 months to come back.

I have recently started following CodeFirefox, which has cleared much of my confusion, but after a full build, I am getting a message that I need to clobber which I now did and had it result in a fail since "mozilla-central\obj-i686-pc-mingw32\" is not empty.

Googling for solutions did not really help. Is it possible for you to help me over this thread, Twitter or IRC please? Or is filing a Build bug more appropriate? Thanks.

Comment 8

[Frederik Braun [:freddy]]

You can try asking in #developers (irc.mozilla.org) before you file a bug for the build system. But I think you can safely delete all "obj-*" dirs when you do a clobbered build.

Comment 9

[desiradaniel2007]

On running a successful build, I realized that my patch breaks the page, which mach reports 2 warnings for aboutPrivateBrowsing.xhtml and aboutPrivateBrowsing.css: no useful preprocessor directives found

Will be hopefully solving this by sometime this week. Thanks! :)

Comment 10

[Daniel Desira]

Attachment: patch 3

Here's the patch but I had to hack around &privatebrowsingpage.title because it does not work right. Suggestions?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8473151 [details] [diff] [review]
patch 3

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

This is close, just a couple things to address.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +7,5 @@
> +
> +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +
> +if (!PrivateBrowsingUtils.isWindowPrivate(window)) {
> +  document.title = "&privatebrowsingpage.title.normal;";

This isn't going to work. You'll need to move these strings to a .properties file and use a StringBundle to load the string from the string bundle to assign it to document.title.

@@ +11,5 @@
> +  document.title = "&privatebrowsingpage.title.normal;";
> +  setFavIcon("chrome://global/skin/icons/question-16.png");
> +} else {
> +#ifndef XP_MACOSX
> +  //document.title = "&privatebrowsingpage.title;";

Why is this line commented out? I see that this is redundant, since the title is already set to be this value within the XHTML file, but we shouldn't show a title that might get changed in the next instant. So let's go with only setting the title from the JS file.

As a result, you can remove the `*` from the jar.mn for the XHTML file since we won't need preprocessing there anymore. Also, you can remove the `#` characters from the license header there since we won't be preprocessing that file anymore and the `#` was only used to comment out the license header from the build output.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
@@ +23,3 @@
>      <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all"/>
>      <link rel="stylesheet" href="chrome://browser/skin/aboutPrivateBrowsing.css" type="text/css" media="all"/>
> +    <link rel="stylesheet" href="chrome://browser/content/aboutPrivateBrowsing.css" type="text/css"></link>

Please move the content stylesheet to be included before the skin stylesheet, so that themes can override the content styling if necessary.

Comment 12

[desiradaniel2007]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Comment on attachment 8473151 [details] [diff] [review]
> patch 3
> 
> Review of attachment 8473151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is close, just a couple things to address.
> 
> ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
> @@ +7,5 @@
> > +
> > +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> > +
> > +if (!PrivateBrowsingUtils.isWindowPrivate(window)) {
> > +  document.title = "&privatebrowsingpage.title.normal;";
> 
> This isn't going to work. You'll need to move these strings to a .properties
> file and use a StringBundle to load the string from the string bundle to
> assign it to document.title.
> 
> @@ +11,5 @@
> > +  document.title = "&privatebrowsingpage.title.normal;";
> > +  setFavIcon("chrome://global/skin/icons/question-16.png");
> > +} else {
> > +#ifndef XP_MACOSX
> > +  //document.title = "&privatebrowsingpage.title;";
> 
> Why is this line commented out? I see that this is redundant, since the
> title is already set to be this value within the XHTML file, but we
> shouldn't show a title that might get changed in the next instant. So let's
> go with only setting the title from the JS file.
> 
> As a result, you can remove the `*` from the jar.mn for the XHTML file since
> we won't need preprocessing there anymore. Also, you can remove the `#`
> characters from the license header there since we won't be preprocessing
> that file anymore and the `#` was only used to comment out the license
> header from the build output.
> 

Are you sure about that? There are still some XHTML entities remaining once &privatebrowsingpage.title is removed so I don't know know how to go about it. :)

> ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
> @@ +23,3 @@
> >      <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all"/>
> >      <link rel="stylesheet" href="chrome://browser/skin/aboutPrivateBrowsing.css" type="text/css" media="all"/>
> > +    <link rel="stylesheet" href="chrome://browser/content/aboutPrivateBrowsing.css" type="text/css"></link>
> 
> Please move the content stylesheet to be included before the skin
> stylesheet, so that themes can override the content styling if necessary.

Done!

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

The XHTML entities will still be inserted appropriately even when preprocessing is disabled.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

The preprocessing was only needed for the `#ifndef` and `#endif` tokens.

Comment 15

[Daniel Desira]

So I ended up using the string bundle service since <stringbundle> is not available in XHTML. Spent some time Googling and asking on IRC and following https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Miscellaneous#Using_string_bundles_from_JavaScript

Only one thing is unclear: How should I construct .properties file URL relative to directory.

Comment 16

[Daniel Desira]

Quick update: Almost done. Just some merge conflicts to overcome due to recent changes.

Comment 17

[Daniel Desira]

Attachment: aboutPrivateBrowsing

This should do it. Proven to work for the new about:privatebrowsing! :D

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8485860 [details] [diff] [review]
aboutPrivateBrowsing

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

Sorry for not getting to this until now.

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml
@@ -30,5 @@
> -        setFavIcon("chrome://global/skin/icons/question-16.png");
> -      } else {
> -#ifndef XP_MACOSX
> -        document.title = "]]>&aboutPrivateBrowsing.title;<![CDATA[";
> -#endif

Weird, so previously new tabs on Mac in Private Browsing mode would show "New Tab" as the page title, but on Windows and Linux we showed "You're browsing privately". "You're browsing privately" probably doesn't make much sense for the title of new tabs in private browsing mode, with maybe the exception of the first tab.

Let's stick with what you've got for this patch and file a new bug to figure out what we want the tab titles to be for new tabs in Private Browsing mode.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

I had to update your patch since http://hg.mozilla.org/mozilla-central/rev/380f8554aa17 just landed and added some more classes to elements within the page.

I pushed the updated patch to the tryserver so we can run it through all of the automated tests. After the tests finish, we'll know if anything was broken from these changes.

If everything looks good, we can add the checkin-needed keyword to this bug. The patch will then be checked-in and should appear in Firefox Nightly within a couple days.

You can view the progress of your build at the following URL:
https://tbpl.mozilla.org/?tree=Try&rev=0b63634d0440

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8485860 [details] [diff] [review]
aboutPrivateBrowsing

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

Thanks for the patch, nicely done!

I've landed the patch for you:
https://hg.mozilla.org/integration/fx-team/rev/4b5f794b80a8

Below are the changes I made:

> Bug 956442 - Rewrite inline script in /browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml (about:privatebrowsing); r=reviewers

I fixed the bug number in your commit message (should be 956482).

::: 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.css              (content/aboutPrivateBrowsing.css)

This file still needs to be pre-processed due to the license header.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for debug OSX perma-fail.
https://hg.mozilla.org/integration/fx-team/rev/a6e449e2c13e

https://tbpl.mozilla.org/php/getParsedLog.php?id=47913954&tree=Fx-Team

Comment 22

[desiradaniel2007]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> Backed out for debug OSX perma-fail.
> https://hg.mozilla.org/integration/fx-team/rev/a6e449e2c13e
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47913954&tree=Fx-Team

Any insight to what we could do about that?

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to desiradaniel2007 from comment #22)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> > Backed out for debug OSX perma-fail.
> > https://hg.mozilla.org/integration/fx-team/rev/a6e449e2c13e
> > 
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=47913954&tree=Fx-Team
> 
> Any insight to what we could do about that?

It looks like browser_privatebrowsing_windowtitle.js needs to be updated now that OSX uses the same title for new private windows as Windows/Linux.

Comment 24

[desiradaniel2007]

We also need to clarifiy whether the names I used in the properties file are compliant with the coding conventions.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23) 
> It looks like browser_privatebrowsing_windowtitle.js needs to be updated now
> that OSX uses the same title for new private windows as Windows/Linux.

Hmm, that's likely to be better to be tracked within another bug as it should have been introduced by a change other than mine. Although I don't own a mac, I am willing to give it a shot.

Comment 25

[desiradaniel2007]

Hi guys, sorry for taking so long on this one. I have just checked the diff for my patch and realized I mistakenly used the older version of the page so perhaps simply rewriting my patch over the last code-base could do it.

Expect a working patch any time soon. :)

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Sounds good, thanks for the update.

Comment 27

[Daniel Desira]

Attachment: aboutPrivateBrowsing2

Please build and test on OS X.

Comment 28

[desiradaniel2007]

Any update regarding review please?

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Hey, what a coincidence. I'm actually doing the review as I write this. I had to get my OSX build up to date. Sorry, I should have commented in here earlier.

Comment 30

[desiradaniel2007]

Thanks! :)

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8515327 [details] [diff] [review]
aboutPrivateBrowsing2

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

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js
@@ +13,5 @@
> +  document.title = stringBundle.GetStringFromName("title.normal");
> +  setFavIcon("chrome://global/skin/icons/question-16.png");
> +} else {
> +#ifndef XP_MACOSX
> +  document.title = stringBundle.GetStringFromName("title");

So there is still an issue here where the XP_MACOSX symbol is not defined in this file. I'm not sure why.

We can drop this special casing though, and just make all new tabs in private windows have this string. It doesn't make sense that OSX gets a different new tab title for private windows. Note that you'll still have to update the test as mentioned in comment #23.

Comment 32

[desiradaniel2007]

I'm sorry, I missed that! Will give it a try later on this week. Should reply by Sunday. Cheers!

Comment 33

[Daniel Desira]

Attachment: aboutPrivateBrowsing2

Hi, I have modified the test and am not sure whether I have done everything needed but please let me know :)

Comment 34

[desiradaniel2007]

Has anyone running OS X re-run the test?

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

The test still hangs for me. I'm looking a bit deeper. We should get you level-1 try access so you can push to the tryserver and have it run the OSX tests for you :)

You can follow the instructions here, https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch with test fixed

Ok, I fixed the test on Mac. Daniel, can you run the test on your machine and see if it passes for you in your environment still? To run it, you should run `mach mochitest-browser browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js`

Comment 37

[desiradaniel2007]

It's a shame for me that I did not do it properly. That was too easy! Will run the test as soon as I'm on that machine. Thanks :)

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

I tested out that patch on Windows and noticed that it now failed on Windows. With a minor tweak it should now work everywhere.

I pushed it to the tryserver to make sure that it will work everywhere now.

The rest of the patch looks good. Once the tryserver results come back and show that nothing has failed, we can get this checked in.

Nice work and congratulations on getting your first patch r+'d \o/

You can view the progress of your build at the following URL:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b2e5664d809e
Alternatively, view them on TBPL (soon to be deprecated):
  https://tbpl.mozilla.org/?tree=Try&rev=b2e5664d809e

Comment 39

[Daniel Desira]

Hey, thanks! I have just run it on my system (Win7) and it seems to pass. (No fail message.)

Actually, this is my second patch.

Will be keeping tabs on the try build's progress to see if there's anything I can do.

Thanks again! :)

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

Ah, you're multiple bugzilla accounts (that only differ by the domain) confused me. Nonetheless, the congrats stays ;)

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for checkin

I just noticed that the commit message had the wrong bug number listed.

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/27b0b0678204

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27b0b0678204

Comment 44

[desiradaniel2007]

Again? :/ Thanks for the coordination.

Comment 45

[Francesco Lodolo [:flod]]

You're also missing a license header in the .properties file.

Comment 46

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #45)
> You're also missing a license header in the .properties file.

Also, file is using Windows line ending, which is definitely unusual (not sure if there's a policy for that).

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

Added the license header and fixed the line endings,
https://hg.mozilla.org/integration/fx-team/rev/6b7bd35aca3d

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b7bd35aca3d

Comment 49

[desiradaniel2007]

Cheers guys. Notes taken for next patch!

Comment 50

[Ada [:adalucinet]]

As written in comment 18, reproduced with Nightly from 2014-09-21 under Mac OS X 10.8.5 the fact that 'New tab' is displayed as title when opening a PB window. With Fx 36 beta 8 (Build ID: 20150209164123), 'You're browsing privately' is displayed on Windows 7 64-bit, Mac OS X 10.8.5 and Ubuntu 14.04 32-bit.
Please let me know at what else should I look in order to properly verify this bug. Thanks in advance!

Comment 51

[Jared Wein [:jaws] (please needinfo? me)]

That should be enough to verify this bug. Thanks!