Opened 7 months ago

Last modified 3 months ago

#7152 new defect

Loop affected by modifications to the list it iterates through

Reported by: greiner Assignee:
Priority: Unknown Milestone:
Module: Core Keywords:
Cc: mjethani, agiammarchi, jsonesen Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Environment

Chrome 70
Adblock Plus 3.3.2

How to reproduce

Iterate through filters using SpecialSubscription.filters() while calling filterStorage.removeFilter() for some of the filters within the loop.

The same issue may also apply to other generators.

Observed behaviour

Loop skips some of the filters.

Expected behaviour

Loop doesn't skip any of the filters.

Further information

This issue was discovered in this UI merge request relating to this Core change.

Change History (4)

comment:1 Changed 7 months ago by agiammarchi

Thanlks for putting me in Cc Thomas, FWIW I think if we need to consume generators all at once to avoid possible side effects while working with these we are missing any benefit of having generators, keeping only the CPU and RAM overhead.

Wouldn't be possible to queue any async operation so that a for/of could still modify all yeald values without missing any of them due Array modification behind the scene?

This is IMO not super urgent but we need to code aware that each for/of should be performed over Array.from(iterable) and not directly via the iterable.

Any improvement to this would be welcome.

comment:2 Changed 7 months ago by mjethani

We'll need to rewrite the filterStorage.removeFilter() function at some point, but meanwhile I think the semantics of Subscription.filters() should be similar to how it works for Set objects, i.e. items deleted do not cause any other items to be skipped.

In the meanwhile, if you would like to go ahead with the UI changes, I would recommend to use Subscription.filterCount and Subscription.filterAt(index) to iterate backwards by index as before. When this issue is fixed, you could then change it again.

comment:3 Changed 7 months ago by greiner

Thanks for the tip. I don't mind changing it again later so I've updated the merge request for the UI changes to use those two.

comment:4 Changed 3 months ago by mjethani

  • Cc jsonesen added
Note: See TracTickets for help on using tickets.