Issue 659973003: [DevTools] Disallow use of global property "document".

Issue 659973003: [DevTools] Disallow use of global property "document". (Closed)

Created:
6 years, 2 months ago by dgozman

Modified:
6 years, 1 month ago

Reviewers:
apavlov

CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

[DevTools] Disallow use of global properties "document", "addEventListener" and "removeEventListener". BUG=424188 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184164

Patch Set 1 #

Total comments: 20

Patch Set 2 : Fixed review comments #

Total comments: 10

Patch Set 3 : Fixed more comments #

Patch Set 4 : rebased #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+427 lines, -104 lines)			Patch
M	Source/devtools/scripts/jsdoc-validator/hashes	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/devtools/scripts/jsdoc-validator/jsdoc-validator.jar	View	1 2	Binary file		0 comments	Download
M	Source/devtools/scripts/jsdoc-validator/run_tests.py	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/ContextTrackingValidationCheck.java	View	1	4 chunks	+14 lines, -0 lines	0 comments	Download
A	Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java	View	1 2	1 chunk	+170 lines, -0 lines	0 comments	Download
M	Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionRecord.java	View	1	2 chunks	+20 lines, -1 line	0 comments	Download
M	Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/MethodAnnotationChecker.java	View	1 2	1 chunk	+13 lines, -15 lines	0 comments	Download
A	Source/devtools/scripts/jsdoc-validator/tests/document.js	View	1 2	1 chunk	+65 lines, -0 lines	0 comments	Download
M	Source/devtools/scripts/jsdoc-validator/tests/golden.dat	View	1 2	1 chunk	+142 lines, -86 lines	0 comments	Download

Messages

Total messages: 10 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

apavlov

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java File Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java (right): https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java#newcode15 Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:15: public final class DisallowedGlobalPropertyChecker extends ContextTrackingChecker { ...PropertiesChecker https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java#newcode17 ...

6 years, 2 months ago (2014-10-17 11:59:41 UTC) #3

dgozman

PTAL https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java File Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java (right): https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java#newcode15 Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:15: public final class DisallowedGlobalPropertyChecker extends ContextTrackingChecker { On ...

6 years, 2 months ago (2014-10-17 13:05:40 UTC) #4

PTAL

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java
(right):

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:15:
public final class DisallowedGlobalPropertyChecker extends
ContextTrackingChecker {
On 2014/10/17 11:59:41, apavlov wrote:
> ...PropertiesChecker

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:17:
private static final String GLOBAL_OBJECT_NAME = "window";
On 2014/10/17 11:59:41, apavlov wrote:
> we've also got "self"...

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:50:
if (node.getType() == Token.FUNCTION) {
On 2014/10/17 11:59:41, apavlov wrote:
> you could use a similar switch for consistency here

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:81:
String.format("\"%s\" global property is disallowed", name));
On 2014/10/17 11:59:41, apavlov wrote:
> global property access is disallowed

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:88:
if (nameNode != null) {
On 2014/10/17 11:59:41, apavlov wrote:
> early bailout:
> if (nameNode == null) {
>     return;
> }

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:106:
if (parent != null && parent.getType() == Token.GETPROP) {
On 2014/10/17 11:59:41, apavlov wrote:
> This conditional can be rewritten in a shorter form:
> 
> if (...) {
>     boolean isGlobalPropertyAccess = ...
>     if (!isGlobalPropertyAccess) {
>         return
>     }
> }
> addGlobalPropertyAccess(nameNode);

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:142:
topLevelGlobalPropertyAccessNodes.add(node);
On 2014/10/17 11:59:41, apavlov wrote:
> You could use a null object pattern here (create a fake FunctionRecord for the
> top level scope)

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertyChecker.java:153:
return node.getType() == Token.NAME &&
On 2014/10/17 11:59:41, apavlov wrote:
> && on the next line

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionRecord.java
(right):

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionRecord.java:18:
Set<String> parameterNames, TypeRecord parentType,
On 2014/10/17 11:59:41, apavlov wrote:
> This should be a list not a set (and this can be reused in another check)

Done.

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
File Source/devtools/scripts/jsdoc-validator/tests/golden.dat (right):

https://codereview.chromium.org/659973003/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/tests/golden.dat:49:
/usr/local/google/home/dgozman/chromium-blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/function.js:8:
ERROR - @return annotation found, yet function does not return value
On 2014/10/17 11:59:41, apavlov wrote:
> ha, this sucks. I have never paid attention to the absolute pathnames :( Can
you
> strip the prefixes up to e.g. "WebKit"? This should be done in the test python
> script.

Done.

apavlov

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java File Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java (right): https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java#newcode86 Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java:86: "access to \"%s\" property of global object is disallowed", ...

6 years, 2 months ago (2014-10-17 13:21:21 UTC) #5

dgozman

6 years, 2 months ago (2014-10-17 13:59:06 UTC) #6

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java
(right):

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java:86:
"access to \"%s\" property of global object is disallowed", name));
On 2014/10/17 13:21:20, apavlov wrote:
> Error messages are capitalized elsewhere, so "access" -> "Access"

Done.

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/DisallowedGlobalPropertiesChecker.java:98:
declaredLocalVariables.get(getCurrentFunction()).add(name);
On 2014/10/17 13:21:20, apavlov wrote:
> This will break for hoisted vars, like
> 
> function f() {
>   document.foo = 1;
>   var document;
> }
> 
> Please add a FIXME, otherwise I don't think we should fix this at the moment.

This works, since we check property access at the end of function.

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/MethodAnnotationChecker.java
(right):

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/MethodAnnotationChecker.java:47:
int actualParamCount = function.parameterNames.size();
On 2014/10/17 13:21:20, apavlov wrote:
> woot

Inlined.

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/...
File Source/devtools/scripts/jsdoc-validator/tests/golden.dat (right):

https://codereview.chromium.org/659973003/diff/20001/Source/devtools/scripts/...
Source/devtools/scripts/jsdoc-validator/tests/golden.dat:49:
/tests/document.js:57: ERROR - access to "addEventListener" property of global
object is disallowed
On 2014/10/17 13:21:20, apavlov wrote:
> This should be allowed, I guess? We have real-world uses of this (e.g. see
> HeapSnapshotWorker.js, where your check is inadvertently circumvented)

We should specifically whitelist some places via @suppressGlobalPropertiesCheck,
and disallow this everywhere. Same as document.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/659973003/60001

6 years, 1 month ago (2014-10-22 09:48:54 UTC) #9

commit-bot: I haz the power

6 years, 1 month ago (2014-10-22 10:53:48 UTC) #10

Message was sent while issue was closed.

Committed patchset #4 (id:60001) as 184164

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages