If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fix all tests that break with E4X support removed

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(8 attachments, 2 obsolete attachments)

32.68 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
474.11 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
70.47 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
16.95 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.62 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
9.97 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.45 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.39 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 705225 [details] [diff] [review]
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.
Attachment #705225 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 705708 [details] [diff] [review]
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".)
Attachment #705708 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #705225 - Attachment is obsolete: true
Attachment #705225 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #705708 - Attachment is obsolete: true
Attachment #705708 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 833668
(Assignee)

Comment 4

5 years ago
I might as well do jstests and mochitests in here, too.
(Assignee)

Updated

5 years ago
Summary: Fix all jit-tests that break with --disable-e4x → Fix all tests that break with E4X support removed
(Assignee)

Comment 5

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

Not much different to the last version.
Attachment #706150 - Flags: review?(jwalden+bmo)
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.
Attachment #706150 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 707440 [details] [diff] [review]
(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
Attachment #707440 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

5 years ago
Created attachment 707441 [details] [diff] [review]
(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.
Attachment #707441 - Flags: review?(jorendorff)
(Assignee)

Comment 9

5 years ago
Created attachment 707442 [details] [diff] [review]
(part 2c) - Remove e4x support from jstests.

This patch removes "|reftest| pref(javascript.options.xml.content,true)" from
all tests that have it.
Attachment #707442 - Flags: review?(jorendorff)
(Assignee)

Comment 10

5 years ago
Created attachment 707444 [details] [diff] [review]
(part 2d) - Remove e4x support from jstests.

This patch removes e4x bits from the jstests infrastructure.
Attachment #707444 - Flags: review?(jorendorff)
(Assignee)

Comment 11

5 years ago
Created attachment 707458 [details] [diff] [review]
(part 3) - Remove e4x support from jsapi-tests.

jsapi-tests are next.
Attachment #707458 - Flags: review?(jorendorff)
(Assignee)

Comment 12

5 years ago
Created attachment 707459 [details] [diff] [review]
(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.
Attachment #707459 - Flags: review?(jorendorff)
(Assignee)

Comment 13

5 years ago
Created attachment 707461 [details] [diff] [review]
(part 5) - Remove e4x support from mochitests.

And finally mochitests.
Attachment #707461 - Flags: review?(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.
Attachment #707440 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #707441 - Flags: review?(jorendorff) → review+
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.
Attachment #707442 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 17

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

Oh yeah... I missed some.
Attachment #707444 - Flags: review?(jorendorff) → review+
Attachment #707458 - Flags: review?(jorendorff) → review+
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.
Attachment #707459 - Flags: review?(jorendorff) → review+
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.
Attachment #707461 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 20

5 years ago
> ::: 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.
(Assignee)

Comment 21

5 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.