Closed Bug 1293305 Opened 4 years ago Closed 4 years ago

Disable non-standard for-each on nightly-only

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- wontfix
firefox53 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(9 files, 7 obsolete files)

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
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.
Depends on: 1234048
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.
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.
and added the flag to all jit-test that uses for-each
for jstests, we can run enableForEach() in shell.js/browser.js
for-each is used only in js1_* directories
and fixed some other tests, by calling enableForEach(), or just skip the test if for-each is disabled and testing function is not available.
Keywords: addon-compat
maybe, we should wait for bug 1234048
Assignee: nobody → arai.unmht
bug 1234048 will be fixed shortly,
will rebase patches.
Attachment #8792273 - Attachment is obsolete: true
Attachment #8809002 - Flags: review?(evilpies)
Attachment #8792274 - Attachment is obsolete: true
Attachment #8809003 - Flags: review?(evilpies)
Attachment #8792275 - Attachment is obsolete: true
Attachment #8809004 - Flags: review?(evilpies)
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.
Thanks.
separated for each directory.
Attachment #8809005 - Attachment is obsolete: true
Attachment #8809786 - Flags: review?(evilpies)
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)
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)
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)
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+
Removing remaining for-each consumer.
Attachment #8812407 - Flags: review?(s.kaspari)
Attachment #8812407 - Flags: review?(s.kaspari) → review+
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.
(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
It seems this have broken HTTPSEverywhere ( https://github.com/EFForg/https-everywhere/issues/7676 ) and FlashGot
Cookie Controller broken.
See Also: → 1321584
Sadly, this broke SQLite Manager, too: https://github.com/lazierthanthou/sqlite-manager/issues/66
Both HTTPSEverywhere and FlashGot is now working fine
Depends on: 1322293
Depends on: 1322551
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.