Disable non-standard for-each on nightly-only

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

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

Trunk
mozilla53
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
bholley
: review+
Details | Diff | Splinter Review
8.18 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.41 KB, patch
sebastian
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 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

3 years ago
Depends on: 1234048
(Assignee)

Comment 1

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

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

3 years ago
and added the flag to all jit-test that uses for-each
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

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

Comment 9

3 years ago
Attachment #8792273 - Attachment is obsolete: true
Attachment #8809002 - Flags: review?(evilpies)
(Assignee)

Comment 10

3 years ago
Attachment #8792274 - Attachment is obsolete: true
Attachment #8809003 - Flags: review?(evilpies)
(Assignee)

Comment 11

3 years ago
Attachment #8792275 - Attachment is obsolete: true
Attachment #8809004 - Flags: review?(evilpies)
(Assignee)

Comment 12

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

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

Comment 18

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

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

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

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

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

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: → 1321584

Comment 36

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

Updated

2 years ago
We've built 51 RC. This is too late for 51. Mark 51 won't fix.
You need to log in before you can comment on or make changes to this bug.