Hide "for each" from content

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: terrence, Assigned: emk)

Tracking

({dev-doc-complete})

Trunk
mozilla20
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p3], URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
This appears to be exposed to content in nightly.
This is broken at least as far back as Firefox 16.
Whiteboard: [js:p3]
(Assignee)

Comment 2

5 years ago
For each was visible from the content since Firefox 1.5! (not a typo of Firefox 15.)
In other words, it has been exposed to the content from the beginning.
(Assignee)

Comment 3

5 years ago
FYI for-of is also visible from the content without any opt-in. Is it intentional?
for-of is on the standards track.  for-each is about as far from that as it's possible to be.
(Assignee)

Comment 5

5 years ago
No, for-each was standardized by E4X. However, it was too much used to remove (see bug 791343).
It would be logical to enable for-each only JSVERSION_1_6 or later because E4X was introduced since JavaScript 1.6.
We used to expose other extensions if the Web page opted-in using ";version=". So there's no point in hiding only this particular feature completely.
(In reply to Masatoshi Kimura [:emk] from comment #5)
> It would be logical to enable for-each only JSVERSION_1_6 or later because
> E4X was introduced since JavaScript 1.6.

That seems likeliest to happen.  Eventually, once for-of is completely standardized, we'll then remove for-each.

> We used to expose other extensions if the Web page opted-in using
> ";version=". So there's no point in hiding only this particular feature
> completely.

Pretty much.  Although, I think the calculus is affected when the feature in question is dying, we don't want anyone else implementing it, and we don't want the web, either existing or new pages, depending on it.  That argues much more strongly for removing it everywhere in as short an order as is possible.
(Assignee)

Comment 7

5 years ago
Created attachment 692587 [details] [diff] [review]
Part 1: Fix tests depending on E4X for-each in content JS
Attachment #692587 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

5 years ago
Created attachment 692588 [details] [diff] [review]
Part 2: Disable for-each-in from content by default

- If javascript.options.xml.(content|chrome) is enabled, "for each" will be enabled because it is a part of E4X.
- Unlike the patch from bug 791343, this patch doesn't disable "for each" on JavaScript >=1.6.
Attachment #692588 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

5 years ago
Created attachment 692589 [details] [diff] [review]
Part 3: Add a regression test
Attachment #692589 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 10

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6eba30b030b5
(Assignee)

Comment 11

5 years ago
Created attachment 692693 [details] [diff] [review]
Part 2: Disable for-each-in from content by default

The previous patch didn't compile with JS_HAS_XML_SUPPORT=0.
Attachment #692588 - Attachment is obsolete: true
Attachment #692588 - Flags: review?(jwalden+bmo)
Attachment #692693 - Flags: review?(jwalden+bmo)
Comment on attachment 692587 [details] [diff] [review]
Part 1: Fix tests depending on E4X for-each in content JS

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

I wonder how many of these not using var are using undeclared variables.  Le sigh.
Attachment #692587 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 692693 [details] [diff] [review]
Part 2: Disable for-each-in from content by default

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

There's a little bit of complexity to the tweaks I'm asking for, so I'd like another look, I guess.

::: js/src/frontend/Parser.cpp
@@ +2989,5 @@
>      PushStatementPC(pc, &forStmt, STMT_FOR_LOOP);
>  
>      pn->setOp(JSOP_ITER);
>      pn->pn_iflags = 0;
> +#if JS_HAS_XML_SUPPORT || JS_HAS_E4X_FOR_EACH

Hmm.  This needing to check both macros seems like a bad idea.  How about we add

#if JS_HAS_XML_SUPPORT
#  define JS_HAS_FOR_EACH 1
#endif

so that we only have to test one macro?

That said, with allowsForEach always present, we don't need any #ifs here at all.  If there's no for-each support, compiler dead-code elimination will get rid of all this.

::: js/src/frontend/Parser.h
@@ +467,5 @@
>  
>      ParseNode *starOrAtPropertyIdentifier(TokenKind tt);
>      ParseNode *propertyQualifiedIdentifier();
> +#elif JS_HAS_E4X_FOR_EACH
> +    bool allowsE4XForEach() { return versionNumber() >= JSVERSION_1_6; }

Let's move allowsForEach (please rename it) out of this #ifdef entirely and just have it always present.

    bool allowsForEach() {
#if !JS_HAS_FOR_EACH
        return false;
#elif JS_HAS_XML_SUPPORT
        return allowsXML() || tokenStream.hasMoarXML();
#else
        return versionNumber() >= JSVERSION_1_6;
#endif
    }

::: js/src/jsversion.h
@@ +60,5 @@
>  #define JS_HAS_UNEVAL           0       /* has uneval() top-level function */
>  #define JS_HAS_CONST            0       /* has JS2 const as alternative var */
>  #define JS_HAS_FUN_EXPR_STMT    0       /* has function expression statement */
>  #define JS_HAS_NO_SUCH_METHOD   0       /* has o.__noSuchMethod__ handler */
> +#define JS_HAS_E4X_FOR_EACH     0       /* has for each (lhs in iterable) */

Let's name this JS_HAS_FOR_EACH -- no need to mention E4X here since we're basically splitting it away from E4X proper.
Attachment #692693 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 14

5 years ago
Created attachment 694586 [details] [diff] [review]
Part 2: Disable for-each-in from content by default

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> ::: js/src/frontend/Parser.cpp
> @@ +2989,5 @@
> >      PushStatementPC(pc, &forStmt, STMT_FOR_LOOP);
> >  
> >      pn->setOp(JSOP_ITER);
> >      pn->pn_iflags = 0;
> > +#if JS_HAS_XML_SUPPORT || JS_HAS_E4X_FOR_EACH
> 
> Hmm.  This needing to check both macros seems like a bad idea.  How about we
> add
> 
> #if JS_HAS_XML_SUPPORT
> #  define JS_HAS_FOR_EACH 1
> #endif
> 
> so that we only have to test one macro?
> 
> That said, with allowsForEach always present, we don't need any #ifs here at
> all.  If there's no for-each support, compiler dead-code elimination will
> get rid of all this.

Removed the #ifdefs.

> ::: js/src/frontend/Parser.h
> @@ +467,5 @@
> >  
> >      ParseNode *starOrAtPropertyIdentifier(TokenKind tt);
> >      ParseNode *propertyQualifiedIdentifier();
> > +#elif JS_HAS_E4X_FOR_EACH
> > +    bool allowsE4XForEach() { return versionNumber() >= JSVERSION_1_6; }
> 
> Let's move allowsForEach (please rename it) out of this #ifdef entirely and
> just have it always present.

Done.

>     bool allowsForEach() {
> #if !JS_HAS_FOR_EACH
>         return false;
> #elif JS_HAS_XML_SUPPORT
>         return allowsXML() || tokenStream.hasMoarXML();
> #else
>         return versionNumber() >= JSVERSION_1_6;
> #endif
>     }
> 
> ::: js/src/jsversion.h
> @@ +60,5 @@
> >  #define JS_HAS_UNEVAL           0       /* has uneval() top-level function */
> >  #define JS_HAS_CONST            0       /* has JS2 const as alternative var */
> >  #define JS_HAS_FUN_EXPR_STMT    0       /* has function expression statement */
> >  #define JS_HAS_NO_SUCH_METHOD   0       /* has o.__noSuchMethod__ handler */
> > +#define JS_HAS_E4X_FOR_EACH     0       /* has for each (lhs in iterable) */
> 
> Let's name this JS_HAS_FOR_EACH -- no need to mention E4X here since we're
> basically splitting it away from E4X proper.

I was a bit worried about a confusion with ES5 Array.forEach(). How about allowsForEachIn and JS_HAS_FOR_EACH_IN?
Attachment #692693 - Attachment is obsolete: true
Attachment #694586 - Flags: review?(jwalden+bmo)
Comment on attachment 694586 [details] [diff] [review]
Part 2: Disable for-each-in from content by default

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

> I was a bit worried about a confusion with ES5 Array.forEach(). How about allowsForEachIn and JS_HAS_FOR_EACH_IN?

That's fair, and that works.
Attachment #694586 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=17d1a8d4a680
Should I ask browser folks for a review of part 3?
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f89c4fb6dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddcfa9f47545

Testcase will be landed as soon as getting r+.
Assignee: general → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Whiteboard: [js:p3] → [js:p3][leave open]
Comment on attachment 692589 [details] [diff] [review]
Part 3: Add a regression test

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

::: dom/base/test/test_e4x_for_each.html
@@ +35,5 @@
> +    ok(t.enabled, "JavaScript" + (t.version ? " " + t.version : "") + " supports for-each-in");
> +  } catch (e) {
> +    ok(!t.enabled, "JavaScript" + (t.version ? " " + t.version : "") + " does NOT support for-each-in");
> +  }
> +}

Put this function into a <div id="template" style="display: none"></div>.

@@ +41,5 @@
> +  var t = tests[i];
> +  document.write('<script type="application/javascript' +
> +                 (t.version ? ';version=' + t.version : '') +
> +                 '">eval(runTest.toString());runTest(' + i +
> +                 ');<' + '/script>');

Then, here, create a new script element from that div's textContent:

  var script = document.createElement("script");
  script.textContent = document.getElementById("template").textContent + "\n" + "runTest(" + i + ");";
  document.body.appendChild(script);

We shouldn't be relying on function serialization as an implementation tactic here.

@@ +43,5 @@
> +                 (t.version ? ';version=' + t.version : '') +
> +                 '">eval(runTest.toString());runTest(' + i +
> +                 ');<' + '/script>');
> +}
> +is(count, tests.length, "runTest() call count");

Then append this as a final script element, after all the others.
Attachment #692589 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 19

5 years ago
Created attachment 695050 [details] [diff] [review]
Part 3: Add a regression test

Killing document.write and function.toString().
Attachment #692589 - Attachment is obsolete: true
Attachment #695050 - Flags: review?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/b0f89c4fb6dd
https://hg.mozilla.org/mozilla-central/rev/ddcfa9f47545
Comment on attachment 695050 [details] [diff] [review]
Part 3: Add a regression test

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

::: dom/base/test/test_e4x_for_each.html
@@ +41,5 @@
> +var count = 0;
> +for (var i = 0; i < tests.length; i++) {
> +  var t = tests[i];
> +  var script = document.createElement("script");
> +  script.type = "application/javascript" + (t.version ? ";version=" + t.version : "");

|"version" in t| reads nicer.
Attachment #695050 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/178bc511dec9
The next step would be disabling for-each-in on content JS regardless of the JS version (in a followup-bug).
Flags: in-testsuite- → in-testsuite+
Whiteboard: [js:p3][leave open] → [js:p3]
(Assignee)

Updated

5 years ago
Blocks: 824289
https://hg.mozilla.org/mozilla-central/rev/178bc511dec9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
This broke a bunch of tests in js/src/tests.

REGRESSIONS
    js1_6/extensions/regress-352392.js
    js1_6/extensions/regress-455464-01.js
    js1_6/extensions/regress-455464-02.js
    js1_6/extensions/regress-455464-03.js
    js1_6/extensions/regress-455464-04.js
    js1_6/extensions/regress-465443.js
    js1_6/extensions/regress-472508.js
    js1_6/extensions/regress-475144.js
    js1_6/extensions/regress-565521.js
    js1_6/Regress/regress-350417.js
    js1_6/Regress/regress-355002.js
    js1_6/Regress/regress-372565.js
FAIL

The first bad revision is:
changeset:   116743:ddcfa9f47545
user:        Masatoshi Kimura <VYV03354@nifty.ne.jp>
date:        Fri Dec 21 20:48:36 2012 +0900
summary:     Bug 804834 - Part 2: Disable for-each-in from content by default. r=waldo
(Assignee)

Comment 25

5 years ago
I added a "js1_6" check for the failed tests on browser.
https://hg.mozilla.org/mozilla-central/rev/b0f89c4fb6dd#l66.1
Probably corresponding check would be need for standalone JS.
That's the right call. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa426440a8e
https://hg.mozilla.org/mozilla-central/rev/3fa426440a8e
Depends on: 830665
Mentioned on https://developer.mozilla.org/en-US/docs/Firefox_20_for_developers and big fat warning on https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for_each...in.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.