Opened 6 years ago

Closed 2 years ago

Last modified 19 months ago

#54 closed defect (rejected)

Transpiling of for-of leads to problems if array is modified within the loop

Reported by: greiner Assignee:
Priority: P4 Milestone:
Module: Automation Keywords: jshydra
Cc: trev, sebastian Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by sebastian)

How to reproduce

  1. Write the following code
    let a = {b: [1, 2, 3]};
    for (let c of a.b)
    {
      console.log(c);
      delete a.b;
    }
    
  2. Transpile it using JSHydra

Observed behaviour

The generated code looks like:

var a = {b: [1, 2, 3]};
for (var i = 0; i < a.b.length; ++)
{
  console.log(a.b[i]);
  delete a.b;
}

Hence, running the code leads to:

TypeError: Cannot read property 'length' of undefined

Expected behaviour

The generated code should rather look like:

var a = {b: [1, 2, 3]};
for (var x = a.b, i = 0; i = 0; i < x.length; ++)
{
  console.log(x[i]);
  delete a.b;
}

Mimicking the behavior on Firefox, producing following output:

1
2
3

Change History (11)

comment:1 Changed 6 years ago by trev

  • Cc trev added

Our JSHydra translator was never meant to ensure 100% correct translation, merely covering our usage. Quite frankly, deleting things while iterating on them is not exactly good style, even if the browser takes care of that scenario. Do you really think this is worth fixing? Is that a real issue you encountered?

comment:2 Changed 6 years ago by fhd

This is probably what was fixed here:
http://codereview.adblockplus.org/6322148272504832/

I'm obviously leaning towards fixing it in JSHydra (and Thomas didn't). I'm slightly worried that this could cause pretty subtle bugs, and it doesn't really seem hard to fix.

comment:3 Changed 6 years ago by trev

  • Priority changed from Unknown to P4

Then one would also have to store the length of the array in a temp variable, to match the behavior of the for each loop in case the array itself is modified. In other words, the code in the description needs to be converted into:

var a = {b: [1, 2, 3]};
for (var array = a.b, i = 0, l = array.length; i < l; i++)
{
  var c = array[i];
  console.log(c);
  delete a.b;
}

Not sure whether this is worth introducing two additional variables but the behavior should really be the same then.

comment:4 Changed 6 years ago by greiner

Actually, for-each-in iterates over enumerable properties so shouldnt' it rather translate to a.b.forEach(...) than to for (var i = 0; i < a.b.length; i++). This would also take care of this issue.

MDN even says "Never use a loop like this on arrays. Only use it on objects."
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in

comment:5 Changed 6 years ago by trev

Performance of Array.forEach is bad.

We never use for each loops on anything other than arrays. We don't modify Array.prototype either. That's the only reason why we use for each loops at all. Switching to for..of loops might be desirable but it won't solve the necessity to translate loops - no browser other than Firefox currently supports those.

comment:6 Changed 6 years ago by trev

  • Ready unset

Note the #301 will replace for each..in loops by for..of loops. As a side-effect this will change the behavior for the scenario that elements are being removed while looping - the code in comment 3 does not replicate the behavior of a for..of loop in Firefox correctly. The l variable is no longer necessary, the loop should check against array.length each time.

comment:7 Changed 5 years ago by philll

  • Platform set to Firefox

comment:8 Changed 5 years ago by sebastian

  • Cc sebastian added
  • Description modified (diff)
  • Platform changed from Firefox to Unknown
  • Ready set
  • Summary changed from Transpiling of for-each leads to problems if array is modified within the loop to Transpiling of for-of leads to problems if array is modified within the loop

comment:9 Changed 5 years ago by greiner

  • Owner greiner deleted

comment:10 Changed 2 years ago by sebastian

  • Resolution set to rejected
  • Status changed from new to closed
  • Tester set to Unknown

Closing since we no longer use/maintain jsHydra.

comment:11 Changed 19 months ago by abpbot

A commit referencing this issue has landed:
Issue 54 - Add documentation for the $csp filter option

This commit relates to GitLab issue 54.

Last edited 19 months ago by kzar (previous) (diff)
Note: See TracTickets for help on using tickets.