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

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
P3
normal
ASSIGNED
2 years ago
7 months ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({leave-open, triage-deferred})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(24 attachments)

8.07 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.71 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.90 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.21 KB, patch
arai
: review+
Details | Diff | Splinter Review
158.86 KB, patch
arai
: review+
Details | Diff | Splinter Review
4.14 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.00 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.67 KB, patch
arai
: review+
Details | Diff | Splinter Review
6.50 KB, patch
arai
: review+
Details | Diff | Splinter Review
7.92 KB, patch
arai
: review+
Details | Diff | Splinter Review
56.40 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.12 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.48 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.49 KB, patch
arai
: review+
Details | Diff | Splinter Review
837 bytes, patch
arai
: review+
Details | Diff | Splinter Review
4.25 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.81 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.62 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.02 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.80 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.70 KB, patch
arai
: review+
Details | Diff | Splinter Review
6.96 KB, patch
arai
: review+
Details | Diff | Splinter Review
8.56 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.07 KB, patch
arai
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

2 years ago
Created 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
Attachment #8763007 - Flags: review?(arai.unmht)
(Assignee)

Comment 2

2 years ago
Created attachment 8763008 [details] [diff] [review]
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.
Attachment #8763008 - Flags: review?(arai.unmht)
(Assignee)

Comment 3

2 years ago
Created attachment 8763009 [details] [diff] [review]
Move DocumentWrite into browser.js's IIFE
Attachment #8763009 - Flags: review?(arai.unmht)
(Assignee)

Comment 4

2 years ago
Created attachment 8763010 [details] [diff] [review]
Move assertThrowsInstanceOf into shell.js's IIFE
Attachment #8763010 - Flags: review?(arai.unmht)
(Assignee)

Comment 5

2 years ago
Created attachment 8763011 [details] [diff] [review]
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*$/'
Attachment #8763011 - Flags: review?(arai.unmht)
(Assignee)

Comment 6

2 years ago
Created attachment 8763012 [details] [diff] [review]
Manually remove the remaining jit() call oddballs
Attachment #8763012 - Flags: review?(arai.unmht)
(Assignee)

Comment 7

2 years ago
Created attachment 8763013 [details] [diff] [review]
Remove jit() fully from shell.js/browser.js
Attachment #8763013 - Flags: review?(arai.unmht)
(Assignee)

Comment 8

2 years ago
Created attachment 8763014 [details] [diff] [review]
Move callstack-recording functionality into shell.js's IIFE
Attachment #8763014 - Flags: review?(arai.unmht)
(Assignee)

Comment 9

2 years ago
Created attachment 8763015 [details] [diff] [review]
Move assertEqArray into shell.js's IIFE, and delete a duplicate of it elsewhere
Attachment #8763015 - Flags: review?(arai.unmht)
(Assignee)

Comment 10

2 years ago
Created attachment 8763017 [details] [diff] [review]
Remove BigO from shell.js, and remove the one (disabled) test that used it
Attachment #8763017 - Flags: review?(arai.unmht)
(Assignee)

Comment 11

2 years ago
Created attachment 8763018 [details] [diff] [review]
Make jsref.js non-special as far as harnesses are concerned, and remove all jsref.js files as unused
Attachment #8763018 - Flags: review?(arai.unmht)
(Assignee)

Comment 12

2 years ago
Created attachment 8763019 [details] [diff] [review]
Remove the dead getFailedCases shell.js function
Attachment #8763019 - Flags: review?(arai.unmht)
(Assignee)

Comment 13

2 years ago
Created attachment 8763020 [details] [diff] [review]
Move Permutations into ecma_6/shell.js, out of the central shell.js
Attachment #8763020 - Flags: review?(arai.unmht)
(Assignee)

Comment 14

2 years ago
Created attachment 8763021 [details] [diff] [review]
Remove the unnecessary VERBOSE variable from shell.js
Attachment #8763021 - Flags: review?(arai.unmht)
(Assignee)

Comment 15

2 years ago
Created attachment 8763022 [details] [diff] [review]
Remove a never-observed |var DEBUG = false;|
Attachment #8763022 - Flags: review?(arai.unmht)
(Assignee)

Comment 16

2 years ago
And, immediate patchbombing complete.  :-)
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
Attachment #8763007 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Duplicate of this bug: 1180262

Updated

2 years ago
Attachment #8763008 - Flags: review?(arai.unmht) → review+
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
Attachment #8763009 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763010 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763011 - Flags: review?(arai.unmht) → review+
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
Attachment #8763012 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763013 - Flags: review?(arai.unmht) → review+
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?
about comment #21
Flags: needinfo?(jwalden+bmo)

Updated

2 years ago
Attachment #8763015 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763017 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763018 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763019 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763020 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763021 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8763022 - Flags: review?(arai.unmht) → review+
(Assignee)

Comment 23

2 years ago
(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.  :-)
Flags: needinfo?(jwalden+bmo)
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.
Attachment #8763014 - Flags: review?(arai.unmht) → review+

Comment 25

2 years ago
Pushed by jwalden@mit.edu:
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
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 27

2 years ago
Pushed by jwalden@mit.edu:
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

Updated

2 years ago
See Also: → bug 1285465

Comment 29

2 years ago
Pushed by jwalden@mit.edu:
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
(Assignee)

Comment 31

2 years ago
Created attachment 8777878 [details] [diff] [review]
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.
Attachment #8777878 - Flags: review?(arai.unmht)
(Assignee)

Comment 32

2 years ago
Created attachment 8777879 [details] [diff] [review]
Make browser.js's newGlobal and DocumentWrite work regardless of the test's behavior
Attachment #8777879 - Flags: review?(arai.unmht)
(Assignee)

Comment 33

2 years ago
Created attachment 8777880 [details] [diff] [review]
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.
Attachment #8777880 - Flags: review?(arai.unmht)
(Assignee)

Comment 34

2 years ago
Created attachment 8777881 [details] [diff] [review]
Make browser.js's writeHeaderToLog write into the page in a way that never sets innerHTML or uses document.write
Attachment #8777881 - Flags: review?(arai.unmht)
(Assignee)

Comment 35

2 years ago
Created attachment 8777882 [details] [diff] [review]
Make writeFormattedResult write into the page in a way that never sets innerHTML or uses document.write
Attachment #8777882 - Flags: review?(arai.unmht)
(Assignee)

Comment 36

2 years ago
Created attachment 8777883 [details] [diff] [review]
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.
Attachment #8777883 - Flags: review?(arai.unmht)
(Assignee)

Comment 37

2 years ago
Created attachment 8777884 [details] [diff] [review]
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.
Attachment #8777884 - Flags: review?(arai.unmht)
(Assignee)

Comment 38

2 years ago
Created attachment 8777885 [details] [diff] [review]
Remove the pointless stopTest function
Attachment #8777885 - Flags: review?(arai.unmht)
(Assignee)

Comment 39

2 years ago
Created attachment 8777886 [details] [diff] [review]
Remove browser.js's never-used include() function
Attachment #8777886 - Flags: review?(arai.unmht)
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.
Attachment #8777878 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8777879 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8777880 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8777881 - Flags: review?(arai.unmht) → review+
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)
Attachment #8777882 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8777883 - Flags: review?(arai.unmht) → review+
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...
Attachment #8777884 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8777885 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8777886 - Flags: review?(arai.unmht) → review+

Comment 43

2 years ago
Pushed by jwalden@mit.edu:
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
Depends on: 1338884
Keywords: triage-deferred
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.