696172 - String prototype extension not detected by code validator

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

Description

[Jorge Villalobos [:jorgev] (he/him)]

I'm reviewing version 0.51 of the DiggiDig extension: https://addons.mozilla.org/en-US/firefox/addon/diggidig/, and found that it has String prototype extension in file options.js, line 219. This should have been picked up by the validator. These are the current results for that file:

https://addons.mozilla.org/en-US/developers/addon/diggidig/file/134270/validation

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

I just had a second look and found this warning:

Global variable overwrite
Warning: An attempt was made to overwrite a global variable in some JavaScript code.

It points to code following the line that should have been flagged. However, the warning says nothing of this being a case of prototype extension, and the code that really matters (String.prototype.diggiTrim = function () ...)doesn't appear anywhere.

Comment 2

[Matt Basta [:basta]]

I can't see the validation results on prod. Can you pastebin the output for me?

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Here's the JSON output: http://pastebin.com/vcb5eELq

Comment 4

[Matt Basta [:basta]]

I'm going to say that this is the intended behavior. String prototypes (or any prototypes, for that matter, are treated the same way. There's no way right now to "traverse back" through the global scope to see where the JSPrototype object is referenced from, so any modification to it is a global variable overwrite error.

As for the code not appearing in the snippet, that's because the assignment spans multiple lines. Since assignments are statements and not declarations, the "start" line (which we use to position the window for the snippet) is at the end of the assignment, rather than the beginning (where the String.prototype.diggiTrim bit is). There's really no workaround for that unless Spidermonkey changes the way it generates its parse tree, unfortunately.

Unless there's something particular about this specific case that you think would be appropriate, I can't do a whole lot to make the warning easier to interpret.

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

It'd be more effective then to change the validation to a regular expression like
(String|Number|Object).prototype.something =
And then you can distinguish it from the other generic errors.

Comment 6

[Matt Basta [:basta]]

The problem there is that it would yield a second error since that kind of assignment is considered an overwrite of a critical global object. It could be marked as non-critical, but then we wouldn't get warnings/errors for things like this:

    var x = String;
    x.prototype.something = foo;

or

    var x = String.prototype;
    x.something = foo;

The regex would also have to consider the alternative means of assignment:

    String.prototype["something"] = foo;

where "something" can be any expression.

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

Almost every single prototype extension case I've seen uses a pattern similar to comment #5. Since the current warnings are not being helpful, I think that adding the RE is an improvement. Extending it to cover String.prototype["something"] sounds good.

Comment 8

[Matt Basta [:basta]]

Done

https://github.com/mozilla/amo-validator/pull/103

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...