1293305 - Disable non-standard for-each on nightly-only

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

We could try disabling for-each on Nightly only, to see how much this change affects, and how much we get bug report for add-ons.
If we don't get notable amount of bug reports for some cycles, we could disable it also on aurora.

bug 1293205 will add warning, we should have at least 1 cycle that shows warning but still supports it.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Disable non-standard for-each on Nightly and add testing function to enable/disable it.

attaching patches, will ask review after next merge.

As we should need to keep for-each on other branches, we should test existing tests related to for-each.
So, for-each is disabled by ContextOptions.forEachStatement_.
That is copied to CompileOptions.forEachStatementOption and used in Parser.allowsForEachIn.

ContextOptions.forEachStatement_ can be switched by testing function.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1.1: Add need-for-each flag to jit-test harness.

as we need to enable for-each *before* parsing, I chose different approach for jit-test and jstests.

for jit-test, I added "need-for-each" flag, that will invoke enableForEach() before loading test.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1.2: Add need-for-each flag to jit-test that uses for-each.

and added the flag to all jit-test that uses for-each

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 1.3: Enable for-each in jstests for js1.x.

for jstests, we can run enableForEach() in shell.js/browser.js
for-each is used only in js1_* directories

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 1.4: Fix other tests to enable for-each, or skip if for-each is disabled.

and fixed some other tests, by calling enableForEach(), or just skip the test if for-each is disabled and testing function is not available.

Comment 6

[Tooru Fujisawa [:arai]]

maybe, we should wait for bug 1234048

Comment 7

[Tooru Fujisawa [:arai]]

bug 1234048 will be fixed shortly,
will rebase patches.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Disable non-standard for-each on Nightly and add testing function to enable/disable it.

rebased

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 1.1: Add need-for-each flag to jit-test harness.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 1.2: Add need-for-each flag to jit-test that uses for-each.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 1.3: Enable for-each in jstests for js1.x.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 1.4: Fix other tests to enable for-each, or skip if for-each is disabled.

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8809003 [details] [diff] [review]
Part 1.2: Add need-for-each flag to jit-test that uses for-each.

In the future when we remove for-each completely we probably have to go through all tests and see which should be update to not use for-each :/

Comment 14

[Tom S [:evilpie]]

Comment on attachment 8809004 [details] [diff] [review]
Part 1.3: Enable for-each in jstests for js1.x.

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

I hope this actually works in the browser.

Comment 15

[Tom S [:evilpie]]

Comment on attachment 8809005 [details] [diff] [review]
Part 1.4: Fix other tests to enable for-each, or skip if for-each is disabled.

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

You should ask somebody else to review the browser parts.

Comment 16

[Tom S [:evilpie]]

Thanks for doing this! Hopefully the add-on fallout is going to be manageable.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 1.4: Fix jit-test to enable for-each.

Thanks.
separated for each directory.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 1.5: Fix addon-sdk test for for-each to skip if for-each is disabled.

In addon-sdk, there's one test that uses for-each.

I've added testing function to enable for-each (Part 1), but addon-sdk test doesn't have access to it.
so, simply, it checks if for-each is enabled, and performs the test only if it's enabled.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 1.6: Fix dom test to enable for-each.

test_e4x_for_each.html tests for-each support for each JS version.
on nightly, it's disabled regardless of version, and needs to enable it before the test.
when for-each is enabled, version check works and it throws syntax error for unsupported versions.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 1.7: Fix xpconnect test to enable for-each.

Same as Part 1.6.
enabled for-each before performing legacy iterator tests.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8809790 [details] [diff] [review]
Part 1.6: Fix dom test to enable for-each.

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

rs=me

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8809791 [details] [diff] [review]
Part 1.7: Fix xpconnect test to enable for-each.

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

Could we just fix this test to not use for each instead?

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 1.7: Remove already disabled test that uses for-each.

it turns out that the test is disabled for long time and there are obsolete syntax.
removed the file.

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8809932 [details] [diff] [review]
Part 1.7: Remove already disabled test that uses for-each.

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

\o/

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Part 1.8: Do not use non-standard for-each.

Removing remaining for-each consumer.

Comment 26

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63ec09d6484c37e80958849ec2ebc891d375bce6
Bug 1293305 - Part 1: Disable non-standard for-each on Nightly and add testing function to enable/disable it. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d173e66fa2c9ddf5cec2bf20411e7e29404ce4
Bug 1293305 - Part 1.1: Add need-for-each flag to jit-test harness. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b118584e84177fe6697b33dfcaad130f5dcebde
Bug 1293305 - Part 1.2: Add need-for-each flag to jit-test that uses for-each. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/07514899546d702e402079e106452c85ccbf430d
Bug 1293305 - Part 1.3: Enable for-each in jstests for js1.x. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea378af1f0a4b60be6f230530c3ffd043d277d3
Bug 1293305 - Part 1.4: Fix jit-test to enable for-each. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6714622e751e711b11550c31b70a754d622b54
Bug 1293305 - Part 1.5: Fix addon-sdk test for for-each to skip if for-each is disabled. r=mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/933d53cdb6fe0c6b5bd141e4345735c12aca6bf9
Bug 1293305 - Part 1.6: Fix dom test to enable for-each. r=bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1054bac4d40a67c179b60c068bb0a58cb071b36
Bug 1293305 - Part 1.7: Remove already disabled test that uses for-each. r=bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/faf335ab953216b790042988defcf5c416e69cf3
Bug 1293305 - Part 1.8: Do not use non-standard for-each. r=sebastian

Comment 27

[Gary [:streetwolf52]]

I'm having having add-on problems presumably with this patch based on my regression range.  The add-on NoSquint Pus v49 no longer works.  I'll check for more.

Good
https://hg.mozilla.org/integration/mozilla-inbound/rev/10daeea4d322f4d396538ebac11b4bfdd5f5a798

Bad
https://hg.mozilla.org/integration/mozilla-inbound/rev/a614fa2f4d787cb75ca83d709bfa84ce71b4b7f0

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

Comment 28

[Gary [:streetwolf52]]

The add-on Text Link Ver.5.0.2016031501 besides not working also screws up right-click context menus.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63ec09d6484c
https://hg.mozilla.org/mozilla-central/rev/c8d173e66fa2
https://hg.mozilla.org/mozilla-central/rev/6b118584e841
https://hg.mozilla.org/mozilla-central/rev/07514899546d
https://hg.mozilla.org/mozilla-central/rev/0ea378af1f0a
https://hg.mozilla.org/mozilla-central/rev/ef6714622e75
https://hg.mozilla.org/mozilla-central/rev/933d53cdb6fe
https://hg.mozilla.org/mozilla-central/rev/f1054bac4d40
https://hg.mozilla.org/mozilla-central/rev/faf335ab9532

Comment 30

[Tooru Fujisawa [:arai]]

updated documents.
  https://developer.mozilla.org/en-US/Firefox/Releases/53
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in

Comment 31

[Tooru Fujisawa [:arai]]

(In reply to Gary [:streetwolf] from comment #28)
> The add-on Text Link Ver.5.0.2016031501 besides not working also screws up
> right-click context menus.

Text Link is fixed on trunk
https://github.com/piroor/textlink/commit/3a132cee51e34a2feb470be86e7a63ffe2470e0f

Comment 32

[Chris Peterson [:cpeterson]]

There are about 120,000 matches for "for each" in the AMO addons DXR:

https://dxr.mozilla.org/addons/search?q=%22for+each%22+%22+in+%22+ext%3Ajs&redirect=false

Comment 33

[V. Korn]

It seems this have broken HTTPSEverywhere ( https://github.com/EFForg/https-everywhere/issues/7676 ) and FlashGot

Comment 34

[Ian Nartowicz]

Cookie Controller broken.

Comment 35

[Lina Butler [:lina]]

Sadly, this broke SQLite Manager, too: https://github.com/lazierthanthou/sqlite-manager/issues/66

Comment 36

[V. Korn]

Both HTTPSEverywhere and FlashGot is now working fine

Comment 37

[Kohei Yoshino [:kohei]]

Updated the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2015/for-each-in-loop-support-will-be-removed/

Comment 38

[Gerry Chang [:gchang]]

We've built 51 RC. This is too late for 51. Mark 51 won't fix.

Comment 39

[Florian Scholz (Open Web Docs)]

Thanks for the docs, arai. Covered on:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/For-each-in_loops_are_deprecated