If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

String prototype extension not detected by code validator

RESOLVED FIXED in Q1 2012

Status

addons.mozilla.org Graveyard
Add-on Validation
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jorgev, Assigned: basta)

Tracking

unspecified
Q1 2012

Details

(Whiteboard: [ReviewTeam], URL)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
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.
Assignee: nobody → mattbasta
(Assignee)

Comment 2

6 years ago
I can't see the validation results on prod. Can you pastebin the output for me?
(Reporter)

Comment 3

6 years ago
Here's the JSON output: http://pastebin.com/vcb5eELq
Target Milestone: 6.3.0 → 6.3.2
Target Milestone: 6.3.2 → 6.3.3
(Assignee)

Comment 4

6 years ago
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.
(Reporter)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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.
(Reporter)

Comment 7

6 years ago
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.
Target Milestone: 6.3.3 → 6.3.4
Target Milestone: 6.3.4 → Q1 2012
(Assignee)

Comment 8

6 years ago
Done

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

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

6 years ago
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.