1098617 - Remove __iterator__ usage from addon-sdk

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect)

Description

[Tom S]

It seems like a bunch of addon-sdk objects have a __iterator__ property. We want to remove support for that from the JS engine. In some cases it would make sense to use an iterator symbol instead. Please note that this has potential backwards compatibility issues, because you can't use for (.. in ..) to iterator over objects with an iterator symbol, like you can with __iterator__.

Uses: http://mxr.mozilla.org/mozilla-central/search?string=__iterator__&find=%2Faddon-sdk%2Fsource&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=mozilla-central

Comment 1

[Tom S]

Attachment: Remove useless __iterator__ usage

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Forwarding this issue on to the add-ons team.

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 8703036 [details] [review]
Remove useless __iterator__ usage

I'm not the right person to review this. Needinfo'd andym who manages add-on engineering now.

Comment 4

[Andy McKay]

Luca could you have a look please?

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1098617-Remove-one-useless-__iterator__-use.patch

Patch converted from github PR 2025 (https://github.com/mozilla/addon-sdk/pull/2025) with author metadata preserved.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1098617-addon-sdk-array-fromIterator-test-case.patch

Attaching a patch to add a new small test case to check its behaviour on valid and deprecated iterators.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=261c370fca06

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

I tried to the new test case in the attachment 8704295 [details] [diff] [review] on both the previous version of "fromIterator" and the patched version and they show the same results (the expected "TypeError: iterator is not iterable" exception is raised if we try to run it on an object that expose the deprecated __iterator__)

This result seems to confirm that the code removed by Tom is useless and removing it should be fine
(and the above try run on all the jetpack tests is green as expected) 

As a side note:
this bugzilla issue aims to remove any "for ... in" usage from the addon-sdk, so it is not completely closed by the above patches.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

I took a look to how much hard could be to completely remove the deprecated iterator
style from the Addon-SDK and reach the final goal of this bugzilla issue:
There are a couple of data structures which defines the legacy __iterator__ property [1]
(same MXR search linked in Comment 0, sometimes it is defined just as a fallback of the
Symbol.iterator one)
and there are places where "for (... in ...)" is still used [2] (some of them seems to be
in the tests and examples)

The Addon-SDK has a "deprecateUsage" helper in the "sdk/util/deprecate" module, and
it could be helpful to remove the __iterator__ usage from the Addon SDK itself
and at the same time start to properly warn add-on developers that they need to remove
__iterator__ usage in their add-on sources as well, before it will be not supported
anymore from the JS interpreter.

So, here is my proposed plan:

- use "deprecateUsage" in the __iterator__ properties defined by the Addon-SDK to
  log deprecation messages when the Addon-SDK code or an add-on is using the
  the legacy iterator

- remove "for (... in ...)" from the Addon-SDK code, guided from the failure generated by the
  deprecation logs in the Addon-SDK test cases

- leave the fallbacks and deprecation errors to "warn" addon developers about their own
  "for (... in ...)" usage, until the legacy iterator is finally removed from the the JS interpreter

- remove any legacy __iterator__ (with the deprecation warnings), once it is going to be removed
  completely from the JS interpreter

Here are two experimental patches and their related try pushes, where I experimented the "deprecateUsage" approach described above:
- deprecateUsage on "sdk/util/list" module
  - https://hg.mozilla.org/try/rev/eb009719669f
  - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecfe827a893c
- deprecateUSage on "sdk/util/collection" module (as child of the "deprecation on sdk/util/list" commit
  above)
  - https://hg.mozilla.org/try/rev/278d6784e473
  - https://treeherder.mozilla.org/#/jobs?repo=try&revision=5192f5aa72b3

Dave, does the above plan make sense to you?

When is the legacy iterator planned to be completely removed from the JS interpreter?

[1]: http://mxr.mozilla.org/mozilla-central/search?string=__iterator__&find=addon-sdk%2Fsource%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2]: http://mxr.mozilla.org/mozilla-central/search?string=for+\%28.*+in+.*\%29&regexp=on&find=addon-sdk%2Fsource%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 10

[Dave Townsend [:mossop]]

(In reply to Luca Greco [:rpl] from comment #9)
> Dave, does the above plan make sense to you?

Sounds good to me.

> When is the legacy iterator planned to be completely removed from the JS
> interpreter?

Tom, do you know this?

Comment 11

[Tom S]

We are going to remove iterators as soon as we can get every usage from Firefox. Or maybe one warning cycle after that.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562