For .. in loops not protected from iterating prototype properties

[xoundboy]

I've been testing the library on Samsung and WebOS tvs and discovered that on one 2020 Samsung device execution breaks inside several of the for.. in loops conained in this library due to native prototype properties being iterated as regular array members. I don't know yet exactly why the problem only appears on this TV, however I was easily able to fix it using the hasOwnProperty check. I'm wondering whether I can submit a PR for this change to be merged to master? I have it ready to push. There are quite a few places the check is needed.

[palemieux]

Is it on all for...in loops, including those over arrays?

Would replacing for...in with for (var i = 0; i < a.length; i++) solve the issue?

[xoundboy]

Yes it seems to be all for...in loops, including arrays, although I didn't test every instance of for...in in the whole library.

replacing for...in with for (var i = 0; i < a.length; i++) should solve the issue. I'll check today and leave an update later. Thanks

[nigelmegitt]

We've noticed something similar and have a pull request on our BBC fork at bbc#9 to address this issue, which may help.

It's also worth noting that replacing the for ... in loops with indexed-based array iterators should provide a performance improvement too: I haven't got any profiling results that demonstrate this for our real world content, unfortunately.

[xoundboy]

I was thinking the very same thing about potentially improving performance by avoiding any hasOwnProperty checks which I believe slow things down, although I don't have any data to prove that either, just a hunch :-) Anyway, thanks for your comment. Good to know it's not just me that has this problem. Maybe it would make sense to fix this upstream and then rebase the changes into your fork @nigelmegitt

[nigelmegitt]

@xoundboy yes, for sure where we have improvements it's our intention to push them upstream (or fetch them from upstream if others have already done the same) to minimise the deltas between our fork and the main repo. We have some divergence that's required for us to deal with specifics of our environment.

[nigelmegitt]

potentially improving performance by avoiding any hasOwnProperty

Just for clarity, the performance improvements don't come from this, but from changing to indexed based array iterator for loops rather than for ... in.

I don't know a way to avoid using hasOwnProperty.

[palemieux]

replacing for...in with for (var i = 0; i < a.length; i++) should solve the issue. I'll check today and leave an update later. Thanks

Great. Looking forward to it.

[xoundboy]

@palemieux I verified that for (var i = 0; i < a.length; i++) solves the problem. Please go ahead and replace the for...in loops, or let me know if you would like me to prepare a PR. Thank you.

[palemieux]

let me know if you would like me to prepare a PR.

That would be great!

[xoundboy]

OK, I've got a branch ready called issues/0209-guard-against-native-prototype-property-iteration (trying to copy your branch naming scheme). Please grant me access to push to the repo. Thanks. @palemieux

[palemieux]

Please grant me access to push to the repo. Thanks.

Even better, any chance you might be able to create the branch on a fork of the repo in your github account?

This is the intended Github workflow AFAIK.

[xoundboy]

@palemieux This is the first time I'm making a PR in Github using this workflow. Thanks for the guidance. Here's the PR: #210 - it includes some whitespace changes which I didn't notice until too late. Let me know if you need me to revert those.

[palemieux]

This is the first time I'm making a PR in Github using this workflow.

Super! Thanks.

Let me know if you need me to revert those.

Not fatal... just distracting. How difficult is it to revert them?

[xoundboy]

It's a real pain to revert them. I'd really prefer not to if possible :)