1309350 - Don't abuse readURL to detect the existence of a script

Status: RESOLVED FIXED

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

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1309350: Part 1 - Remove dead code and clean up cruft.

I had to untangle a lot of this mess just to understand what was going on well
enough to make changes.

Review commit: https://reviewboard.mozilla.org/r/85126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85126/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

This results in about a 28% speed-up for Jetpack mochitest runs, for me.

Review commit: https://reviewboard.mozilla.org/r/85128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85128/

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

https://reviewboard.mozilla.org/r/85126/#review83814

Looks good, thanks for all these cleanups.

We should have introduced startsWith sooner!!

::: addon-sdk/source/lib/toolkit/loader.js:491
(Diff revision 1)
>      }
> -    try {
> -      let tmpPath = path + '/index.js';
> -      readURI(tmpPath);
> -      return tmpPath;
> -    } catch (e) {}
> +  } catch (e) {}

What was that? Just duplicated for no reason?!

::: addon-sdk/source/lib/toolkit/loader.js:729
(Diff revision 1)
>          // If the override is for x -> y,
>          // then using require("x/lib/z") to get reqire("y/lib/z")
>          // should also work
> -        if (id == key || (id.substr(0, key.length + 1) == (key + "/"))) {
> +        if (id == key || id.startsWith(key + "/")) {
>            id = overrides[key] + id.substr(key.length);
> -          id = id.replace(/^[\.\/]+/, "./");
> +          id = id.replace(/^([\.\/]+)?/, "");

I don't quite follow why you need the additional "(  )?", but may be my regexp is rusty...
It seems useless given that we are using `replace`.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review83822

This shouldn't work for unpackaged addons, like addons loaded from sources via about:debugging?
Also it is quite complex (the ziprequest, the caches, the observer notifications...). Did you had any alternative ideas?
For ex, Would this be enough?
http://searchfox.org/mozilla-central/source/services/sync/tps/extensions/mozmill/resource/stdlib/securable-module.js#318
It still uses a channel but not reading its full content...

Also, did you tried to run a test pilot addon with all these patches applied?
Given the complexity of the loader I would be more confident knowing a non-naive addon still works fine!

Comment 5

[Kris Maglione [:kmag]]

It does work with packaged add-ons, yes. I didn't test with Test Pilot, but I did test with GeckoProfiler. Unfortunately, though, as far as I can tell, we don't actually have automated tests for this :(

Unfortunately, the complexity is necessary, due to the huge number of files and paths the node resolver looks for. My initial version (which was a bit simpler, but not as simple as just using a channel) gave about a 15% speed-up on the Jetpack tests. This version gives about a 28% speed-up. On the entire test run, not just the module loading...

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

https://reviewboard.mozilla.org/r/85126/#review83814

> What was that? Just duplicated for no reason?!

Yeah... I really have no idea why, but it definitely wasn't necessary

> I don't quite follow why you need the additional "(  )?", but may be my regexp is rusty...
> It seems useless given that we are using `replace`.

You're right, it's not necessary.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85126/diff/1-2/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85128/diff/1-2/

Comment 9

[Kris Maglione [:kmag]]

Updated to run resolution tests with both packed and unpacked add-ons, and with all relevant URL types.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85128/diff/2-3/

Comment 11

[Kris Maglione [:kmag]]

Also removed the node resolver cache, since it doesn't make a difference anymore.

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

https://reviewboard.mozilla.org/r/85124/#review84200

::: addon-sdk/source/lib/toolkit/loader.js:335
(Diff revision 3)
> +function checkUrlExists(url) {
> +  if (!/\.(?:js|json)$/.test(url))
> +    url = addTrailingSlash(url);
> +
> +  let baseURL = getBaseURL(url);
> +

I don't know what was your simplier version, but how different the performances would be if you do something like this here:
  var channel = NetUtil.newChannel({
    uri: Services.io.newURI(url, null, null),
    loadUsingSystemPrincipal: true
  });
  try {
    channel.open2().close();
    return true;
  } catch (e) {
    return false;
  }
And cache this result in a DefaultMap?
The complexity that disturbs me here is that we have some code specific to each scheme.

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84118

::: addon-sdk/source/lib/toolkit/loader.js:371
(Diff revisions 1 - 3)
>  function join(...paths) {
>    let joined = pathJoin(...paths);
>    let resolved = normalize(joined);
>  
> +  // Make sure that resource: and jar: URLs have the necessary trailing slash.
> +  resolved = resolved.replace(/^(?:jar:[^!]+!|resource:\/[^\/]+)$/, "$&/");

This replace isn't clear. It is hard to guess what it tries to address.

Is that because normalize also strip trailing slash?
Why is that specific to only jar and resource?
Why does is replace resource:/foo to resource:/foo/
as well as resource:/foo.html to resource:/foo.html/?
Couldn't it be done using startsWith("resource") && !endsWith("/")? Or call the existing addTrailingSlashes?

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84228

::: addon-sdk/source/test/test-native-loader.js:1
(Diff revision 3)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Oh, and I tried to run this test locally without much success. That may be helpful to put a comment on how to do that in the test itself.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84118

> This replace isn't clear. It is hard to guess what it tries to address.
> 
> Is that because normalize also strip trailing slash?
> Why is that specific to only jar and resource?
> Why does is replace resource:/foo to resource:/foo/
> as well as resource:/foo.html to resource:/foo.html/?
> Couldn't it be done using startsWith("resource") && !endsWith("/")? Or call the existing addTrailingSlashes?

Yes, normalize strips the trailing slash, and sometimes produces invalid URLs like "resource://foo" and "jar:file:///foo/!", which are illegal. We don't need the URLs to end with `/`, but we do need them to at least always have a path component.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84228

> Oh, and I tried to run this test locally without much success. That may be helpful to put a comment on how to do that in the test itself.

I wound up doing it by commenting out all of the other tests in `jetpack-package.ini` and running `mach mochitest --flavor=mochitest-jetpack-package --disable-e10s addon-sdk/source/test/` but that's the same for any test in this suite.

Comment 18

[Kris Maglione [:kmag]]

Mossop, since you apparently tried to tackle this before I did, do you have any opinions on which approach we should prefer?

Comment 19

[Kris Maglione [:kmag]]

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> Comment on attachment 8800095 [details]
> Bug 1309350: Part 1 - Remove dead code and clean up cruft.
> 
> https://reviewboard.mozilla.org/r/85124/#review84200
> 
> ::: addon-sdk/source/lib/toolkit/loader.js:335
> (Diff revision 3)
> > +function checkUrlExists(url) {
> > +  if (!/\.(?:js|json)$/.test(url))
> > +    url = addTrailingSlash(url);
> > +
> > +  let baseURL = getBaseURL(url);
> > +
> 
> I don't know what was your simplier version, but how different the
> performances would be if you do something like this here:
>   var channel = NetUtil.newChannel({
>     uri: Services.io.newURI(url, null, null),
>     loadUsingSystemPrincipal: true
>   });
>   try {
>     channel.open2().close();
>     return true;
>   } catch (e) {
>     return false;
>   }
> And cache this result in a DefaultMap?
> The complexity that disturbs me here is that we have some code specific to
> each scheme.

Manually copying my comment from mozreview, since for some reason it didn't wind up in the bug:

    OK, I tried this, and with caching, the test run is only slightly (~2%) slower than with the other version. I'm still leaning toward the other approach for a few reasons, though:

    1) That's still 2% of the entire test suite run, not just of the module loading overhead.

    2) The test suite benefits much more from this kind of caching than an extension would at cold startup. That's especially true for the common case of extensions stored as packed XPIs, where every URL existence check is reduced to a simple Set lookup.

    3) This approach doesn't work for testing the existence of directories, which means that we can't skip the lookups in node_modules directories that we know don't exist, and have to do an uncached check for each module in each parent directory of the current module whenever it's required.

    4) The extra overhead is going to hit most users far harder than it hits my test system, with its gigabytes of RAM, memory-cached filesystem contents, fast SSD, and overpowered CPU.

Comment 20

[Dave Townsend [:mossop]]

When I tackled this in bug 1036625 I resolved urls down to an nsIFileURL or nsIJARURI and used the file and zip APIs to check for file existence. My suspicion is that that is the fastest since attempting to open a channel to a url is going to have to do all that as a minimum as well as seeking and setting up a reading stream to the source (unless we lazily do that)

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85128/diff/3-4/

Comment 22

[Kris Maglione [:kmag]]

This version simplifies things a bit, but still uses the direct URL resolution for now. I'm open to switching to the channel-based approach if necessary, but I still think the slightly increased complexity of this approach is worth it for the additional efficiency.

Comment 23

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf07a31a44fcbcba9d15114e87e6a2ae86fd9a5
Bug 1309350: Part 1 - Remove dead code and clean up cruft. r=ochameau

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbf07a31a44f

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review85008

Ok, let's go with the fastest option. I just have a bunch of nits.

Thanks for maintaining this orphan code!

::: addon-sdk/source/lib/toolkit/loader.js:241
(Diff revision 4)
> +   *
> +   * @returns {Set<string>}
> +   */
> +  getZipFileContents(uri, baseURL) {
> +    let file = uri.JARFile.QueryInterface(Ci.nsIFileURL).file;
> +    let basePath = addTrailingSlash(uri.JAREntry).slice(1);

nit: a comment saying that we remove the trailing slash would help. Because calling addTrailingSlash to do that is hard to follow.

::: addon-sdk/source/lib/toolkit/loader.js:243
(Diff revision 4)
> +   */
> +  getZipFileContents(uri, baseURL) {
> +    let file = uri.JARFile.QueryInterface(Ci.nsIFileURL).file;
> +    let basePath = addTrailingSlash(uri.JAREntry).slice(1);
> +
> +    let enumerator = zipCache.getZip(file).findEntries("(*.js|*.json|*/)");

Shouldn't we rather do the opposite?
findEntries(basePath) and then iterate to filter the .js, .json and directories?
We might even be able to come up with a search string for findEntries that doesn't need any filter?
basePath + "/*.json|" + basePath + "/*.js| "+basePath + "/*/"
?

::: addon-sdk/source/lib/toolkit/loader.js:261
(Diff revision 4)
> +
> +  zipContentsCache: new DefaultMap(baseURL => {
> +    let uri = NetUtil.newURI(baseURL);
> +
> +    if (baseURL.startsWith("resource:"))
> +      uri = NetUtil.newURI(resProto.resolveURI(uri));

nit: here and elsewhere, sdk codebase uses braces even for single line if blocks.

::: addon-sdk/source/lib/toolkit/loader.js:279
(Diff revision 4)
> +    } catch (e) {
> +      return false;
> +    }
> +  }),
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference]),

nit: Add an empty line between QueryInterface and observe

::: addon-sdk/source/lib/toolkit/loader.js:316
(Diff revision 4)
> +   * @returns {boolean}
> +   */
> +  exists(url) {
> +    if (!/\.(?:js|json)$/.test(url))
> +      url = addTrailingSlash(url);
> +

nit: You may move create of nsIURI in exists() (instead of zipContentsCache/filesCaches to create only one such object. Because here you are instanciating it twice in each.
I may also be correct to only call zipContentsCache only if you can query it to nsIJARURI from here.
Same for fileCaches, only call it if it queries to nsIFileURL?

::: addon-sdk/source/lib/toolkit/loader.js:355
(Diff revision 4)
> -function join(...paths) {
> -  let joined = pathJoin(...paths);
> -  let resolved = normalize(joined);
> -
> -  // OS.File `normalize` strips out any additional slashes breaking URIs like
> -  // `resource://`, `resource:///`, `chrome://` or `file:///`, so we work
> +function join(base, ...paths) {
> +  // If this is an absolute URL, we need to normalize only the path portion,
> +  // or we wind up stripping too many slashes and producing invalid URLs.
> +  let match = /^((?:resource|file|chrome)\:\/\/[^\/]*|jar:[^!]+!)(.*)/.exec(base);
> +  if (match)
> +    return match[1] + normalize(pathJoin(match[2], ...paths));

Thanks, it looks clearer.
(do not forget about the braces for all single line if blocks!)

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review85008

> Shouldn't we rather do the opposite?
> findEntries(basePath) and then iterate to filter the .js, .json and directories?
> We might even be able to come up with a search string for findEntries that doesn't need any filter?
> basePath + "/*.json|" + basePath + "/*.js| "+basePath + "/*/"
> ?

I've done that in the past[1], but dealing with escaping metacharacters makes the code a lot more complicated, so I decided to go with the simpler route here. Also, the normal case for this is a `resource:` URI that points to the root of an XPI, so we should probably get better filtering by extension than by base path.

[1]: http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/components/extensions/Extension.jsm#921-924

> nit: here and elsewhere, sdk codebase uses braces even for single line if blocks.

Hm. I'd normally use braces, but this module (and most of the SDK code I've seen recently) seemed to pretty consistently omit them. But I'm happy to add them in code this touches anyway.

> nit: You may move create of nsIURI in exists() (instead of zipContentsCache/filesCaches to create only one such object. Because here you are instanciating it twice in each.
> I may also be correct to only call zipContentsCache only if you can query it to nsIJARURI from here.
> Same for fileCaches, only call it if it queries to nsIFileURL?

I did that initially, but for the normal case of a `resource:` URL that points to a `jar:` URL, this version doesn't create a `nsIURI` at all for most calls. For `jar:` URLs, we only create one once the JAR is cached. For `file:` URLs that aren't cached, we create two, but that's the (hopefully least common) case where we have the most IO overhead anyway, so it's relatively less important.

And, as for only querying `zipContentsCache` even if we can't query to `nsIJARURI` here, the benefit is that we cache the failure for file-backed `resource:` URLs, and save a fair bit of XPConnect overhead in future checks.

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review85008

> I did that initially, but for the normal case of a `resource:` URL that points to a `jar:` URL, this version doesn't create a `nsIURI` at all for most calls. For `jar:` URLs, we only create one once the JAR is cached. For `file:` URLs that aren't cached, we create two, but that's the (hopefully least common) case where we have the most IO overhead anyway, so it's relatively less important.
> 
> And, as for only querying `zipContentsCache` even if we can't query to `nsIJARURI` here, the benefit is that we cache the failure for file-backed `resource:` URLs, and save a fair bit of XPConnect overhead in future checks.

(I'll add some comments about this)

Comment 28

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

Formatted:

(In reply to Kris Maglione [:kmag] from comment #27)
> Comment on attachment 8800096 [details]
> Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.
> 
> https://reviewboard.mozilla.org/r/85128/#review85008
> 
>> I did that initially, but for the normal case of a `resource:` URL that points to a `jar:` URL, this
>> version doesn't create a `nsIURI` at all for most calls. For `jar:` URLs, we only create one once the JAR 
>> is cached. For `file:` URLs that aren't cached, we create two, but that's the (hopefully least common) 
>> case where we have the most IO overhead anyway, so it's relatively less important.
>> 
>> And, as for only querying `zipContentsCache` even if we can't query to `nsIJARURI` here, the benefit is
>> that we cache the failure for file-backed `resource:` URLs, and save a fair bit of XPConnect overhead in 
>> future checks.
> 
> (I'll add some comments about this)

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review86322

There is already code in the build system to produce zip files. The code is intelligent and does things like not zlib compressing entries if they are larger than their input.

Unfortunately, I don't think we have an easy mechanism to hook up this code directly via moz.build, so you'll need a small proxy script.

::: addon-sdk/source/test/fixtures/create_xpi.py:7
(Diff revision 4)
> +import os.path
> +import zipfile
> +
> +
> +def main(output, input_dir):
> +    output.mode = 'rb'
> +
> +    with zipfile.ZipFile(output, 'w') as output_zip:
> +        base_dir = os.path.abspath(input_dir)
> +
> +        for (dirpath, dirnames, filenames) in os.walk(base_dir):
> +            for filename in filenames:
> +                path = os.path.join(dirpath, filename)
> +                relpath = os.path.relpath(path, base_dir).replace('\\', '/')
> +
> +                output_zip.write(path, relpath)

Replace this with:


# TODO replace this script with a direct Python action invocation
from mozbuild.action.zip import main as create_zip

def main(output, input_dir):
    return create_zip(['-C', input_dir, output, '**'])


This code will generate a well-formed and deterministic zip file.

::: addon-sdk/source/test/fixtures/moz.build:18
(Diff revision 4)
> +    file_ = GENERATED_FILES[xpi]
> +    file_.script = 'create_xpi.py'
> +    file_.inputs = [fixture]

Please don't use a trailing underscore. Just use `f` as the variable name.

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85128/diff/4-5/

Comment 31

[Mike Hommey [:glandium]]

> Unfortunately, I don't think we have an easy mechanism to hook up this code directly via moz.build, so you'll need a small proxy script.

That script already exists doesn't it? (mozbuild.action.zip)

Comment 32

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #31)
> > Unfortunately, I don't think we have an easy mechanism to hook up this code directly via moz.build, so you'll need a small proxy script.
> 
> That script already exists doesn't it? (mozbuild.action.zip)

Except scripts used as $(call py_action) have a different "calling convention" from GENERATED_FILES[x].script :/

Comment 33

[Gregory Szorc [:gps]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review86594

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9a143d2179a
Part 2 - Speed up synchronous resolution of module paths. r=gps,ochameau

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

Approval Request Comment
[Feature/regressing bug #]: Bug 935109

[User impact if declined]:

The current version of this code has severe performance issues, particularly at startup. With the rollout of e10s to users of add-ons, the problem becomes much worse, since it affects both main process and content process startup time.

[Describe test coverage new/current, TreeHerder]:

The module resolution functionality affected by these changes is covered by existing tests, but those tests only test under one (non-common) configuration. This change adds tests for the other, more common configurations.

[Risks and why]:

Moderate. This code has not been well maintained, and required significant refactoring to implement these changes. It's possible that some complex add-ons may rely on obscure quirks in its behavior that are not well-understood. QA is currently testing with popular and complex add-ons, which should reduce the likelihood that anything has been missed.

[String/UUID change made/needed]: None.

Comment 36

[Kris Maglione [:kmag]]

Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

Approval Request Comment: See comment 35

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9a143d2179a

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

I reviewed the patches attached to both of those bugs and I do *not* feel comfortable uplifting them to 50.0b10 especially given that QA is still ongoing. If rejecting these uplifts to Beta50 means we have fewer add-ons getting white listed in 50 then (sorry!) but that is a potential situation we need to prepare for.

The fact that this code has not stabilized on pre-beta channels sufficiently is a big concern. The risk mentioned in bug 1309350 is medium and the patches landed in Nightly yesterday which is clearly not enough bake time to minimize risk to quality when uplifting so late in the Beta cycle.

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

Fix a performance issue at startup. I'd like to take it in 51 aurora and see if any issues happen.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9544b06de180
https://hg.mozilla.org/releases/mozilla-aurora/rev/a16ab27e5330

Comment 41

[Kris Maglione [:kmag]]

Note: The first part of this needs a minor change for beta, since separate lexical scopes for for-of iterations landed in 51: https://hg.mozilla.org/try/rev/e7e1d1f4fb2284c9d8125beb7f0547d0643f8a4e

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/49a8e8f20df9
https://hg.mozilla.org/releases/mozilla-beta/rev/15fa0eb880c7

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

There was a long discussion on the benefits, risks and how to mitigate those risks. We have decided to take this in 50, Beta+