833208 - Fix all tests that break with E4X support removed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

56 jit-tests fail when the shell is built with --disable-e4x.  They need to be fixed -- either the test should be removed, or the e4x part(s) should be removed.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix or remove all tests that break with --disable-e4x.

This patch:

- Deletes tests where the e4x part is most or all of it.

- Removes/replaces e4x stuff in tests where the e4x stuff is only part of it.
  The replacements were chosen in a rather ad hoc fashion.

- Moves the three remaining tests from jit-tests/tests/e4x into
  jit-tests/test/basic, and removes jit-tests/tests/e4x.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Fix or remove all tests that break with --disable-e4x.

New version.  I missed some cases because I was using --no-slow.  Also, some
test had e4x stuff that apparently didn't cause them to fail;  but I removed
that code anyway.  (I found it by grepping for "xml" and "XML".)

Comment 4

[Nicholas Nethercote [inactive]]

I might as well do jstests and mochitests in here, too.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Fix or remove all jit-tests that break E4X support removed.

Not much different to the last version.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 706150 [details] [diff] [review]
(part 1) - Fix or remove all jit-tests that break E4X support removed.

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

Looks good, r and/or rs=me-ish on this.

::: js/src/jit-test/tests/basic/bug646968-6.js
@@ +1,3 @@
>  function test(s) {
>      eval(s);
> +    let (a = {} = 0, x = x)

{} doesn't read very well as an LHS here (only if you allow empty destructuring patterns, which read pretty horribly) -- ({}).q maybe?

::: js/src/jit-test/tests/e4x/bug651966.js
@@ +21,2 @@
>  f();
>  try { f("<x/>.(x=[]);function x(){}(x())"); } catch (e) {}

This garbage should be changed, too.

Ideally we'd remove all E4X-like syntax completely from everything, but it's probably too hard to exhaustively search for it.  But if we see instances that for whatever reason happen to be harmless, we should kill them.

@@ +25,5 @@
>      a = {
>          x
>      } = x, (x._)
>      function
> +    x(){}

Make this x()({}) or something?  Looks like you're removing a call to |x()| here, if you know enough E4X syntax.

::: js/src/jit-test/tests/jaeger/bug576398.js
@@ +1,2 @@
>  function K(x) {
> +  var dontCompile = [1,2,3];

Going by the name, this was supposed to prevent compilation by using an XML op.  Now...hmm, I dunno what that'd be.  I think probably you want to replace this whole like with a |with ({});| to get an equivalent-enough effect.

::: js/src/jit-test/tests/jaeger/bug601982.js
@@ +5,2 @@
>      if (i % 3)
> +        [1,2,3]

Use a |with ({});| here too.

@@ +16,5 @@
>  }
>  
>  function g(i) {
> +    /* Method JIT will try to remove this frame(?) */
> +    if (i == 14) { [1,2,3] }

Try a |with ({});| here instead, I think.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 2a) - Remove e4x support from jstests.

The next four patches fix up jstests.  I've split them for ease of reviewing,
but I'll fold them together before landing because the intermittent states
aren't valid.

----

This patch deals with the js/src/tests/e4x/ directory.  I went through all the
tests, and remove all of them except the following four, which all got moved
into different directories:

e4x/Statements/12.3-01.js     --> js1_6/extensions/nested-for-each.js
e4x/Regress/regress-355569.js --> js1_8_5/regress/regress-355569.js
e4x/Regress/regress-477053.js --> js1_8_5/regress/regress-477053.js
e4x/Regress/regress-561031.js --> js1_8_5/regress/regress-561031.js

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 2b) - Remove e4x support from jstests.

This patch deals with the tests outside of the js/src/tests/e4x/ directory.
Some were removed;  some were modified.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 2c) - Remove e4x support from jstests.

This patch removes "|reftest| pref(javascript.options.xml.content,true)" from
all tests that have it.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 2d) - Remove e4x support from jstests.

This patch removes e4x bits from the jstests infrastructure.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove e4x support from jsapi-tests.

jsapi-tests are next.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove e4x support from xpcshell tests.

And now for xpcshell tests.  bz and dbaron said on IRC that the use of |..| in
test_protocolproxyservice.js was almost certainly a typo, and that the code was
probably unused, which would explain why the test passes.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove e4x support from mochitests.

And finally mochitests.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 707440 [details] [diff] [review]
(part 2a) - Remove e4x support from jstests.

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

Stealing review. Looks good.

::: js/src/tests/narcissus-failures.txt
@@ +1,2 @@
> +e4x/Regress/regress-380833.js
> +e4x/Regress/regress-561031.js

e4x/Regress/regress-380833.js was deleted, and e4x/Regress/regress-561031.js was moved to another directory.

@@ +3,1 @@
>  e4x/Regress/regress-355569.js

This file was renamed too.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 707441 [details] [diff] [review]
(part 2b) - Remove e4x support from jstests.

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

::: js/src/tests/ecma_5/extensions/strict-e4x-ban.js
@@ +9,1 @@
>                                parseRaisesException(SyntaxError)),

You can just delete this one.

::: js/src/tests/js1_6/Regress/regress-314887.js
@@ -5,5 @@
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -
> -//-----------------------------------------------------------------------------
> -var BUGNUMBER = 314887;
> -var summary = 'Do not crash when morons embed script tags in external script files';

Heh. I almost hate to see this go. Classic.

::: js/src/tests/js1_7/extensions/regress-351102-04.js
@@ -23,5 @@
> - 
> -  var f;
> -  try
> -  {
> -    try { @foo } catch([] if gc()) { }

I think this could be kept, replacing @foo with any name that will trigger a ReferenceError.

::: js/src/tests/js1_8/extensions/regress-476871-02.js
@@ -19,5 @@
> -{
> -  this.watch("NaN", /a/g)
> -    let(x)
> -    ((function(){
> -        for each (NaN in [null, '', '', '', '']) true;

I think you could leave this in, too. It doesn't seem to be testing anything E4X-specific, unless you count `for each`.

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ -898,5 @@
> -// E4X
> -
> -assertExpr("x..tagName", binExpr("..", ident("x"), lit("tagName")));
> -assertExpr("x.*", memExpr(ident("x"), xmlAnyName));
> -assertExpr("x[*]", memExpr(ident("x"), xmlAnyName));

I'm so glad to see this stuff go I could cry.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 707442 [details] [diff] [review]
(part 2c) - Remove e4x support from jstests.

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

::: js/src/tests/js1_8_1/regress/regress-452498-053.js
@@ +1,1 @@
> +// |reftest| 

Delete the line altogether, in these cases?

In any case we don't want trailing whitespace.

Comment 17

[Nicholas Nethercote [inactive]]

> > +// |reftest| 
> 
> Delete the line altogether, in these cases?

Oh yeah... I missed some.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 707459 [details] [diff] [review]
(part 4) - Remove e4x support from xpcshell tests.

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

::: netwerk/test/unit/test_protocolproxyservice.js
@@ -141,5 @@
>    nextFunction: null,
>  
>    QueryInterface : function (iid) {
>      const interfaces = [Components.interfaces.nsIProtocolProxyCallback,
> -                        Components.interfaces..nsISupports];

...Whoa.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 707461 [details] [diff] [review]
(part 5) - Remove e4x support from mochitests.

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

::: js/xpconnect/tests/mochitest/test_bug618017.html
@@ -38,5 @@
> -<script type='application/javascript;version=1.7'>
> -let x = 12;
> -function doLetEval() {
> -  ok(eval('let x = 13; x') === 13, 'let statement is valid syntax in version 1.7');
> -}

This part isn't really to do with E4X, and seems still worth testing in the browser.

Comment 20

[Nicholas Nethercote [inactive]]

> ::: js/src/tests/js1_8/extensions/regress-476871-02.js
> @@ -19,5 @@
> > -{
> > -  this.watch("NaN", /a/g)
> > -    let(x)
> > -    ((function(){
> > -        for each (NaN in [null, '', '', '', '']) true;
> 
> I think you could leave this in, too. It doesn't seem to be testing anything
> E4X-specific, unless you count `for each`.

It's easy to miss, but there's this lower down:

  NaN.( /x/ );

The test doesn't do much else of note, so I'll stick with the decision to delete it.

Comment 21

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cde4cf36ca7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1606f7f92fb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/43fec63eae07
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23bdbc1f145
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7cba737027

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cde4cf36ca7c
https://hg.mozilla.org/mozilla-central/rev/1606f7f92fb9
https://hg.mozilla.org/mozilla-central/rev/43fec63eae07
https://hg.mozilla.org/mozilla-central/rev/a23bdbc1f145
https://hg.mozilla.org/mozilla-central/rev/ef7cba737027