1123122 - Stop using legacy generators in comm-central

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joshua Cranmer [:jcranmer]]

Unfortunately, I can't give a nice search for this. Legacy generators look like:
function () { yield 5; }
As opposed to ES6 generators:
function* () { yield 5; }

<https://dxr.mozilla.org/comm-central/search?q=yield+path%3A.js+-path%3Amozilla&case=true&redirect=true> matches both. The only sure-fire way to know which one is being used is to check the code. Although, if you're in test code, and you include asyncTestUtils, that's a good sign you're using legacy generators (add_task generally has better support, and that's in xpcshell core).

Thankfully, legacy generators appear to be rather little used outside of test code and gloda.

Comment 1

[:aceman]

So the tests have been converted in bug 1292569 by force :) Bug 1199296 should clear out all legacy generators as methods by rejecting them in core JS.

Can there be any more occurrences? Maybe functions in functions and anonymous functions in expressions. When fixing bug 1292569 I actually reviewed all occurrences of 'yield' and added the asterisk to its parent function whatever it was. So I hope /mailnews and /mail is clean. I think there are still unreviewed occurrences in /chat and /suite. I haven't grepped calendar.

Comment 2

[Jorg K (CEST = GMT+2)]

So we do a survey in /chat, /suite and /calendar, fix any problems there and then close this bug?
Or can we assume it's done since the system works after legacy generators were disabled by M-C in bug 1199296 and Aceman and Aleth fixed all (known) failures in bug 1292569?

Comment 3

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #2)
> Or can we assume it's done since the system works after legacy generators
> were disabled by M-C in bug 1199296 and Aceman and Aleth fixed all (known)
> failures in bug 1292569?

How do we know the "system" works? Have you looked at any Seamonkey tests or calendar runs?

We need to wait until the relevant peers confirm that.

From a quick glance, I am not sure about this occurrence:
https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/modules/shim/Loader.jsm#226
https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/components/calGoogleCalendar.js#401

For SM, e.g. these look unconverted:
https://dxr.mozilla.org/comm-central/source/suite/common/dataman/dataman.js#289
https://dxr.mozilla.org/comm-central/source/suite/common/tests/browser/browser_637020.js#47

Comment 4

[[:MakeMyDay]]

These are only occurences in the Google provider, not in calendar core code. For these occurrences, we should track this in a seperate bug if needed, as gdata requires to provide legacy support for postbox and friends, so maybe not all occurrences will go away.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #3)
> How do we know the "system" works?
You're right, without defining what "system" means, we don't know it's working.
And for bug 1287067 I switched back from icaljs to ical C++, so I can't tell for sure that "calendar JS" is working when using Daily 51a1.

> Have you looked at any Seamonkey tests or
> calendar runs?
Are there tests for SM? Calendar tests run as part of our test suite, right?
For example there is one that fails every 1st of the month ;-) (Bug 1244818)
 
> We need to wait until the relevant peers confirm that.
Indeed, that why I NI'ed them.

> From a quick glance, I am not sure about this occurrence:
> https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/modules/
> shim/Loader.jsm#226
> https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/
> components/calGoogleCalendar.js#401
Indeed, thanks for the references. They look problematic. I don't use the Google provider, but I recall there was a test for it that due to many failures got (partly) disabled (bug 1266823).

Comment 6

[:aceman]

(In reply to [:MakeMyDay] from comment #4)
> These are only occurences in the Google provider, not in calendar core code.
> For these occurrences, we should track this in a seperate bug if needed, as
> gdata requires to provide legacy support for postbox and friends, so maybe
> not all occurrences will go away.

They will need to go away when m-c disables all legacy generators. So maybe the gdata code still works now as bug 1199296 disabled them for "methods", which may not apply to e.g. the anonymous function passed to task.spawn in the example. But who knows what class of legacy generators they disable next.

Comment 7

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #5)
> > Have you looked at any Seamonkey tests or
> > calendar runs?
> Are there tests for SM? Calendar tests run as part of our test suite, right?
> For example there is one that fails every 1st of the month ;-) (Bug 1244818)

Seamonkey has their own build server and tests somewhere.

Comment 8

[[:MakeMyDay]]

(In reply to :aceman from comment #6)
> They will need to go away when m-c disables all legacy generators. So maybe
> the gdata code still works now as bug 1199296 disabled them for "methods",
> which may not apply to e.g. the anonymous function passed to task.spawn in
> the example. But who knows what class of legacy generators they disable next.

Especially the shim loader is to implement newer js functionality for older Gecko version (down to TB 8, iirc), so having it in there does not neccessarilly mean it has to go away. Maybe an addional try/catch wrapper is required, but I leave that for Fallen to decide which route to take for it. This is also why I suggested to track that in a seperate bug.

Comment 9

[:aceman]

Sure, there is no problem with a new bug.
Please file it and block this one so we know what areas are still unconverted.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

We might just bite the bullet and remove gecko 8 compat, at least to a level where we can use new language features. If Postbox wants to continue support then I might consider building a local version that uses babel to replace the es6 features.

Comment 11

[Magnus Melin [:mkmelin]]

It's up to the maintainers of course, but personally I never understood why add-ons would do anything but version-to-version match. If you use software version X you use the add-on for version X.

Comment 12

[Tooru Fujisawa [:arai]]

triggered try run that will log the place of "yield" of legacy generator.
  https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a79647927b4774a349d12e1d37c7266f0bbbf58f

it will be printed like following in build/test logs:
  LEGACY_GENERATOR: resource:///modules/iteratorUtils.jsm:77:8

Comment 13

[Tooru Fujisawa [:arai]]

so far the try run above hits the following places:
resource:///modules/iteratorUtils.jsm:77:8
resource://gdata-provider/modules/gdataSession.jsm:378:23
resource://gdata-provider/modules/gdataSession.jsm:381:16
resource://gdata-provider/modules/gdataSession.jsm:385:16
resource://gdata-provider/modules/gdataSession.jsm:390:38
resource://gdata-provider/modules/gdataSession.jsm:392:38
resource://gdata-provider/modules/gdataUtils.jsm:1003:20
resource://gdata-provider/modules/gdataUtils.jsm:1005:24
resource://gdata-provider/modules/gdataUtils.jsm:1057:20
resource://gdata-provider/modules/gdataUtils.jsm:1060:20
resource://gdata-provider/modules/gdataUtils.jsm:1077:28
resource://gdata-provider/modules/gdataUtils.jsm:1086:20
resource://gdata-provider/modules/gdataUtils.jsm:1091:16
resource://gdata-provider/modules/gdataUtils.jsm:1150:16
resource://gdata-provider/modules/gdataUtils.jsm:1152:16
resource://gdata-provider/modules/gdataUtils.jsm:1181:28
resource://gdata-provider/modules/gdataUtils.jsm:1183:20
resource://gdata-provider/modules/gdataUtils.jsm:1205:20
resource://gdata-provider/modules/gdataUtils.jsm:1301:38

iteratorUtils.jsm will be handled in bug 1340947.

Comment 14

[Tooru Fujisawa [:arai]]

all depending bugs are fixed.
feel free to reopen if there's any remaining usage.