1598612 - Reenable js ref tests for Atomic

Status: NEW

(Core :: JavaScript Engine, task, P3)

Description

[Tom Tung [:tt, :ttung]]

In Bug 1562667, we flip the pref for SAB on by default on Nightly except on Andriod. However, it causes jsreftests for Atomic to fail and the errors I got is:
tests/test262/shell.js:492: Error: Agents not available item 1

To reenable them as soon as possible, we might want to first check:
"if SpecialPowers is available in shared workers, when running these tests, can we easily change our $.agent implementation to use it?"

Comment 1

[André Bargull [:anba]]

The test262 Atomics tests aren't currently runnable in the browser, because the required test harness functionality is shell-specific, cf. https://searchfox.org/mozilla-central/source/js/src/tests/test262-host.js. I'd suggest to simply skip those tests when running in the browser.

Add to https://searchfox.org/mozilla-central/source/js/src/tests/jstests.list in the "Test262 tests disabled on browser" section:

skip-if(!xulRuntime.shell) include test262/built-ins/Atomics/notify/jstests.list
skip-if(!xulRuntime.shell) include test262/built-ins/Atomics/wait/jstests.list

Comment 2

[Tom Tung [:tt, :ttung]]

(In reply to André Bargull [:anba] from comment #1)

Thanks for the explanation!

I should have mentioned that I would probably disable them at https://phabricator.services.mozilla.com/D53966. That's why I file this bug as the follow-up bug.

The test262 Atomics tests aren't currently runnable in the browser, because the required test harness functionality is shell-specific, cf. https://searchfox.org/mozilla-central/source/js/src/tests/test262-host.js. I'd suggest to simply skip those tests when running in the browser.

Add to https://searchfox.org/mozilla-central/source/js/src/tests/jstests.list in the "Test262 tests disabled on browser" section:

skip-if(!xulRuntime.shell) include test262/built-ins/Atomics/notify/jstests.list
skip-if(!xulRuntime.shell) include test262/built-ins/Atomics/wait/jstests.list

The code I use for skipping the test is:
skip-if(!this.hasOwnProperty("Atomics")||!this.hasOwnProperty("getSharedArrayBuffer")) include test262/built-ins/Atomics/jstests.list
And that works on my local while running these tests. (I'm verifying if that also work on try or not now)

Please let me know if you find there are any potential issues. Thanks!

Comment 3

[André Bargull [:anba]]

(In reply to Tom Tung [:tt, :ttung] from comment #2)

The code I use for skipping the test is:
skip-if(!this.hasOwnProperty("Atomics")||!this.hasOwnProperty("getSharedArrayBuffer")) include test262/built-ins/Atomics/jstests.list
And that works on my local while running these tests. (I'm verifying if that also work on try or not now)

That will also work, because it disables all Atomics tests and not only the tests for Atomics.notify and Atomics.wait. !xulRuntime.shell is maybe a bit cleaner than !this.hasOwnProperty("getSharedArrayBuffer"), because not only getSharedArrayBuffer, but also a couple of other required shell functions are missing when running in the browser, like setSharedArrayBuffer and evalInWorker.

Comment 4

[Tom Tung [:tt, :ttung]]

(In reply to André Bargull [:anba] from comment #3)

(In reply to Tom Tung [:tt, :ttung] from comment #2)

The code I use for skipping the test is:
skip-if(!this.hasOwnProperty("Atomics")||!this.hasOwnProperty("getSharedArrayBuffer")) include test262/built-ins/Atomics/jstests.list
And that works on my local while running these tests. (I'm verifying if that also work on try or not now)

That will also work, because it disables all Atomics tests and not only the tests for Atomics.notify and Atomics.wait. !xulRuntime.shell is maybe a bit cleaner than !this.hasOwnProperty("getSharedArrayBuffer"), because not only getSharedArrayBuffer, but also a couple of other required shell functions are missing when running in the browser, like setSharedArrayBuffer and evalInWorker.

I see will change that to what you suggested (IIUC,

skip-if(!xulRuntime.shell) include test262/built-ins/Atomics/notify/jstests.list
skip-if(!xulRuntime.shell) include test262/built-ins/Atomics/wait/jstests.list

).
Thanks!

Comment 5

[Anne (:annevk)]

Note that we'd like to ship SharedArrayBuffer and atomics to Release in Fx 75 (without postMessage() support). Making this only block that rather than the overall effort of "resab", which includes postMessage() changes. Please let me know if Fixing this in the Fx 74 timeframe is unrealistic.

Comment 6

[Nicolas B. Pierron [:nbp]]

Stupid question, do we want special powers to have more control over shared memory?
It sounds to me that this could become a foot-gun for building attacks against Firefox.

Comment 7

[Anne (:annevk)]

As I understand it (from talking to Nihanth and Johann) special powers isn't packaged in release builds and for testing only so that shouldn't matter.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1598612 - Move SAB testing functions to make them visible to ref tests in the browser.

Comment 9

[Nicolas B. Pierron [:nbp]]

Note, the current patch is a work-in-progress, on which I do not have plan to dedicate more time for now.

This patch does not work in JS shell test cases as worker threads seems to be continuing their work beyond the closure of the top-level Runtime, which is to me unexpected, as I would have expected the worker threads to be closed before the closure of worker threads. This causes a seg-fault while running jit-test/tests/atomics/mutual-exclusion.js test case.

I have not even checked if it would work with ref-tests, but I would expect this is a step in the right direction to get this fixed.
Feel free to take over the current patch.

Comment 10

[Steve Fink [:sfink] [:s:]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)

This patch does not work in JS shell test cases as worker threads seems to be continuing their work beyond the closure of the top-level Runtime, which is to me unexpected, as I would have expected the worker threads to be closed before the closure of worker threads. This causes a seg-fault while running jit-test/tests/atomics/mutual-exclusion.js test case.

The problem was that all the runtimes did the finalization, instead of just the top-level one.

I have not even checked if it would work with ref-tests, but I would expect this is a step in the right direction to get this fixed.
Feel free to take over the current patch.

Yeah, that's going to take some more work. Currently, the tests depend on having evalInWorker available, which is not implemented for the browser. I don't know if there's an easy way to make it use real web workers. I'm kind of out of my depth, but I'll do some poking at it.

Comment 11

[Steve Fink [:sfink] [:s:]]

Even with the above patch, it looks like this would require some amount of work to get this running in the browser. (There's a scary note about blocking the main thread and things.) I haven't looked to see how hard it would be, but it seems like a potential time sink. I got distracted in trying to figure out a usable way to develop in-browser jsreftest code -- the edit/debug cycle is pretty long with needing to start up a browser, have it automatically install an extension, load in the jsreftest stuff.

So I'm wondering what the current priority of this is, given that these tests are running for the JS shell? Is this still trying to ship soon, and is getting the tests running in the browser a blocker?

(We could probably land this patch anytime, though.)

Comment 12

[Anne (:annevk)]

Thanks Steve! Lars should probably weigh in on importance.

Comment 13

[Lars T Hansen [:lth]]

Priority low IMO. The correct fix here is maybe bug 1414823 which might create a meaningful portable browser-compatible agent abstraction for the shell, and then we could rewrite the tests for that (though note three hypotheticals in that sentence). That bug's been stalled for two years and it's not happening this week either.

We have portable atomics tests in wpt that were originally derived from some of our shell tests, and that's probably adequate for now.

Comment 14

[Anne (:annevk)]

Okay, I regret not checking that in advance and letting Nicolas and Steve spin cycles on this. My apologies. No longer blocking bug 1606624 and will let the WebVM team decide on a new priority. Thanks everyone!