My addon has been rejected, need some info to fix it

Add-ons addons.mozilla.org
#1

So this is the cause:

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:
http://jsfiddle.net/8or0az7s/1/

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) ({ "&": "&amp;", '"': "&quot;", "<": "&lt;", ">": "&gt;" })[m]);

as suggested here: https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion
will make hard to highlight terms like " > < &

Right now it works as expected all the characters mentioned above can be highlighted and are not causing any issues.

So what can i do to resolve this without losing functionality and speed?

Thanks.

P.S. Rest of the code can be found here:

It’s done using jpm so just use jpm run and it should work.

1 Like
#2

What about the other methods suggested on that MDN page?

#3

I’ll probably loss some performance using DOM creation methods in this particular case.

P.S. I’ve simplified the example here:
http://jsfiddle.net/8or0az7s/3/

#4

Why do you think you’ll lose performance?
It’s probably faster.
As far as I know, innerHTML is slow.

Here it is creating nodes directly:
http://jsfiddle.net/8or0az7s/4/

#5

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.

#6

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';
1 Like
#7

in this particular case it’s only ~20% faster.

#8

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.

#9

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.

The working code will be available here if someone needs to use it: https://github.com/sdar/Multiple-Highlighter

This function is located inside data/highlight.js

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.

#10

Cool.
I think you could make it do all words and colors in one go though, if there is a need for that.

#12

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.

#13

Hi. My addon also been rejected with the same reason. The editors specified this reason alone. I fixed this issue and now I have a doubt.

  • Whether the AMO editors reviewed the full code and mentioned the issues or they mentioned when they got their first issue and stopped the review?

Thanks in Advance

#14

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.