Perform hasOwnProperty validation

[ghost]

Resolves

#366

Proposed Changes

add hasOwnProperty check to for loops in vm

Reason for Changes

without it things like Object.prototype.harmless = false; break the app

That still breaks it because there is the same problem in the audio engine

Test Coverage

I didn't add it but there is this: http://eslint.org/docs/2.0.0/rules/guard-for-in

[thisandagain]

Thanks @magmaboat .

@rschamp @cwillisf any thoughts on adding the for in guard to our eslint config?

[rschamp]

I'm +1 on adding guard-for-in to our lint config.

[rschamp]

I do worry about performance with this though — it looks expensive to add a function call to every loop, especially for some of these which are called on each tick or more. Perhaps we should just keep an eye on it, and in the most expensive cases, cache and iterate with Object.keys(objToIterate) or something?

[griffpatch]

I had been thinking just the same thing. Is there a legitimate reason to worry about the object model being compremised in this way? I have been actively removing them where possible when there was no chance of it causing issue in the normal running of scratch 3 on my performance branch. On 10 Feb 2017 6:14 p.m., "Ray Schamp" <[email protected]> wrote: I do worry about performance with this though — it looks expensive to add a function call to every loop, especially for some of these which are called on each tick or more. Perhaps we should just keep an eye on it, and in the most expensive cases, cache and iterate with Object.keys(objToIterate) or something? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#446 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGbNvjy2TTSHfAfV3-Da0er4Pn7JNCiLks5rbKkIgaJpZM4L86zR> .

[tmickel]

Have been enjoying watching these discussions :)

I got ~90% slower with the guard: https://jsperf.com/for-in-hasownproperty-or-not - although I do think it's probably an extreme case, not sure if any loop in the execution path is executing enough times per tick to make it significant.

[cwillisf]

My favorite example to explain what hasOwnProperty does is to type this into the JS console, and then try to run some JS that isn't protected by hasOwnProperty checks:

Object.prototype.harmless = false

For example:

var stuff = {a: 1, b: 2}
for (var propertyName in stuff) { console.log(propertyName, '=', stuff[propertyName]) }

The output of the above:

a = 1
b = 2
harmless = false

Even with that demo, though, there are two valid questions being discussed here:

Is this really something to worry about?

Extending Object.prototype is hopefully exceedingly rare in professional code, since it has the potential to disrupt for-in loops like this. However, if answers on Stack Overflow are any guide, there seems to be a strong segment of the JavaScript programming audience that finds it convenient to extend native prototypes.

What I really worry about is libraries which extend, say, Array with convenience methods. These libraries might be used in extensions -- Scratch extensions or even browser extensions -- and disrupt functionality in fairly unpredictable ways. An even more reasonable possibility: a Scratch extension might choose to extend the prototype of an object it "owns" and passes into Scratch routines. It might not be clear to an extension author why our code doesn't handle that object as expected. Even if they understand what's going on, it seems kind of rude to tell them that their favorite style of programming JavaScript isn't valid when communicating with Scratch. There's also potential here for support headaches, with people reporting bugs that we can't reproduce in house.

More info about this can be found here: http://bonsaiden.github.io/JavaScript-Garden/#object.forinloop

Won't this hurt performance?

I agree with @rschamp when he says:

Perhaps we should just keep an eye on it, and in the most expensive cases [do something else]

Also, in my opinion, @tmickel's benchmark above actually indicates that this isn't very expensive.

That benchmark tests the cost of adding one line of code to a block of JavaScript:

var count = 0;
for (var test in x) {
    if (!x.hasOwnProperty(test)) continue; // <---- how expensive is this line?
    count++;
}

The result: for Tim it runs about 90% slower, and on my system it's about 80% slower. Let's go with the 90% number since it's more dire.

When jsPerf says one piece of code is "90% slower" than another, it really means that 1 - (number of times the slower code runs per second) / (number of times the faster code runs per second) is 0.90, approximately. Another way to put this is that the slower code takes about 10 times as long to run, compared to the faster code.

Let's algebra!

• Let p represent the amount of time spent to "prepare" the loop, including the var count = 0 line, creation of the test variable, etc.
• Let b represent the amount of time spent per iteration of the "body" of the loop, not counting the hasOwnProperty check -- mostly just incrementing the count variable.
• Let h represent the time spent on this line of code: if (!x.hasOwnProperty(test)) continue;
• Note that the loop executes 10 times per test.

p + 10 * (h + b) = 10 * (p + 10 * b)
p + 10h + 10b = 10p + 100b
10h = 9p + 90b
h = 0.9p + 9b

So, roughly speaking, the line if (!x.hasOwnProperty(test)) continue is approximately as expensive as this code:

var p;
for (var b = 0; b < 9; ++b) {}

While that's not the same as "free" it seems fairly cheap to me -- especially considering that most of these pieces of code don't run with high frequency.

[cwillisf]

👍
tested locally