Disable non-standard for-each on nightly-only

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: arai, Assigned: arai)

Tracking

({addon-compat, dev-doc-complete, site-compat})

Trunk
mozilla53
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox53 fixed)

Details

Attachments

(9 attachments, 7 obsolete attachments)

8.03 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.61 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
62.90 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.98 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
1.40 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
2.07 KB, patch
mossop
: review+
Details | Diff | Splinter Review
1.16 KB, patch
Bobby Holley (On Leave Until June 11th)
: review+
Details | Diff | Splinter Review
8.18 KB, patch
Bobby Holley (On Leave Until June 11th)
: review+
Details | Diff | Splinter Review
1.41 KB, patch
sebastian
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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.

Updated

2 years ago
Keywords: dev-doc-needed, site-compat

Updated

2 years ago
Depends on: 1234048
(Assignee)

Comment 1

2 years ago
Created attachment 8792272 [details] [diff] [review]
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8792273 [details] [diff] [review]
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8792274 [details] [diff] [review]
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
(Assignee)

Comment 4

2 years ago
Created attachment 8792275 [details] [diff] [review]
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
(Assignee)

Comment 5

2 years ago
Created attachment 8792276 [details] [diff] [review]
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.

Updated

2 years ago
Keywords: addon-compat
(Assignee)

Comment 6

2 years ago
maybe, we should wait for bug 1234048
Assignee: nobody → arai.unmht
(Assignee)

Comment 7

2 years ago
bug 1234048 will be fixed shortly,
will rebase patches.
(Assignee)

Comment 8

2 years ago
Created attachment 8809000 [details] [diff] [review]
Part 1: Disable non-standard for-each on Nightly and add testing function to enable/disable it.

rebased
Attachment #8792272 - Attachment is obsolete: true
Attachment #8809000 - Flags: review?(evilpies)
(Assignee)

Comment 9

2 years ago
Created attachment 8809002 [details] [diff] [review]
Part 1.1: Add need-for-each flag to jit-test harness.
Attachment #8792273 - Attachment is obsolete: true
Attachment #8809002 - Flags: review?(evilpies)
(Assignee)

Comment 10

2 years ago
Created attachment 8809003 [details] [diff] [review]
Part 1.2: Add need-for-each flag to jit-test that uses for-each.
Attachment #8792274 - Attachment is obsolete: true
Attachment #8809003 - Flags: review?(evilpies)
(Assignee)

Comment 11

2 years ago
Created attachment 8809004 [details] [diff] [review]
Part 1.3: Enable for-each in jstests for js1.x.
Attachment #8792275 - Attachment is obsolete: true
Attachment #8809004 - Flags: review?(evilpies)
(Assignee)

Comment 12

2 years ago
Created attachment 8809005 [details] [diff] [review]
Part 1.4: Fix other tests to enable for-each, or skip if for-each is disabled.
Attachment #8792276 - Attachment is obsolete: true
Attachment #8809005 - Flags: review?(evilpies)
Attachment #8809000 - Flags: review?(evilpies) → review+
Attachment #8809002 - Flags: review?(evilpies) → review+
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 :/
Attachment #8809003 - Flags: review?(evilpies) → review+
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.
Attachment #8809004 - Flags: review?(evilpies) → review+
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.
Attachment #8809005 - Flags: review?(evilpies) → feedback+
Thanks for doing this! Hopefully the add-on fallout is going to be manageable.
(Assignee)

Comment 17

2 years ago
Created attachment 8809786 [details] [diff] [review]
Part 1.4: Fix jit-test to enable for-each.

Thanks.
separated for each directory.
Attachment #8809005 - Attachment is obsolete: true
Attachment #8809786 - Flags: review?(evilpies)
(Assignee)

Comment 18

2 years ago
Created attachment 8809787 [details] [diff] [review]
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.
Attachment #8809787 - Flags: review?(dtownsend)
(Assignee)

Comment 19

2 years ago
Created attachment 8809790 [details] [diff] [review]
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.
Attachment #8809790 - Flags: review?(bobbyholley)
(Assignee)

Comment 20

2 years ago
Created attachment 8809791 [details] [diff] [review]
Part 1.7: Fix xpconnect test to enable for-each.

Same as Part 1.6.
enabled for-each before performing legacy iterator tests.
Attachment #8809791 - Flags: review?(bobbyholley)
Attachment #8809787 - Flags: review?(dtownsend) → review+
Attachment #8809786 - Flags: review?(evilpies) → review+
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
Attachment #8809790 - Flags: review?(bobbyholley) → review+
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?
Attachment #8809791 - Flags: review?(bobbyholley)
(Assignee)

Comment 23

2 years ago
Created attachment 8809932 [details] [diff] [review]
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.
Attachment #8809791 - Attachment is obsolete: true
Attachment #8809932 - Flags: review?(bobbyholley)
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/
Attachment #8809932 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 25

2 years ago
Created attachment 8812407 [details] [diff] [review]
Part 1.8: Do not use non-standard for-each.

Removing remaining for-each consumer.
Attachment #8812407 - Flags: review?(s.kaspari)
Attachment #8812407 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 26

2 years ago
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

2 years ago
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

2 years ago
The add-on Text Link Ver.5.0.2016031501 besides not working also screws up right-click context menus.
(Assignee)

Comment 31

2 years ago
(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
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
Blocks: 1153671

Comment 33

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

Comment 34

2 years ago
Cookie Controller broken.
(Assignee)

Updated

2 years ago
See Also: → bug 1321584

Comment 36

2 years ago
Both HTTPSEverywhere and FlashGot is now working fine
Depends on: 1322293
Depends on: 1322551

Updated

a year ago
Blocks: 1328861
We've built 51 RC. This is too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.