Closed Bug 1083482 Opened 10 years ago Closed 7 years ago

Remove SpiderMonkey support for JS1.7 legacy generators

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: cpeterson, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(11 files, 1 obsolete file)

87.42 KB, patch
arai
: review+
Details | Diff | Splinter Review
158.49 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.13 KB, patch
arai
: review+
Details | Diff | Splinter Review
11.93 KB, patch
arai
: review+
Details | Diff | Splinter Review
13.03 KB, patch
anba
: review+
Details | Diff | Splinter Review
9.43 KB, patch
arai
: review+
Details | Diff | Splinter Review
18.49 KB, patch
anba
: review+
Details | Diff | Splinter Review
80.41 KB, patch
arai
: review+
Details | Diff | Splinter Review
26.24 KB, patch
anba
: review+
Details | Diff | Splinter Review
12.72 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.54 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
      No description provided.
Depends on: 1083485
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Blocks: 1103158
Keywords: site-compat
Depends on: 1120051
Depends on: 1119777
Depends on: 1123118
No longer depends on: 1123118
Depends on: 1123122
Depends on: 1104014
No longer depends on: 1083485
Depends on: 1133277
Depends on: 1215846
Depends on: 1215965
Depends on: 1226398
Bug 880447's test case needs to transitioned to the new syntax.
Blocks: 1251882
Depends on: 1083476
No longer depends on: 1226398
Blocks: 1JS
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)
Depends on: 1392070
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.
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 :)
Blocks: 1412533
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
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.
Attachment #8923451 - Flags: review?(arai.unmht)
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
Attachment #8923454 - Flags: review?(arai.unmht)
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.
Attachment #8923451 - Flags: review?(arai.unmht) → review+
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.
Attachment #8923454 - Flags: review?(arai.unmht) → review+
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...
Attachment #8924145 - Flags: review?(arai.unmht)
Comment on attachment 8924145 [details] [diff] [review]
Part 3 - Convert console test

Review of attachment 8924145 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!
Attachment #8924145 - Flags: review?(arai.unmht) → review+
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
Attachment #8924149 - Flags: review?(andrebargull)
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.
Attachment #8924149 - Flags: review?(andrebargull) → review+
CloseIterator can now be infallible. We can then also remove UnwindIteratorForException and call CloseIterator directly.

Simplifying the exception unwinding code is pretty nice :)
Attachment #8924167 - Flags: review?(arai.unmht)
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
Keywords: leave-open
(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
Attachment #8924167 - Flags: review?(arai.unmht) → review+
Updated the patch. Good finds again :)
Attachment #8924149 - Attachment is obsolete: true
Attachment #8924188 - Flags: review?(andrebargull)
Comment on attachment 8924188 [details] [diff] [review]
Part 4 - Remove more self-hosting code

Review of attachment 8924188 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #8924188 - Flags: review?(andrebargull) → review+
The Class can now be moved into GeneratorObject.

This leaves some stale comments, I'll update them in a later patch.
Attachment #8924458 - Flags: review?(arai.unmht)
Renames StarGenerator -> Generator in Generator.js and self-hosting intrinsics.
Attachment #8924459 - Flags: review?(andrebargull)
* 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).
Attachment #8924463 - Flags: review?(arai.unmht)
Attachment #8924458 - Flags: review?(arai.unmht) → review+
Attachment #8924466 - Flags: review?(andrebargull)
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.
Attachment #8924463 - Flags: review?(arai.unmht) → review+
Also renames some related functions.
Attachment #8924473 - Flags: review?(andrebargull)
JSOP_ITERNEXT now always sees a string value so Ion can use an infallible unbox. I also added an isString() assert to the interpreter.
Attachment #8924488 - Flags: review?(evilpies)
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.
Attachment #8924459 - Flags: review?(andrebargull) → review+
Attachment #8924466 - Flags: review?(andrebargull) → review+
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
Attachment #8924473 - Flags: review?(andrebargull) → review+
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.
Attachment #8924488 - Flags: review?(evilpies) → review-
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
Attachment #8924488 - Flags: review- → review+
Blocks: 1413867
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
Keywords: leave-open
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.
You need to log in before you can comment on or make changes to this bug.