Closed Bug 948892 Opened 11 years ago Closed 9 years ago

Move inline scripts and styles into separate file for mobile/android/chrome/content/about.xhtml (URL=about:)

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 37

People

(Reporter: freddy, Assigned: mandarg, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=html][good first bug][lang=js])

Attachments

(1 file, 5 obsolete files)

0001-Bug-948892-Move-inline-scripts-and-styles-into-separ.patch
11.81 KB, patch
Margaret
: review+
Details | Diff | Splinter Review 
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.
Attached patch move-inline-js-about.patch (obsolete) — — Splinter Review 
Added patch. Can someone review this?

[Note: This is my first attempt at a contribution to Mozilla, so please bear with any faux pas I may be committing.]
Attachment #8351050 - Flags: review+
Attachment #8351050 - Flags: review?(mark.finkle)
Attachment #8351050 - Flags: review?(margaret.leibovic)
Attachment #8351050 - Flags: review+
Assignee: nobody → mozbugs.retornam
Comment on attachment 8351050 [details] [diff] [review]
move-inline-js-about.patch

>diff --git a/mobile/android/chrome/content/about.js b/mobile/android/chrome/content/about.js

>+let Ci = Components.interfaces, Cc = Components.classes, Cu = Components.utils, Cr = Components.results;

The new preferred way of doing this seems to be:
const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

>+      Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+      Cu.import("resource://gre/modules/Services.jsm");

Your indent is off in the rest of the file

I am a little worried about keeping all of this code at the global scope. Normally I would suggest putting it in a window.addEventListener("load", ...) callback. The original code was global because the script was at the bottom of the HTML and executed after content is loaded. With the JS being in a file, I kinda want to be a little more robust about making sure this won't break if someone moves the <script> tag in the HTML file up to the <head>.

Margaret can give some more thought about this.

Treating this as a feedback+
Attachment #8351050 - Flags: review?(mark.finkle) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #2)

> I am a little worried about keeping all of this code at the global scope.
> Normally I would suggest putting it in a window.addEventListener("load",
> ...) callback. The original code was global because the script was at the
> bottom of the HTML and executed after content is loaded. With the JS being
> in a file, I kinda want to be a little more robust about making sure this
> won't break if someone moves the <script> tag in the HTML file up to the
> <head>.

I agree we should put the JS that depends on the DOM being ready inside a DOMContentLoaded listener, especially if we're moving the JS into its own file where it's not obvious when it's being run.
Comment on attachment 8351050 [details] [diff] [review]
move-inline-js-about.patch

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

I second what mfinkle said about fixing the indentation. As a quick fix for the DOM ready concern, you could just wrap lines 9-58 in a function that you set as a callback for a "DOMContentLoaded" listener, and do the same thing for the stuff inside the #ifdef MOZ_UPDATER block.

::: mobile/android/chrome/content/about.js
@@ +102,5 @@
> +        noneSpan.style.display = "none";
> +        foundSpan.style.display = "none";
> +        downloadingSpan.style.display = "none";
> +        downloadedSpan.style.display = "none";
> +        updateLink.style.display = "block";

This is pretty gnarly. We should file a new bug to do this all with CSS, I can mentor that bug as well!
Attachment #8351050 - Flags: review?(margaret.leibovic) → feedback+
Status: NEW → ASSIGNED
Hey, mandarg.
You received good feedback on your first attempt. Would you be willing to continue here?
Assignee: mozbugs.retornam → mandarg
Flags: needinfo?(mandarg)
Add patch partially addressing comments by mfinkle
Attachment #8351050 - Attachment is obsolete: true
Flags: needinfo?(mandarg)
I would be willing to continue here. I have addressed the comments by mfinkle. 

I still have to address the robustness part, and the DOMContentLoaded listener by Margaret Leibovic. Is there an example of the same elsewhere in the codebase that I could refer to? 

Currently the only similar instance I seem to see is lines 62-74 in browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml -- should I do something like that?
Your partial patch looks good.
For the DOMContentLoaded things you can refer to the bits in aboutPrivatebrowsing.xhtml. If you want you can also look at the Wiki page https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles#Finding_Inspiration - the last section links to similar bugs that have already been fixed. They should contain similar examples.
Mentor: fbraun
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [lang=html][good first bug][lang=js]
mandarg, are you still interested in working on this bug?
Mentor: margaret.leibovic
Flags: needinfo?(mandarg)
Yes, I would be. Here is an updated patch that I *think* might be doing the right thing. Could you take a look at it?
(In reply to [:mandarg] from comment #10)
> Yes, I would be. Here is an updated patch that I *think* might be doing the
> right thing. Could you take a look at it?

I'd be happy to look at a new patch. Can you attach it to this bug and flag me for review?
Added patch.
Attachment #8420609 - Attachment is obsolete: true
Flags: needinfo?(mandarg)
Attachment #8543378 - Flags: feedback+
Attachment #8543378 - Flags: feedback+ → feedback?(margaret.leibovic)
Comment on attachment 8543378 [details] [diff] [review]
0001-Bug-948892-Move-inline-scripts-and-styles-into-separ.patch

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

Heading in the right direction, but there are some problems here we'll need to address before we land this.

::: mobile/android/chrome/content/about.js
@@ +6,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function getBuildInfo() {
> +    // Include the build date and a warning about Telemetry

Please use 2-space indentation in this file.

@@ +161,5 @@
> +#endif
> +}
> +
> +document.addEventListener("DOMContentLoaded", getBuildInfo, false);
> +document.addEventListener("DOMContentLoaded", updateCheck, false);

Instead of creating two different functions here, I think you should just wrap all the logic you copied over inside a single 'init' function.

::: mobile/android/chrome/content/about.xhtml
@@ +71,4 @@
>      </div>
>  #endif
>  
> +    <script type="application/javascript;version=1.8" src="chrome://global/content/about.js" />

This is loading the about.js included in toolkit. You want to load chrome://browser/content/about.js.

Did you test this patch out locally? You should have noticed that your JS wasn't being loaded.

::: mobile/android/chrome/jar.mn
@@ +8,4 @@
>  % content browser %content/ contentaccessible=yes
>  
>  * content/about.xhtml                  (content/about.xhtml)
> +  content/about.js                     (content/about.js)

You need to include an asterisk on this line as well, since about.js includes pre-processor directives.
Attachment #8543378 - Flags: feedback?(margaret.leibovic) → feedback+
Thanks for the quick review!

> Please use 2-space indentation in this file.

Reformatted, this should be resolved now.

> This is loading the about.js included in toolkit. You want to load chrome://browser/content/about.js.
> Did you test this patch out locally? You should have noticed that your JS wasn't being loaded.

Fixed. I had not tested locally previously – I was having problems with the bootstrap script. Ran through the steps manually, built Fennec (with these changes), and looked at 'about:' on the custom Fennec build on my phone. Looks correct as far as I can see.

> You need to include an asterisk on this line as well, since about.js includes pre-processor directives.

Fixed.

Uploading a new patch now.
Attachment #8543378 - Attachment is obsolete: true
Attachment #8543480 - Flags: feedback?(margaret.leibovic)
Attachment #8543481 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8543481 [details] [diff] [review]
0001-Bug-948892-Move-inline-scripts-and-styles-into-separ.patch

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

Looking good! I just have a few small pieces of feedback. You can request review on your next version, and then we can land it once I grant a review+!

::: mobile/android/chrome/content/about.js
@@ +57,5 @@
> +      pref: "app.privacyURL"
> +    }, {
> +      id: "creditsURL",
> +      pref: "app.creditsURL"
> +    }, ];

I must have failed to comment on this in my last review, but I think you should keep the formatting for the array as it was originally, so that this trailing comma makes more sense.

@@ +88,5 @@
> +    showCheckingMessage();
> +
> +    Services.androidBridge.handleGeckoMessage({
> +      type: "Update:Check"
> +    });

You can also keep the formatting for these handleGeckoMessage calls the same.
Attachment #8543481 - Flags: feedback?(margaret.leibovic) → feedback+
> I must have failed to comment on this in my last review, but I think you should keep the formatting for the array as it was originally, so that this trailing comma makes more sense.

Done.

> You can also keep the formatting for these handleGeckoMessage calls the same.

Done (with two-space indents for the function body).

Built, then tested on my Nexus 4. 'about:' page worked.
Attachment #8543481 - Attachment is obsolete: true
Attachment #8545209 - Flags: review?(margaret.leibovic)
Attachment #8545209 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8545209 [details] [diff] [review]
0001-Bug-948892-Move-inline-scripts-and-styles-into-separ.patch

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

Excellent!
Attachment #8545209 - Flags: review?(margaret.leibovic)
Attachment #8545209 - Flags: review+
Attachment #8545209 - Flags: feedback?(margaret.leibovic)
Thank you so much for the patch! Let me know if you need help finding other bugs to work on. Feel free to join #mobile on irc.mozilla.org to chat about any Firefox for Android bugs you find that you'd like to work on.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/470845ebf534
Keywords: checkin-needed
Whiteboard: [lang=html][good first bug][lang=js] → [lang=html][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/470845ebf534
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=html][good first bug][lang=js][fixed-in-fx-team] → [lang=html][good first bug][lang=js]
Target Milestone: --- → Firefox 37
Depends on: 1120080
Bug 1120080 claims that this broke our manual update check
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: