1280362 - Clean up jstests' shell.js and browser.js to do everything in IIFEs, with explicit functionality exports for exposure to tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jeff Walden [:Waldo]]

The "API" exposed to jstests is a bit vague -- a bunch of function names that probably nobody ever fully uses.  And exactly what things tests cannot do, because doing so would break the harness that runs after the test code has, is wholly undocumented.  We should make shell.js/browser.js do all their work in IIFEs, and when they want to add a function to the global object for use in tests, they should do it via an exports.assertEq = ...-style process that makes those exports extremely clear.

I have a dozen patches that *start* work on adding IIFEs, making the relied-upon functionality in exported functions not depend upon test-modifiable semantics (by caching primordial functionality that can't be affected by the test's subsequent shenanigans), &c.  There's still a ton more to do on this point even past those patches, so this'll probably remain open for a bit as we slog through it all.

Preliminarily, I should note that these dozen-plus patches I'm uploading do *not* work in some small detail, but I don't know which detail.  This is mostly to get a ball rolling, and if it turns out the relevant buglet is complex somehow, I'll repost the relevant part for rereview.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Begin to make jstests' shell.js/browser.js less dependent on tests being well-behaved, and make exporting functionality a more deliberate choice

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Move newGlobal's browser polyfill into shell.js, as the first step of unifying shell.js/browser.js

Fair warning, these changes are probably laboriously minimal.  :-)  Better than mega-patches that are unreviewable.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Move DocumentWrite into browser.js's IIFE

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Move assertThrowsInstanceOf into shell.js's IIFE

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Remove a gazillion useless jit(true) and jit(false) calls from jstests

The one-liner used to perform these changes (in js/src/tests) was:

  find . -name '*.js' | xargs perl -ni -e 'print unless /^\s*jit'\\'(''(true|false)'\\');\s*$/'

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Manually remove the remaining jit() call oddballs

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Remove jit() fully from shell.js/browser.js

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Move callstack-recording functionality into shell.js's IIFE

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Move assertEqArray into shell.js's IIFE, and delete a duplicate of it elsewhere

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Remove BigO from shell.js, and remove the one (disabled) test that used it

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Make jsref.js non-special as far as harnesses are concerned, and remove all jsref.js files as unused

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Remove the dead getFailedCases shell.js function

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Move Permutations into ecma_6/shell.js, out of the central shell.js

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Remove the unnecessary VERBOSE variable from shell.js

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Remove a never-observed |var DEBUG = false;|

Comment 16

[Jeff Walden [:Waldo]]

And, immediate patchbombing complete.  :-)

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8763007 [details] [diff] [review]
Begin to make jstests' shell.js/browser.js less dependent on tests being well-behaved, and make exporting functionality a more deliberate choice

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

Thanks :)
Will review remaining parts gradually, hopefully within this week.

::: js/src/tests/shell.js
@@ +34,5 @@
> +  var assertEq = global.assertEq;
> +  if (typeof assertEq !== "function") {
> +    assertEq = function assertEq(actual, expected, message) {
> +      if (!SameValue(actual, expected))
> +      {

would be better moving this "{" to the end of the previous line

Comment 19

[Tooru Fujisawa [:arai]]

Comment on attachment 8763009 [details] [diff] [review]
Move DocumentWrite into browser.js's IIFE

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

::: js/src/tests/browser.js
@@ +38,5 @@
> +  function DocumentWrite(s) {
> +    try {
> +      var msgDiv = global.document.createElement('div');
> +      msgDiv.innerHTML = s;
> +      document.body.appendChild(msgDiv);

global.document.body.appendChild

Comment 20

[Tooru Fujisawa [:arai]]

Comment on attachment 8763012 [details] [diff] [review]
Manually remove the remaining jit() call oddballs

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

r+ assuming the function in bug646001.js is kept

::: js/src/jit-test/tests/jaeger/bug646001.js
@@ -1,3 @@
> -function jit(on)
> -{
> -  if (on && !options().match(/tracejit/)) { }

according to bug 646001 comment #0, this line was necessary to reproduce the issue.
not sure if it makes sense to keep whole this testcase tho, at least it would make sense to keep this function here (as renamed?) as long as keeping whole testcase

Comment 21

[Tooru Fujisawa [:arai]]

Comment on attachment 8763014 [details] [diff] [review]
Move callstack-recording functionality into shell.js's IIFE

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

::: js/src/tests/shell.js
@@ +36,5 @@
> +
> +    var v = arr[len - 1];
> +    arr.length--;
> +    return v;
> +  }

Any reason not to use Array.prototype.pop and push?

Comment 22

[Tooru Fujisawa [:arai]]

about comment #21

Comment 23

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #21)
> Any reason not to use Array.prototype.pop and push?

I think Array.prototype.pop, cached, could be used.  I did it this way to be absolutely clear about its having no side effects, other than length-consultation and modification if necessary.  That's not necessarily so clear for a mutating, builtin function.

Array.prototype.push isn't used because it *sets* the element to append it to the array, rather than defining it.  So it's vulnerable to a setter on Array.prototype or Object.prototype making behavior undependable.  This is a little odd -- because I think we get this wrong now.  Chrome, at least, correctly prints with this testcase:

  Object.defineProperty(Array.prototype, 0, { set: function(v) { print('hi'); } });
  [].push(5);

But we don't seem to in Firefox (and I think not in a shell build, although I'm having trouble making one right now because of build nonsense).  I don't know if we have a bug on file for this yet; I'll try to find one.

I'm tryservering everything up to (but not including) this patch to land it first, separately, so you have a little time before I get to the rest of the patches here, if you have any observations you want to make.  :-)

Comment 24

[Tooru Fujisawa [:arai]]

Comment on attachment 8763014 [details] [diff] [review]
Move callstack-recording functionality into shell.js's IIFE

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

Thank you for clarification :)

In that case, it would be nice to have some comment about the reason why re-implementing push/pop there,
so that no one won't re-write it with cached Array.prototype.push in future.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0192747331
Begin to make jstests' shell.js/browser.js less dependent on tests being well-behaved, and make exporting functionality a more deliberate choice.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/94cec7dd2981
Move newGlobal's browser polyfill into shell.js, as the first step of unifying shell.js/browser.js.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ffca66a5b6
Move DocumentWrite into browser.js's IIFE.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2d92d20513
Move assertThrowsInstanceOf into shell.js's IIFE.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/11cea69b0788
Remove a gazillion useless jit(true) and jit(false) calls from jstests.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/b179ec85b1c0
Manually remove the remaining jit() call oddballs.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b09cf811dd
Remove jit() fully from shell.js/browser.js.  r=arai

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7d0192747331
https://hg.mozilla.org/mozilla-central/rev/94cec7dd2981
https://hg.mozilla.org/mozilla-central/rev/c0ffca66a5b6
https://hg.mozilla.org/mozilla-central/rev/3e2d92d20513
https://hg.mozilla.org/mozilla-central/rev/11cea69b0788
https://hg.mozilla.org/mozilla-central/rev/b179ec85b1c0
https://hg.mozilla.org/mozilla-central/rev/f4b09cf811dd

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbde749db2bd
Move callstack-recording functionality into shell.js's IIFE.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/056efc0848c3
Move assertEqArray into shell.js's IIFE, and delete a duplicate of it elsewhere.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ab52f7cf76
Remove BigO from shell.js, and remove the one (disabled) test that used it.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e274e923823
Make jsref.js non-special as far as harnesses are concerned, and remove all jsref.js files as unused.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c003801a25c1
Remove the dead getFailedCases shell.js function.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf85303aeb2
Move Permutations into ecma_6/shell.js, out of the central shell.js.  r=arai

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbde749db2bd
https://hg.mozilla.org/mozilla-central/rev/056efc0848c3
https://hg.mozilla.org/mozilla-central/rev/d3ab52f7cf76
https://hg.mozilla.org/mozilla-central/rev/4e274e923823
https://hg.mozilla.org/mozilla-central/rev/c003801a25c1
https://hg.mozilla.org/mozilla-central/rev/cbf85303aeb2

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d62360b963
Remove the unnecessary VERBOSE variable from shell.js.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/90582bb8f7ed
Remove a never-observed |var DEBUG = false;|.  r=arai

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9d62360b963
https://hg.mozilla.org/mozilla-central/rev/90582bb8f7ed

Comment 31

[Jeff Walden [:Waldo]]

Attachment: Define gczeal/quit in shell.js, out of browser.js, plus a few other minor changes

In the longer run, everything should be in shell.js.  For now, I am going to be somewhat inconsistent about whether cleanups in browser.js move functionality into shell.js, or simply into browser.js's IIFE.  Once everything's in IIFEs, moving stuff out of browser.js should be easier.  In the meantime, and especially for functionality that other non-IIFE functions in browser.js use, I somewhat prefer leaving the functionality definition in browser.js, rather than move it to shell.js and have to export it.

Comment 32

[Jeff Walden [:Waldo]]

Attachment: Make browser.js's newGlobal and DocumentWrite work regardless of the test's behavior

Comment 33

[Jeff Walden [:Waldo]]

Attachment: Make browser.js's print() function write into the page in a way that never sets innerHTML or uses document.write

Obviously we should be a bit leery about changing the main harness HTML file.  But there's not really any other way to do this, than to move things around.  Fortunately, jstests rarely directly interact with the harness page.

Comment 34

[Jeff Walden [:Waldo]]

Attachment: Make browser.js's writeHeaderToLog write into the page in a way that never sets innerHTML or uses document.write

Comment 35

[Jeff Walden [:Waldo]]

Attachment: Make writeFormattedResult write into the page in a way that never sets innerHTML or uses document.write

Comment 36

[Jeff Walden [:Waldo]]

Attachment: Remove DocumentWrite and htmlesc as unused

Some non-IIFE'd browser.js code still uses document.write, but this gets rid of all the code that had to do faux HTML escaping, which is a big win.

Comment 37

[Jeff Walden [:Waldo]]

Attachment: Define and export print and dump functions from shell.js's IIFE *only*

This one took awhile to get done, with all the preliminary patches.

Comment 38

[Jeff Walden [:Waldo]]

Attachment: Remove the pointless stopTest function

Comment 39

[Jeff Walden [:Waldo]]

Attachment: Remove browser.js's never-used include() function

Comment 40

[Tooru Fujisawa [:arai]]

Comment on attachment 8777878 [details] [diff] [review]
Define gczeal/quit in shell.js, out of browser.js, plus a few other minor changes

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

Thanks :)
will review remaining patches tomorrow.

Comment 41

[Tooru Fujisawa [:arai]]

Comment on attachment 8777882 [details] [diff] [review]
Make writeFormattedResult write into the page in a way that never sets innerHTML or uses document.write

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

::: js/src/tests/browser.js
@@ +158,5 @@
> +    AppendChild(b, font);
> +
> +    var tt = CreateElement("tt");
> +    SetTextContent(tt, string);
> +    AppendChild(tt, b);

I hope we can replace those font/b/tt elements' effect with CSS and move those styling into HTML file.
(not in this patch.  later patch or separated bug)

Comment 42

[Tooru Fujisawa [:arai]]

Comment on attachment 8777884 [details] [diff] [review]
Define and export print and dump functions from shell.js's IIFE *only*

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

::: js/src/tests/shell.js
@@ +153,5 @@
>    /****************************
>     * UTILITY FUNCTION EXPORTS *
>     ****************************/
>  
> +  var dump = global.dump;

I was about to ask this for previous patch.
good to see this here :)

@@ +167,5 @@
> +      // no-op.
> +      dump = function() {};
> +    } else {
> +      // |print| prints to stdout: make |dump| do likewise.
> +      dump = global.print;

|dump| doesn't put newline at the end, but |print| does.
so this will put extra newline for standard use-case of |dump|, like:
  dump("hello\n");
it won't matter much here tho...

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b22fd600973
Define gczeal/quit in shell.js, out of browser.js, plus a few other minor changes.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/00936e6b524e
Make browser.js's newGlobal and DocumentWrite work regardless of the test's behavior.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4862d126b246
Make browser.js's print() function write into the page in a way that never sets innerHTML or uses document.write.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf48cc23269d
Make browser.js's writeHeaderToLog write into the page in a way that never sets innerHTML or uses document.write.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/df10776fd165
Make writeFormattedResult write into the page in a way that never sets innerHTML or uses document.write.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a3429db6cb
Remove DocumentWrite and htmlesc as unused.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/e96d2fb8ccc7
Define and export print and dump functions from shell.js's IIFE *only*.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9a6e4989147
Remove the pointless stopTest function from shell.js.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a03ccc7c02e
Remove browser.js's never-used include() function.  r=arai

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b22fd600973
https://hg.mozilla.org/mozilla-central/rev/00936e6b524e
https://hg.mozilla.org/mozilla-central/rev/4862d126b246
https://hg.mozilla.org/mozilla-central/rev/cf48cc23269d
https://hg.mozilla.org/mozilla-central/rev/df10776fd165
https://hg.mozilla.org/mozilla-central/rev/c2a3429db6cb
https://hg.mozilla.org/mozilla-central/rev/e96d2fb8ccc7
https://hg.mozilla.org/mozilla-central/rev/e9a6e4989147
https://hg.mozilla.org/mozilla-central/rev/6a03ccc7c02e

Comment 45

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:Waldo, maybe it's time to close this bug?

Comment 46

[Jeff Walden [:Waldo]]

I'd really like to combine/unify shell.js and browser.js into one still, but it *does* appear the two files do everything in IIFEs.  (If three of them, because browser.js has two for some reason.)  Let's call this done and deal with the rest in other bugs, whenever someone gets has reason to deal with that.