Closed Bug 948882 Opened 9 years ago Closed 5 years ago

Move inline scripts and styles into separate file for toolkit/crashreporter/content/crashes.xhtml (URL=about:crashes)

Categories

(Firefox :: General, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: freddy, Assigned: tiago, Mentored, NeedInfo)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 6 obsolete files)

bug_final.patch
5.00 KB, patch
freddy
: feedback+
Details | Diff | Splinter Review
Bug 948882 - Move inline scripts and styles into separate file for toolkit/crashreporter/content/crashes.xhtml (URL=about:crashes).
59 bytes, text/x-review-board-request
freddy
: review+
Details 
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.
Hello I want to work on this. I am very new but I want to start work here.Please guide me how to do that.
Hi this is my first try at bug so questions maybe bit naive. The JS for this already seems to have been moved to separate crashes.js. But I found this css that was still inline 

<style type="text/css">
:root {
  font-family: sans-serif;
}
table {
  padding-bottom: 2em;
}
th {
  text-align: left;
  white-space: nowrap;
}
th[chromedir="rtl"] {
  text-align: right;
}
/* name */
th:first-child {
  -moz-padding-end: 2em;
}
:link, :visited {
  display: block;
  min-height: 17px;
}
/* date */
td:first-child + td {
  -moz-padding-start: 1em;
  -moz-padding-end: .5em;
  white-space: nowrap;
}
/* time */
td:last-child {
  -moz-padding-start: .5em;
  white-space: nowrap;
}

#clear-reports {
  float: right;
}
#clear-reports[chromedir="rtl"] {
  float: left;
}

.submitting {
  background-image: url(chrome://global/skin/icons/loading_16.png);
  background-repeat: no-repeat;
  background-position: right;
}
</style>

Does this needs to be moved too? If so I will submit a patch for it soon.
Yes, the inline style should be moved to an external file. I haven't checked dirListing.css, but it looks like we should create a new file for this (crashes.css or so).

Please also move the inline JavaScript in line 77 (onclick=...) to crashes.js.
Hi, 
I was looking for some good first bugs and stumbles upon this one. I would like to start with this as my first bug. Can I work on it? Thanks :)
I was hoping that the person in comment 2 will continue their work. 
lp, please take another bug :)
Assignee: nobody → shailrishabh
Rishabh, do you still plan to work on this?


(Someone in #introduction is interested in this bug too)
Flags: needinfo?(shailrishabh)
Never mind, he found a bug.
Hey Sorry Manish I totally missed it I would like to continue the work on this bug if you don't mind. I will try to send a patch as soon as possible
Flags: needinfo?(shailrishabh)
(In reply to shailrishabh from comment #8)
> Hey Sorry Manish I totally missed it I would like to continue the work on
> this bug if you don't mind. I will try to send a patch as soon as possible

No hurry though. I was just wondering if you were still interested :)
Status: NEW → ASSIGNED
(In reply to shailrishabh from comment #8)
> Hey Sorry Manish I totally missed it I would like to continue the work on
> this bug if you don't mind. I will try to send a patch as soon as possible

Any news?
Attached patch bug-948882.patch (obsolete) — — Splinter Review 
I am noob here.This is my first patch .
Attachment #8407744 - Flags: review?(fbraun)
Assignee: shailrishabh → brnet00
Comment on attachment 8407744 [details] [diff] [review]
bug-948882.patch

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

This looks good, aside from the small nits below :)

::: toolkit/crashreporter/content/crashes.xhtml
@@ +13,5 @@
>  ]>
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>  <head>
> +<link rel="stylesheet" type="text/css" href="chrome://global/content/crashes.css" >

Nit: Change the extra space here at the end to a />

(instead of `...crashes.css" >`, have `...crashes.css"/>`)

::: toolkit/crashreporter/jar.mn
@@ +4,5 @@
>  
>  toolkit.jar:
>    content/global/crashes.xhtml            (content/crashes.xhtml)
>    content/global/crashes.js               (content/crashes.js)
> +  content/global/crashes.css               (content/crashes.css)

Nit: There's an extra space, try to align the parentheses.
Attachment #8407744 - Flags: feedback+
Attached patch bug.patch (obsolete) — — Splinter Review 
Hi Manish 

Added !
Attachment #8408443 - Flags: feedback?(manishearth)
Attachment #8407744 - Attachment is obsolete: true
Attachment #8407744 - Flags: review?(fbraun)
Comment on attachment 8408443 [details] [diff] [review]
bug.patch

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

This looks good :)
Attachment #8408443 - Flags: review?(fbraun)
Attachment #8408443 - Flags: feedback?(manishearth)
Attachment #8408443 - Flags: feedback+
Comment on attachment 8408443 [details] [diff] [review]
bug.patch

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

OK, looking good for a first start :-)
This removes the inline styles but leaves inline scripts. Can you modify crashes.xhtml so that "onclick" and "onload" attributes are replaced by JavaScript that adds event handlers (in crashes.js)?

You can look how other volunteers have fixed this (In bug 948881 for example).
See also our project wiki page! https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles
Attachment #8408443 - Flags: review?(fbraun) → feedback+
Attached patch bug_final.patch (obsolete) — — Splinter Review 
Hello Frederik 

Done ! I have attached here also.
Attachment #8408443 - Attachment is obsolete: true
Attachment #8410229 - Flags: review?(fbraun)
Comment on attachment 8410229 [details] [diff] [review]
bug_final.patch

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

This looks good. Some indentation may be called for (split the handlers across multiple lines), but otherwise it should work.
Attachment #8410229 - Flags: feedback+
This looks good.
Can you use DOMContentLoaded instead of window's onload?
Attachment #8410229 - Flags: review?(fbraun) → feedback+
Attached patch bug_final.patch (obsolete) — — Splinter Review 
Done !
Attachment #8410229 - Attachment is obsolete: true
Attachment #8410903 - Flags: feedback?(manishearth)
Attachment #8410903 - Flags: feedback?(fbraun)
Comment on attachment 8410903 [details] [diff] [review]
bug_final.patch

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

One small fix, and then I think you're all set :)

::: toolkit/crashreporter/content/crashes.js
@@ +15,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "CrashSubmit",
>    "resource://gre/modules/CrashSubmit.jsm");
>  
> +document.getElementById('clear-reports').addEventListener('click', function () { clearReports().then(null, Cu.reportError); });
> +document.addEventListener( "DOMContentLoaded", function () { populateReportList(); }, false );

Nit: There's an extra space right before "DomContentLoaded"
Attachment #8410903 - Flags: feedback?(manishearth) → feedback+
Attached patch bug_final.patch (obsolete) — — Splinter Review 
Sorry ! Thanks for pointing out
Attachment #8410903 - Attachment is obsolete: true
Attachment #8410903 - Flags: feedback?(fbraun)
Attachment #8410915 - Flags: review?(fbraun)
Attachment #8410915 - Flags: feedback?(manishearth)
Attachment #8410915 - Flags: feedback?(manishearth) → feedback+
Flags: needinfo?(malexis)
whoops
Flags: needinfo?(malexis)
Comment on attachment 8410915 [details] [diff] [review]
bug_final.patch

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

Good progress!

::: toolkit/crashreporter/content/crashes.js
@@ +14,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "CrashSubmit",
>    "resource://gre/modules/CrashSubmit.jsm");
>  
> +document.getElementById('clear-reports').addEventListener('click', function () { clearReports().then(null, Cu.reportError); });

Sorry I *just* noticed this: The tag with the 'clear-reports' Id is only available after the page has loaded, not when the script is executed.
You could add some newlines about the following DOMContentLoaded handler and add this click event handler in there!

Would you please test your next patch? I know it's a steep curve, but it's not that hard: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Attachment #8410915 - Flags: review?(fbraun)
Attachment #8410915 - Flags: feedback+
Biraj, can you address my recent comments? This patch is nearly ready!
Flags: needinfo?(brnet00)
Attached patch bug_final.patch (obsolete) — — Splinter Review 
sorry for late reply. I was busy with my project.

I have attached the patch here.
Attachment #8421506 - Flags: review?(fbraun)
Flags: needinfo?(brnet00)
Comment on attachment 8421506 [details] [diff] [review]
bug_final.patch

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

Looks good. One more thing though:

::: toolkit/crashreporter/content/crashes.js
@@ +14,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "CrashSubmit",
>    "resource://gre/modules/CrashSubmit.jsm");
> +document.addEventListener("DOMContentLoaded", function () { populateReportList(); }, false );
> +document.getElementById('clear-reports').addEventListener('click', function () { clearReports().then(null, Cu.reportError); });

Adding the event listener for clear-reports has to be placed in the event handler for DOMContentLoaded, otherwise JavaScript doesn't see the button yet.

Can you please address this and test your next patch?
Attachment #8421506 - Flags: review?(fbraun) → feedback+
Attached patch bug_final.patch — — Splinter Review 
Added !
Attachment #8410915 - Attachment is obsolete: true
Attachment #8421506 - Attachment is obsolete: true
Attachment #8424383 - Flags: review?(fbraun)
Comment on attachment 8424383 [details] [diff] [review]
bug_final.patch

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

Closer and closer :)
Please address the following comments:

::: toolkit/crashreporter/content/crashes.js
@@ +14,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "CrashSubmit",
>    "resource://gre/modules/CrashSubmit.jsm");
> +document.addEventListener("DOMContentLoaded", function () { populateReportList(); }, false );
> +document.addEventListener("DOMContentLoaded", function () { document.querySelector('clear-reports').addEventListener('click', function () { clearReports().then(null, Cu.reportError); }) }, false);

I just noticed your querySelector is not working. It is also better for performance if you use document.getElementById() instead.
You can wrap all your DOMContentLoaded things into one function. Please introduce line breaks after { and } and use indentation as discussed in our style guides (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices)

You may start with something like this:

> document.addEventListener("DOMContentLoaded", function () { 
>  populateReportList();
>  //TODO insert getElementById fix, including another set of newlines and indentation
> });

Thanks!
Attachment #8424383 - Flags: review?(fbraun) → feedback+
Mentor: fbraun
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [lang=html][good first bug][lang=js]
Comment on attachment 8868445 [details]
Bug 948882 - Move inline scripts and styles into separate file for toolkit/crashreporter/content/crashes.xhtml (URL=about:crashes).

https://reviewboard.mozilla.org/r/140048/#review143376

Looks good! Note, that I am not a browser peer, so we need another reviewer here.
Attachment #8868445 - Flags: review?(fbraun) → review+
Ok, thank you!

I'm new around here, do you have any idea who could be a browser peer or where to find one?

Thanks again!
Flags: needinfo?(fbraun)
Excellent. I'll let you know how other folks do it, instead of magically coming up with a name.
You said you're new here, but it looks like your other patches went really well, so I trust you'll be up to this challenge?

I usually look into 'hg log <file>' to find previous commits. The commit message has reviewer names, that I then hunt on IRC.
Try looking at <https://hg.mozilla.org/mozilla-central/log/tip/toolkit/crashreporter/content/crashes.xhtml> and find someone in #fx-team or #developers.

P.S: I was lazy enough to search for "crashes.xhtml" in searchfox.org and then find a link to the public version history.
Flags: needinfo?(fbraun)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1034f8be097f
Move inline scripts and styles into separate file for toolkit/crashreporter/content/crashes.xhtml (about:crashes). r=freddyb,gijs
I happened to come across this bug, so rs=me on the patch from Santiago, and I landed it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1034f8be097fd6d2f8eda8f7f7d277b92d91ff41
Assignee: brnet00 → tiago.paez11
Backed out for ESlint failures on /toolkit/crashreporter/content/crashes.js

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/dfff9e92ca9710b2e56e0a38f86362b7a8903049

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1034f8be097fd6d2f8eda8f7f7d277b92d91ff41&selectedJob=183762018

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183762018&repo=mozilla-inbound&lineNumber=245

[task 2018-06-19T10:03:36.725Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-06-19T10:03:36.725Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-06-19T10:03:36.725Z] 
[task 2018-06-19T10:03:36.725Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-06-19T10:08:46.466Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/crashreporter/content/crashes.js:15:55 | Unexpected space before function parentheses. (space-before-function-paren)
[task 2018-06-19T10:08:46.466Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/crashreporter/content/crashes.js:17:27 | Strings must use doublequote. (quotes)
[task 2018-06-19T10:08:46.466Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/crashreporter/content/crashes.js:17:78 | Unexpected space before function parentheses. (space-before-function-paren)
[taskcluster 2018-06-19 10:08:46.842Z] === Task Finished ===
[taskcluster 2018-06-19 10:08:46.842Z] Unsuccessful task run with exit code: 1 completed in 551.489 seconds
Flags: needinfo?(tiago.paez11)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75df8c6facf7
Move inline scripts and styles into separate file for toolkit/crashreporter/content/crashes.xhtml (about:crashes). r=freddyb,gijs
https://hg.mozilla.org/mozilla-central/rev/75df8c6facf7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.