Last Comment Bug 804834 - Hide "for each" from content
: Hide "for each" from content
Status: RESOLVED FIXED
[js:p3]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Masatoshi Kimura [:emk]
: general
:
Mentors:
data:text/html,<script>document.write...
Depends on: 830665
Blocks: 824289
  Show dependency treegraph
 
Reported: 2012-10-23 16:39 PDT by Terrence Cole [:terrence]
Modified: 2013-03-22 15:11 PDT (History)
13 users (show)
VYV03354: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Fix tests depending on E4X for-each in content JS (100.47 KB, patch)
2012-12-15 05:53 PST, Masatoshi Kimura [:emk]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 2: Disable for-each-in from content by default (7.69 KB, patch)
2012-12-15 05:56 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 3: Add a regression test (2.34 KB, patch)
2012-12-15 05:56 PST, Masatoshi Kimura [:emk]
jwalden+bmo: review-
Details | Diff | Splinter Review
Part 2: Disable for-each-in from content by default (9.07 KB, patch)
2012-12-15 21:37 PST, Masatoshi Kimura [:emk]
jwalden+bmo: review-
Details | Diff | Splinter Review
Part 2: Disable for-each-in from content by default (8.13 KB, patch)
2012-12-20 15:12 PST, Masatoshi Kimura [:emk]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 3: Add a regression test (2.56 KB, patch)
2012-12-21 16:29 PST, Masatoshi Kimura [:emk]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-10-23 16:39:23 PDT
This appears to be exposed to content in nightly.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-23 16:41:56 PDT
This is broken at least as far back as Firefox 16.
Comment 2 Masatoshi Kimura [:emk] 2012-12-11 19:43:00 PST
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] 2012-12-11 19:46:17 PST
FYI for-of is also visible from the content without any opt-in. Is it intentional?
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-12-12 11:25:59 PST
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] 2012-12-12 15:45:54 PST
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] (remove +bmo to email) 2012-12-13 08:10:42 PST
(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] 2012-12-15 05:53:26 PST
Created attachment 692587 [details] [diff] [review]
Part 1: Fix tests depending on E4X for-each in content JS
Comment 8 Masatoshi Kimura [:emk] 2012-12-15 05:56:04 PST
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.
Comment 9 Masatoshi Kimura [:emk] 2012-12-15 05:56:43 PST
Created attachment 692589 [details] [diff] [review]
Part 3: Add a regression test
Comment 10 Masatoshi Kimura [:emk] 2012-12-15 05:57:09 PST
https://tbpl.mozilla.org/?tree=Try&rev=6eba30b030b5
Comment 11 Masatoshi Kimura [:emk] 2012-12-15 21:37:14 PST
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.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2012-12-20 12:41:43 PST
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] (remove +bmo to email) 2012-12-20 13:06:47 PST
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] 2012-12-20 15:12:17 PST
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?
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2012-12-20 15:23:26 PST
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] 2012-12-20 17:01:39 PST
https://tbpl.mozilla.org/?tree=Try&rev=17d1a8d4a680
Should I ask browser folks for a review of part 3?
Comment 17 Masatoshi Kimura [:emk] 2012-12-21 03:53:39 PST
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] (remove +bmo to email) 2012-12-21 09:27:07 PST
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] 2012-12-21 16:29:02 PST
Created attachment 695050 [details] [diff] [review]
Part 3: Add a regression test

Killing document.write and function.toString().
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2012-12-22 10:03:35 PST
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] 2012-12-22 12:19:55 PST
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] 2012-12-23 13:09:15 PST
https://hg.mozilla.org/mozilla-central/rev/178bc511dec9
Comment 24 Jason Orendorff [:jorendorff] 2013-01-07 17:45:27 PST
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
Comment 25 Masatoshi Kimura [:emk] 2013-01-07 21:09:48 PST
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] 2013-01-08 11:14:28 PST
That's the right call. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa426440a8e
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-01-08 17:43:02 PST
https://hg.mozilla.org/mozilla-central/rev/3fa426440a8e

Note You need to log in before you can comment on or make changes to this bug.