Closed Bug 1444439 Opened 3 years ago Closed 3 years ago

Remove unsafeSetInnerHTML in bug418986-2.js

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: johannh, Assigned: prathiksha)

References

Details

Attachments

(1 file)

Bug 1444439 - Remove unsafeSetInnerHTML from bug418986-2.js.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
We're using unsafeSetInnerHTML to inject HTML in the bug418986-2.js file which is used in https://searchfox.org/mozilla-central/source/layout/style/test/test_bug418986-2.html#13 and https://searchfox.org/mozilla-central/source/layout/style/test/chrome/test_bug418986-2.xul#20.

While that in itself is not dangerous, we're aiming to remove unsafeSetInnerHTML as soon as possible, and so we'll need to rewrite the test.

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/layout/style/test/chrome/bug418986-2.js#242
[Triage 2018/03/23 - P3]
Priority: -- → P3
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Comment on attachment 8965927 [details]
Bug 1444439 - Remove unsafeSetInnerHTML from bug418986-2.js.

https://reviewboard.mozilla.org/r/234736/#review240520

This looks fine to me, with nits fixed :)

Thank you!

::: layout/style/test/chrome/bug418986-2.js:149
(Diff revision 2)
>  // __generateHtmlLines(resisting)__.
>  // Create a series of div elements that look like:
>  // `<div class='spoof' id='resolution'>resolution</div>`,
>  // where each line corresponds to a different media query.
>  var generateHtmlLines = function (resisting) {
> -  let lines = "";
> +  let lines = document.createElement("div");

If I'm not mistaken the original "lines" was a bunch of divs without a parent div to contain them, so you should probably use a document fragment here instead.

::: layout/style/test/chrome/bug418986-2.js:171
(Diff revision 2)
> +      div.textContent = key;
> +      lines.appendChild(div);
>      });
>    if (OS === "WINNT") {
> -    lines += "<div class='windows' id='-moz-os-version'>-moz-os-version</div>";
> -    lines += "<div class='windows' id='-moz-windows-theme'>-moz-windows-theme</div>";
> +    let ids = ["-moz-os-version", "-moz-windows-theme"];
> +    for (let i = 0; i < ids.length; i++) {

nit: you could do for (let id of ids)

::: layout/style/test/chrome/bug418986-2.js:259
(Diff revision 2)
>  // Creates a series of divs and CSS using media queries to set their
>  // background color. If all media queries match as expected, then
>  // all divs should have a green background color.
>  var testCSS = function (resisting) {
> -  document.getElementById("display").unsafeSetInnerHTML(generateHtmlLines(resisting));
> -  document.getElementById("test-css").unsafeSetInnerHTML(generateCSSLines(resisting));
> +  document.getElementById("display").appendChild(generateHtmlLines(resisting));
> +  document.getElementById("test-css").textContent = generateCSSLines(resisting);

Huh, I was not aware that CSS style could be set with .textContent. Good if that works...

::: layout/style/test/chrome/bug418986-2.js:292
(Diff revision 2)
>  // __testMediaQueriesInPictureElements(resisting)__.
>  // Test to see if media queries are properly spoofed in picture elements
>  // when we are resisting fingerprinting. A generator function
>  // to be used with SpawnTask.js.
>  var testMediaQueriesInPictureElements = async function(resisting) {
> -  let lines = "";
> +  let lines = document.createElementNS(HTML_NS, "picture");

Can you rename this variable to "picture" or something like that?
Attachment #8965927 - Flags: review?(jhofmann) → review+
https://hg.mozilla.org/mozilla-central/rev/a88b99cfec8c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.