804834 - Hide "for each" from content

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

This appears to be exposed to content in nightly.

Comment 1

[Jeff Walden [:Waldo]]

This is broken at least as far back as Firefox 16.

Comment 2

[Masatoshi Kimura [:emk]]

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.

Comment 3

[Masatoshi Kimura [:emk]]

FYI for-of is also visible from the content without any opt-in. Is it intentional?

Comment 4

[Jeff Walden [:Waldo]]

for-of is on the standards track.  for-each is about as far from that as it's possible to be.

Comment 5

[Masatoshi Kimura [:emk]]

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.

Comment 6

[Jeff Walden [:Waldo]]

(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.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Fix tests depending on E4X for-each in content JS

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: 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.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Add a regression test

Comment 10

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=6eba30b030b5

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Disable for-each-in from content by default

The previous patch didn't compile with JS_HAS_XML_SUPPORT=0.

Comment 12

[Jeff Walden [:Waldo]]

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.

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: 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?

Comment 15

[Jeff Walden [:Waldo]]

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.

Comment 16

[Masatoshi Kimura [:emk]]

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

Comment 17

[Masatoshi Kimura [:emk]]

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+.

Comment 18

[Jeff Walden [:Waldo]]

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.

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Add a regression test

Killing document.write and function.toString().

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b0f89c4fb6dd
https://hg.mozilla.org/mozilla-central/rev/ddcfa9f47545

Comment 21

[Jeff Walden [:Waldo]]

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.

Comment 22

[Masatoshi Kimura [:emk]]

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).

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/178bc511dec9

Comment 24

[Jason Orendorff [:jorendorff]]

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 <[e-mail]>
date:        Fri Dec 21 20:48:36 2012 +0900
summary:     Bug 804834 - Part 2: Disable for-each-in from content by default. r=waldo

Comment 25

[Masatoshi Kimura [:emk]]

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.

Comment 26

[Jason Orendorff [:jorendorff]]

That's the right call. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa426440a8e

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fa426440a8e

Comment 28

[Tom S [:evilpie]]

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.