1512989 - jstests in the browser are broken, failures are not reported

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Bug 1510819 sort of exposed this because J1 on OS X starts hitting the 1h timeout (intermittently). Looking at the logs there are many failures that should result in orange but don't:

[task 2018-12-10T14:23:43.892Z] 14:23:43     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js
[task 2018-12-10T14:23:43.893Z] 14:23:43     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js | 1959 / 11236 (17%)
[task 2018-12-10T14:23:43.944Z] 14:23:43     INFO - TEST-INFO |  FAILED!  ReferenceError: quit is not defined
[task 2018-12-10T14:23:43.945Z] 14:23:43     INFO - JavaScript error: file:///builds/worker/workspace/build/tests/jsreftest/tests/non262/regress/regress-1476383-calloc-exc.js, line 3: ReferenceError: quit is not defined
[task 2018-12-10T14:23:43.970Z] 14:23:43     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js | (LOAD ONLY)
[task 2018-12-10T14:23:43.970Z] 14:23:43     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js

Comment 1

[Jan de Mooij [:jandem]]

It may be interesting to bisect this to see when it started.

Comment 2

[Jason Orendorff [:jorendorff]]

My first guess is that this is intentional -- somewhere there's a log-parsing script with a long list of regular expressions, and `/quit is not defined/` is on that list.

It should be fixed, though.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> My first guess is that this is intentional -- somewhere there's a
> log-parsing script with a long list of regular expressions, and `/quit is
> not defined/` is on that list.

I don't think this is true. The failure in bug 1510819 is a similar issue that's being swept under the rug - that job just turns orange because there's a timeout.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So both here and in bug 1510819, we get

INFO - REFTEST TEST-PASS | [path] | (LOAD ONLY)

and I found a comment that claims this means "test without a reference (just test that it does not assert, crash, hang, or leak)". The failing tests throw an exception, but don't seem to be doing any of the things that are expected to make it fail, according to the comment.

Possibly these tests should be "script" rather than "load" tests; I managed to trace the determination back as far as https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/layout/tools/reftest/reftest.jsm#739

Comment 5

[André Bargull [:anba]]

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #4)
> Possibly these tests should be "script" rather than "load" tests; 

^ This!

Starting with bug 1392391, test entries for TYPE_SCRIPT are marked as TYPE_LOAD. [1]

[1] https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/layout/tools/reftest/manifest.jsm#299

Comment 6

[André Bargull [:anba]]

Attachment: bug1512989-part1-pass-test-type.patch

Changes ReadManifest to propagate the correct test type.

Comment 7

[André Bargull [:anba]]

Attachment: bug1512989-part2-update-tests.patch

Fix various tests to work correctly in browser environments. 

Changes:

js/src/tests/Makefile.in
- The "whatwg" directory wasn't packed for the test environment.

js/src/tests/jstests.list
js/src/tests/non262/lexical-environment/block-scoped-functions-annex-b-with.js
- ECMA-262 defines that global var-bindings are non-configurable, but in the browser they're apparently configurable.

js/src/tests/non262/Date/browser.js
js/src/tests/non262/String/ropes.js
- Export various testing functions to the global scope.

js/src/tests/non262/ReadableStream/subclassing.js
js/src/tests/non262/generators/yield-star-throw-htmldda.js
js/src/tests/non262/regress/regress-1456512-greyreadbarrier.js
js/src/tests/non262/regress/regress-1456512.js
js/src/tests/non262/regress/regress-1463421.js
js/src/tests/non262/regress/regress-1476383-calloc-exc.js
- Shell-only functions are called in these tests.

js/src/tests/non262/String/replace-rope-empty.js
- The "isRope" testing function doesn't seem to work correctly in a browser environment.
- I guess the function is in a different compartment, so when the argument string is passed to this different compartment, it gets flattened...?

js/src/tests/test262-update.py
js/src/tests/test262/*
- dynamic import is only available in the shell
- regenerated the tests to contain the correct skip annotation

js/src/tests/ecma_6/Date/parse-from-tostring-methods.js
- Test file wasn't moved into the correct directory when the patch was rebased.

Comment 8

[Jan de Mooij [:jandem]]

anba++

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 9030511 [details] [diff] [review]
bug1512989-part1-pass-test-type.patch

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

Sorry about that :(, we need better test coverage for the harness. I'm going to see if there's an easy way to make sure these tests fail if they have the wrong type, but feel free to go ahead and land (assuming there haven't been regressions in the meantime).

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 9030517 [details] [diff] [review]
bug1512989-part2-update-tests.patch

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

Thank you so much for doing this.

::: js/src/tests/non262/generators/yield-star-throw-htmldda.js
@@ +1,1 @@
> +// |reftest| skip-if(!xulRuntime.shell) -- needs createIsHTMLDDA

This horrible functionality is for the browser's benefit, so let's make the test run in the browser:

if (this.createIsHTMLDDA === undefined) {
  this.createIsHTMLDDA = () => document.all;
}

::: js/src/tests/non262/regress/regress-1476383-calloc-exc.js
@@ +3,4 @@
>  if (!this.oomTest) {
>      this.reportCompare && reportCompare(true, true);
>      quit(0);
>  }

Please consider `skip-if(!this.oomTest)` and deleting lines 3-6.

Comment 11

[André Bargull [:anba]]

Attachment: bug1512989-part2-update-tests.patch

Updated part 2 per review comments, carrying r+.

Comment 12

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> ::: js/src/tests/non262/generators/yield-star-throw-htmldda.js
> @@ +1,1 @@
> > +// |reftest| skip-if(!xulRuntime.shell) -- needs createIsHTMLDDA
> 
> This horrible functionality is for the browser's benefit, so let's make the
> test run in the browser:
> 
> if (this.createIsHTMLDDA === undefined) {
>   this.createIsHTMLDDA = () => document.all;
> }

I saw that there are more tests using dynamic checks to see if |createIsHTMLDDA| is present, so I've instead added "createIsHTMLDDA" to js/src/tests/browser.js to enable us to run these on the browser, too.

Comment 13

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f53ead27ca4813dcb7e833d52da5ca731455340b

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a6d8169f80
Part 1: Pass correct test type. r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c79e8192f9f
Part 2: Fix browser jstests failures. r=jorendorff

Comment 15

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1512989) for jsreftests failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=216660022&revision=4c79e8192f9f6e64d79d406f9fe4bd975a172229

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216660022&repo=mozilla-inbound&lineNumber=3566

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/22e977cbf29560f5dd7e88f6349789c2649356a3

Comment 16

[André Bargull [:anba]]

Hmm, based on the time zone offsets in the error messages it looks like the "OS X 10.10" and "Android 4.3" both run with the time zone "America/Los_Angeles" instead of "PST8PDT". And "Android 7.0 x86" runs with "Europe/London" instead of "PST8PDT".

Comment 17

[André Bargull [:anba]]

Attachment: bug1512989-part2-update-tests.patch

Two updates:

js/src/tests/non262/Date/15.9.4.2.js
- Handle the case when the time zone offset in January 1970 is different than the time zone offset in January 2009.


js/src/tests/non262/Date/parse-from-tostring-methods.js
- Handle the case when the time zone offset contains seconds (or milliseconds).
- The time zone offset for America/Los_Angeles is -7:52:58 until 1883 Nov 18 12:07:02 (cf. https://en.wikipedia.org/wiki/Local_Mean_Time)
- This also means the requirements for Date.parse in <https://tc39.github.io/ecma262/#sec-date.parse> need to be updated, because |x.valueOf()| may be different than |Date.parse(x.toString())| when the time zone string in |x.toString()| omits seconds offset values.

Comment 18

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80676ed904cd279861fa89a075e8f1f11ce72e8d

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94904f2a0a97
Part 1: Pass correct test type. r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/099ba012de27
Part 2: Fix browser jstests failures. r=jorendorff

Comment 20

[Bogdan Tara[:bogdan_tara]]

https://hg.mozilla.org/mozilla-central/rev/94904f2a0a97
https://hg.mozilla.org/mozilla-central/rev/099ba012de27