1. Your add-on creates DOM nodes from HTML strings containing
potentially unsanitized data, by assigning to innerHTML, jQuery.html, or
through similar means. Aside from being inefficient, this is a major
security risk. For more information, see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion .
Please fix them and submit again. Thank you.
The function causing this should be this one:
It’s slightly modified but it uses innerHTML in the same way of the one I’ve submitted.
I’m calling the function as an example of how it works, the words or regular expressions are given by the user then i look for the text nodes and replace the matches with my code, as far as i know this is not a security risk, and the function is quite efficient.
Using:
function escapeHTML(str) str.replace(/[&"<>]/g, function (m) ({ "&": "&", '"': """, "<": "<", ">": ">" })[m]);
You’re right it’s slightly faster, in my version dom creation was slower but it’s coded a lot different, I’ll play some more with your code to see if i can squeeze some more milliseconds and I’ll update the code, thank you.
While it may be more work to write the code, DOM creation is greatly faster and more efficient. It does not interfere with other JS on page either (if applicable).
For example, in case of simple text insertion:
elem.appendChild(document.createTextNode('test')); //x6 faster than innerHTML
elem.textContent = 'test'; //x5 faster than innerHTML
elem.innerHTML = 'test';
It fails to highlight all matches… sometimes it skips some of them and it needs to run several times until everything is highlighted… the innerHTML method worked in a more reliable way.
Ok, I’ve found the bug, in my previous version (the one using innerHTML) the lastindex was resetting itself when doing the data.replace(searchText, replacement) but with the new version it needs to be manually reset using searchText.lastIndex = 0 or just removing the global flag on the regex.
P.S. Funny thing according to the ECMAScript 3 specification this is the normal behavior but when testing on other browsers some of them automatically resets the lastindex.
Right now I’m testing with long text and code with lots of matches using regexps and it takes 1ms to 30ms to finish all highlights so I’ll focus on add more features first and try to further improve performance once all the features are implemented, but i wanted some user feedback first.
i.e I’ve two concepts to highlight selected text but i wanted to make a little poll to know which one the users prefer, and the interface is ugly as hell and it needs to be expanded for that functionality so maybe I’ll rewrite the panel from scratch once i decided a better element distribution and color scheme.
The only problem so far are the long review queues i wanted to use the user feedback to add features and improve the existing ones but i fear the review queues will make the whole process too slow.
We ask reviewers to be ask thorough as possible, but there are many cases where it is impractical to continue a review after certain number/kinds of issues have been found, and other cases where things are overlooked in one review and brought up in a subsequent review.