Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/legacy-iterator-and-generator-support-will-be-removed/

Bug 880447's test case needs to transitioned to the new syntax.

telemetry table generated by the script in bug 1083470 comment #6 CONTENT | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 2003 | 1709 | 2719 | 11737 46 | 20 | 565 | 3345 | 9817 47 | 20 | 165 | 1047 | 16060 48 | 50 | 16 | 535 | 6193 49 | 31 | 34 | 471 | 9863 50 | 18 | 25 | 228 | 10234 51 | 608 | 83 | 563 | 5419 52 | 840 | 30 | 297 | 5600 53 | 1145 | 32 | 301 | 6236 54 | 38 | 46 | 322 | 660 55 | 44 | 1 | 1 | - 56 | 23 | - | - | - ADDONS | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 47517 | 40843 | 432765 | 1507757 46 | 30906 | 72904 | 373542 | 1400562 47 | 31962 | 94858 | 448458 | 3056093 48 | 62026 | 82998 | 678289 | 1889221 49 | 68560 | 213359 | 668605 | 2381255 50 | 82689 | 137253 | 510054 | 3750138 51 | 139355 | 153249 | 801567 | 2680756 52 | 188945 | 243029 | 434475 | 2411083 53 | 247355 | 208747 | 406009 | 3261211 54 | 189208 | 309908 | 719157 | 270072 55 | 397648 | 0 | 43044 | - 56 | 44866 | - | - | - (note: latest versions don't have enough data to compare)

It would be really nice if we could remove this post 57, because we just landed that warning.

CONTENT | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 2003 | 1709 | 2719 | 11737 46 | 20 | 565 | 3345 | 9818 47 | 20 | 165 | 1047 | 16070 48 | 50 | 16 | 535 | 6200 49 | 31 | 34 | 471 | 9894 50 | 18 | 25 | 228 | 10234 51 | 608 | 83 | 563 | 5424 52 | 840 | 30 | 297 | 5604 53 | 1145 | 32 | 302 | 6236 54 | 38 | 47 | 405 | 7928 55 | 44 | 1 | 629 | 3110 56 | 44 | 1 | 478 | - 57 | 45 | - | - | - ADDONS | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 47543 | 40851 | 433229 | 1507757 46 | 30906 | 72904 | 373567 | 1400562 47 | 31962 | 94858 | 448458 | 3059068 48 | 62026 | 82998 | 687115 | 1907313 49 | 68560 | 213359 | 668605 | 2381255 50 | 82689 | 137253 | 510160 | 3756327 51 | 139355 | 153249 | 801610 | 2743658 52 | 188959 | 243029 | 434767 | 2423855 53 | 247356 | 217548 | 406009 | 3295420 54 | 189405 | 309912 | 723884 | 3111299 55 | 398453 | 0 | 484314 | 1736829 56 | 348415 | 6375 | 254185 | - 57 | 54973 | - | - | -

(In reply to Tooru Fujisawa [:arai] from comment #5) > ADDONS > 57 | 54973 | - | - | - Where does it come from? Is this telemetry accurate?

(In reply to Masatoshi Kimura [:emk] from comment #6) > (In reply to Tooru Fujisawa [:arai] from comment #5) > > ADDONS > > 57 | 54973 | - | - | - > > Where does it come from? Is this telemetry accurate? the number is from telemetry. also, "-" means not-yet-available (doesn't have enough data to compare with others), instead of "0"

I mean, legacy generators should not be available from addons in 57. Why does it count so much?

(In reply to Masatoshi Kimura [:emk] from comment #8) > I mean, legacy generators should not be available from addons in 57. Why > does it count so much? can you explain why it's not available? (I just don't remember the situation properly) is it not available even if one tweak about:config? also, if there was some change, which bug is it and when have it landed?

(In reply to Tooru Fujisawa [:arai] from comment #9) > (In reply to Masatoshi Kimura [:emk] from comment #8) > > I mean, legacy generators should not be available from addons in 57. Why > > does it count so much? > > can you explain why it's not available? (I just don't remember the situation > properly) > is it not available even if one tweak about:config? It would be available only in privileged HTML if extensions.legacy.enabled is flipped. But it was disabled in most other places (including XUL). > also, if there was some change, which bug is it and when have it landed? Bug 1390106 made this change.

thanks. (In reply to Masatoshi Kimura [:emk] from comment #10) > > also, if there was some change, which bug is it and when have it landed? > > Bug 1390106 made this change. So, it have landed after 0.3 cycles for 57. Hitting 16% against previous version (nightly 56) sounds fair.

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-09-29&keys=__none__!__none__!__none__&max_channel_version=nightly%252F58&measure=JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_ADDONS&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-09-24&table=1&trim=1&use_submission_date=0 fwiw, it's 0% on 58 so far. and I think we can just remove it now.

Are you working on this, arai? If not I can take it next week.

I'm not. I was just checking telemetry numbers. feel free to take this :)

This removes most of the legacy generator code AFAIK. There's more to do in follow-up patches, in particular StarGenerator should be renamed Generator, we can merge StarGeneratorObject and GeneratorObject, etc.

Note to self: there's also more to clean up around CloseIterator, maybe StopIteration, etc.

This fixes 171 JS shell tests that used legacy generators. I tried to fix tests to use star generators where possible. Some ancient (fuzz) tests I just removed because they don't seem very valuable to keep. I also removed some tests for things that have better tests these days (try-finally in generators for instance). Linux64 is green on Try with these 2 patches; a bit surprising we don't have any remaining legacy generator uses in browser code/tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3522a3b93c924544899ecfe4f1b1a745ac4a3a48&group_state=expanded

Comment on attachment 8923451 [details] [diff] [review] Part 1 - Remove most legacy generator code Review of attachment 8923451 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/GeneratorObject.cpp @@ -263,5 @@ > > -#define JSPROP_ROPERM (JSPROP_READONLY | JSPROP_PERMANENT) > - > -static const JSFunctionSpec legacy_generator_methods[] = { > - JS_SELF_HOSTED_SYM_FN(iterator, "LegacyGeneratorIteratorShim", 0, 0), Removing the only caller to "LegacyGeneratorIteratorShim" also allows us to remove all this code in Iterator.js: https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/js/src/builtin/Iterator.js#9-73 :-) And with that code gone, we can also remove the WeakMap exports in SelfHosting.cpp (std_WeakMap_get, std_WeakMap_set, and WeakMap itself plus its alias std_WeakMap in Utilities.js).

Comment on attachment 8923451 [details] [diff] [review] Part 1 - Remove most legacy generator code Review of attachment 8923451 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Looking forward to further clean up :) ::: js/src/builtin/Generator.js @@ -74,5 @@ > - if (!IsObject(this) || !IsLegacyGeneratorObject(this)) > - return callFunction(CallLegacyGeneratorMethodIfWrapped, this, val, "LegacyGeneratorNext"); > - > - if (LegacyGeneratorObjectIsClosed(this)) > - ThrowStopIteration(); so, ThrowStopIteration intrinsic can be removed in followup patch.

Comment on attachment 8923454 [details] [diff] [review] Part 2 - Fix shell tests Review of attachment 8923454 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/jit-test/tests/collections/WeakMap-constructor-generator-3.js @@ +4,5 @@ > if (0) yield 0; > } > new WeakMap(none()); > > function* none2() { this part can be removed. ::: js/src/tests/js1_7/extensions/regress-350312.js @@ +20,5 @@ > printBugNumber(BUGNUMBER); > printStatus (summary); > > var iter; > + function* gen() I think, it's better cleaning up generator related tests in js1_* and move them to ecma_6 or just remove if duplicates. maybe in other bug. ::: js/src/tests/js1_8_5/extensions/is-generator.js @@ +13,5 @@ > reportCompare(false, Function.prototype.toString.isGenerator(), "native is not a generator fn"); > reportCompare(false, (function(){with(obj){}}).isGenerator(), "heavyweight is not a generator fn"); > reportCompare(false, (function(){obj}).isGenerator(), "upvar function is not a generator fn"); > > +reportCompare(true, (function*(){yield}).isGenerator(), "simple generator fn"); oops, I misremembered isGenerator returns false... :P maybe we should reopen bug 1119777 to track it separately.

browser_webconsole_bug_632347_iterators_generators.js is testing a legacy generator. I just converted it a star generator. Note that gen2 in that test is already a star generator, but it seems fine to keep both. I didn't notice this before because this is a non-e10s test so it only fails on Win7 debug...

Comment on attachment 8924145 [details] [diff] [review] Part 3 - Convert console test Review of attachment 8924145 [details] [diff] [review]: ----------------------------------------------------------------- thanks!

This removes the LegacyGeneratorIteratorShim in Iterator.js (good catch!). It also removes the following intrinsics: * GetIteratorPrototype * ThrowStopIteration * std_WeakMap_get/std_WeakMap_set (and made these static). * std_WeakMap and std_StopIteration in Utilities.js

Comment on attachment 8924149 [details] [diff] [review] Part 4 - Remove more self-hosting code Review of attachment 8924149 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/builtin/Utilities.js @@ -53,5 @@ > // > // Symbol is a bare constructor without properties or methods. > var std_Symbol = Symbol; > -// WeakMap is a bare constructor without properties or methods. > -var std_WeakMap = WeakMap; Whoa, I've just remembered there's even more code we can now remove: With |WeakMap| gone from the self-hosted global, we can also remove [1] and InitBareWeakMapCtor's declaration [2] and its definition [3]. And for bonus points InitWeakMapClass [4] can also be simplified, because it's now always called with |defineMembers| = true. [1] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/js/src/vm/GlobalObject.cpp#524 [2] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/js/src/builtin/WeakMapObject.h#44 [3] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/js/src/builtin/WeakMapObject.cpp#336 [4] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/js/src/builtin/WeakMapObject.cpp#299 @@ -55,5 @@ > var std_Symbol = Symbol; > -// WeakMap is a bare constructor without properties or methods. > -var std_WeakMap = WeakMap; > -// StopIteration is a bare constructor without properties or methods. > -var std_StopIteration = StopIteration; Similar to the WeakMap case from above, it should also be possible to remove [1]. [1] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/js/src/vm/GlobalObject.cpp#525 ::: js/src/builtin/WeakMapObject.cpp @@ +73,5 @@ > args.rval().setUndefined(); > return true; > } > > +static bool Thanks for making these static! :-) ::: js/src/builtin/WeakMapObject.h @@ +31,5 @@ > public: > static const Class class_; > }; > > // WeakMap methods exposed so they can be installed in the self-hosting global. Nit: This comment can now be removed, too.

CloseIterator can now be infallible. We can then also remove UnwindIteratorForException and call CloseIterator directly. Simplifying the exception unwinding code is pretty nice :)

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/298b5372db24 part 1 - Remove SpiderMonkey support for legacy generators. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/34b3cd5e9426 part 2 - Fix/remove shell tests that used legacy generators. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/cf18ce60ba49 part 3 - Fix browser_webconsole_bug_632347_iterators_generators.js to not use legacy generators. r=arai

(In reply to Tooru Fujisawa [:arai] from comment #22) > > function* none2() { > > this part can be removed. Oops yes, done. > I think, it's better cleaning up generator related tests in js1_* and move > them to ecma_6 or just remove if duplicates. > maybe in other bug. Yeah IMO we should just "unversion" all jsreftests - will file a bug. > oops, I misremembered isGenerator returns false... :P > maybe we should reopen bug 1119777 to track it separately. Done :)

Comment on attachment 8924167 [details] [diff] [review] Part 5 - Clean up CloseIterator Review of attachment 8924167 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup :D

Updated the patch. Good finds again :)

Comment on attachment 8924188 [details] [diff] [review] Part 4 - Remove more self-hosting code Review of attachment 8924188 [details] [diff] [review]: ----------------------------------------------------------------- Great!

https://hg.mozilla.org/mozilla-central/rev/298b5372db24 https://hg.mozilla.org/mozilla-central/rev/34b3cd5e9426 https://hg.mozilla.org/mozilla-central/rev/cf18ce60ba49

The Class can now be moved into GeneratorObject. This leaves some stale comments, I'll update them in a later patch.

Renames StarGenerator -> Generator in Generator.js and self-hosting intrinsics.

* This renames GeneratorKind::StarGenerator to Generator. Then I made GeneratorKind an enum class because js::Generator is very generic (and there's a function with that name). * Also renames JSScript/JSFunction isStarGenerator to isGenerator. * JSScript/LazyScript now store GeneratorKind in 1 bit instead of 2. * ParseContext.h is missing a license header and mode lines so I added it while I was there (it was confusing Emacs).

Comment on attachment 8924463 [details] [diff] [review] Part 8 - More cleanup Review of attachment 8924463 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/frontend/SharedContext.h @@ +514,5 @@ > isExprBody_ = true; > } > > void setGeneratorKind(GeneratorKind kind) { > + // The transition can only happen from NotGenerator. I think this function is no more used. ::: js/src/jsscript.h @@ +751,5 @@ > static const uint32_t INTRODUCTION_SCRIPT_SLOT = 3; > static const uint32_t RESERVED_SLOTS = 4; > }; > > +enum class GeneratorKind : bool { NotGenerator, Generator }; Nice :) @@ +1034,5 @@ > // optional arrays in |data|. See the comments above Create() for details. > uint8_t hasArrayBits:ARRAY_KIND_BITS; > > // The GeneratorKind of the script. > + uint8_t generatorKindBits_:1; we could remove "Bits" part, here and some other places, since now it's single bit flag.

Also renames some related functions.

JSOP_ITERNEXT now always sees a string value so Ion can use an infallible unbox. I also added an isString() assert to the interpreter.

Comment on attachment 8924459 [details] [diff] [review] Part 7 - Rename self-hosting code Review of attachment 8924459 [details] [diff] [review]: ----------------------------------------------------------------- Good to see StarGenerator gone and be replaced with just Generator. ::: js/src/jscntxt.h @@ -1191,5 @@ > } > }; > > /* Exposed intrinsics for the JITs. */ > -bool intrinsic_IsSuspendedStarGenerator(JSContext* cx, unsigned argc, Value* vp); Pre-existing, but why is this function declared in jscntxt.h instead of vm/SelfHosting.h? Maybe for a follow-up.

Comment on attachment 8924473 [details] [diff] [review] Part 10 - Rename ResumeKind::CLOSE to ResumeKind::RETURN Review of attachment 8924473 [details] [diff] [review]: ----------------------------------------------------------------- This comment [1] needs to be updated, too. Otherwise looks good. [1] https://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/js/src/frontend/BytecodeEmitter.cpp#9368

Comment on attachment 8924488 [details] [diff] [review] Part 11 - Use infallible unbox for JSOP_ITERNEXT Review of attachment 8924488 [details] [diff] [review]: ----------------------------------------------------------------- We either need to remove for-each completely (bug 1388317) or reject scripts with for-each in JSOP_ITER.

Comment on attachment 8924488 [details] [diff] [review] Part 11 - Use infallible unbox for JSOP_ITERNEXT Review of attachment 8924488 [details] [diff] [review]: ----------------------------------------------------------------- Sorry. I had forgotten that ITERNEXT is just used for for..in now: https://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/js/src/frontend/BytecodeEmitter.cpp#7535

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad4e37c81f6 part 4 - Remove more self-hosting code. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/efaccf22b253 part 5 - Clean up CloseIterator, remove UnwindIteratorForException. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad093cafc30 part 6 - Merge GeneratorObject and StarGeneratorObject. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/def80dfc0654 part 7 - Rename StarGenerator to Generator in self-hosted code. r=anba

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa956f09d3e part 8 - Clean up more generator code. r=arai https://hg.mozilla.org/integration/mozilla-inbound/rev/a4852b85f1c6 part 9 - More StarGenerator -> Generator renaming. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/3f850c136ee2 part 10 - Rename ResumeKind::CLOSE to ResumeKind::RETURN. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/a602f924a33c part 11 - Use infallible unbox for JSOP_ITERNEXT in Ion. r=evilpie

https://hg.mozilla.org/mozilla-central/rev/0ad4e37c81f6 https://hg.mozilla.org/mozilla-central/rev/efaccf22b253 https://hg.mozilla.org/mozilla-central/rev/0ad093cafc30 https://hg.mozilla.org/mozilla-central/rev/def80dfc0654 https://hg.mozilla.org/mozilla-central/rev/2aa956f09d3e https://hg.mozilla.org/mozilla-central/rev/a4852b85f1c6 https://hg.mozilla.org/mozilla-central/rev/3f850c136ee2 https://hg.mozilla.org/mozilla-central/rev/a602f924a33c

Developer release notes https://developer.mozilla.org/en-US/Firefox/Releases/58#JavaScript Reference pages https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Deprecated_and_obsolete_features/The_legacy_Iterator_protocol https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/Legacy_generator_function https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Legacy_generator_function Compat data https://github.com/mdn/browser-compat-data/pull/602

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/legacy-generator-support-has-been-removed/

Some of the legacy generator code now triggers a SyntaxError. Usually when JS features are deprecated, and us extension authors want to support both (super-)old versions and newer ones, we do some kind of an if (version check) { use new code; } else { use old code; } - and the old code doesn't throw any exceptions since it isn't run. But with JS syntax changes that can't work. What would you suggest be done when an extension wants to support both legacy generators and the new ones (for FF/TB versions which support just one and versions supporting just the other)?

For Firefox, you will have to write two versions (one for WebExtensions, the other for legacy) anyway. All Firefox versions that support WebExtensions also supports ES6 generators. For Thunderbird, script elements that have a type element with a version parameter (such as "application/javascript;version=1.8") will be ignored in new versions, so you can write a old code in versioned script elements.

(In reply to Eyal Rozenberg from comment #50) > What would you suggest be done when an extension wants to > support both legacy generators and the new ones (for FF/TB versions which > support just one and versions supporting just the other)? I think the new star generators have been in Firefox since version 26 or so. That's really ancient and unsupported, do you really need to support older versions?

(In reply to Jan de Mooij from comment #52) Ancient? That's from, like, 2 years ago... there are no ancient Firefox or Thunderbird versions, these apps are new.

(In reply to Eyal Rozenberg from comment #53) > Ancient? That's from, like, 2 years ago... there are no ancient Firefox or > Thunderbird versions, these apps are new. These versions have been unsupported for years, so they contain lots of (unpatched and publicly known) security vulnerabilities and don't come with sandboxing etc. Nobody should be using unsupported browser versions.