Closed Bug 1624266 Opened 4 years ago Closed 4 years ago

Conditionally hide the SharedArrayBuffer constructor

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: annevk, Assigned: tt)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 4 obsolete files)

Bug 1624266 - Allow the SharedArrayBuffer global constructor property to be optionally omitted from a new global object. r=arai!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Add code to page/worker/worklet code so that the global "SharedArrayBuffer" property can be trivially omitted from their global objects by changing how a single C++ variable for each case is initialized. r=baku!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Remove a debug fprintf that crept into previous changes.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Window;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Workers;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Worklets;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Update the related wpt test;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Update some mochitests for testing interfaces;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Uh, actually properly do not define (or pretend to the definition of) the global "SharedArrayBuffer" constructor in all potential probing circumstances, rather than do it halfway and incompetently. r=evilpie!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1624266 - Disable the js reftests that fail because the SharedArrayBuffer's CTOR is not exposed;
47 bytes, text/x-phabricator-request
Details | Review

V8 has found that it's not web-compatible (at least for them, we haven't had a compatibility bug reported by Nightly/early Beta users) to expose the SharedArrayBuffer constructor when postMessage() throws. This is discussed in https://github.com/whatwg/html/issues/4732. Based on that discussion we've decided to hide (delete if you will) the constructor when cross-origin isolated is false.

Web developers can still get at the constructor through new WebAssembly.Memory({ shared:true, initial:0, maximum:0 }).buffer.constructor. The motivation here is web compatibility, not security.

https://github.com/web-platform-tests/wpt/issues/22358 links four PRs that update all tests in web-platform-tests that are impacted by this change. (Including a test to ensure globalThis.SharedArrayBuffer is indeed undefined across window and worker environments.)

In https://github.com/whatwg/html/pull/4734 commit https://github.com/whatwg/html/pull/4734/commits/b1ad28254eaed82c2342449198f3b38034abe4d3 I tried to spell out what this means standards-wise.

I don't have a concrete plan for how to do this code-wise. I think it would make the most sense that when javascript.options.shared_memory is true, the constructor is not there, but can be restored by dom.postMessage.sharedArrayBuffer.withCOOP_COEP. That way we can ship javascript.options.shared_memory to release in bug 1606624 while we sort out the remaining issues with COOP and COEP.

Also, while it needs to be hidden from all web-exposed globals, i.e., Window, worker types, and worklet types, we should probably keep it exposed to chrome and jsshell.

Tom, would you feel comfortable taking this or should we request the WebVM team to provide the necessary infrastructure?

Flags: needinfo?(ttung)

(In reply to Anne (:annevk) from comment #0)

Also, while it needs to be hidden from all web-exposed globals, i.e., Window, worker types, and worklet types, we should probably keep it exposed to chrome and jsshell.

Tom, would you feel comfortable taking this or should we request the WebVM team to provide the necessary infrastructure?

I guess I can take this. I will need to:

One question here is that:
Should we provide an ability to disable this by dom_postMessage_sharedArrayBuffer_bypassCOOP_COEP_insecure_enabled as well (or another pref)?
We introduced that pref for developers so that they can develop their stuff without considering COOP and COEP headers. I wonder if doing that again makes sense or not.

Assignee: nobody → ttung
Flags: needinfo?(ttung) → needinfo?(annevk)

Letting that override this makes sense to me. Thanks for thinking of that!

Flags: needinfo?(annevk)

[marking as P1, because the bug is assigned]

Priority: -- → P1
Status: NEW → ASSIGNED

Hi Lars,

I tried to propagate crossOriginIsolated (more clear IsSharedMemoryAllowed) to RealmCreationOptions right before createGlobal(https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom12CreateGlobalEP9JSContextPT_P14nsWrapperCachePK7JSClassRN2JS12RealmOptionsEP12JSPrincipalsbNSA_13MutableHandleIP8JSObjectEE&redirect=false) as a new added attribute. Also, check the attribute in option in skipDeselectedConstructor so that the SharedArrayBuffer won't be able to access when the crossOriginIsolated is false. (e.g. globalThis.SharedArrayBuffer is undefined)

However, creating a buffer from a wasm memory (e.g. new WebAssembly.Memory({ shared:true, initial:1, maximum:1 }).buffer) would also fail. IIUC, it's because allocating that buffer goes to the code path for getting a SharedArrayBufferObject.

To avoid this, I'm thinking of creating another JSProto type to differentiate it from pure SharedArrayBufferObject in https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/js/public/ProtoKey.h#93. What do you think? Or do you have any suggestions to avoid this in some other ways? Thanks!

Flags: needinfo?(lhansen)

The idea of creating a new JSProto for this makes me very, very nervous so I think that should be a last resort. Surely there must be a better way. Can you attach your WIP patch and I can take a look when I have more context?

Flags: needinfo?(lhansen)

Maybe it's possible to use something similar to SkipUneval to ensure enumeration and resolve hooks on the global skip SharedArrayBuffer? That will require some changes to intrinsic_GetBuiltinConstructor, though, because in that caller JS_IdToProtoKey (which also calls SkipUneval) must not skip JSProto_SharedArrayBuffer.

Attached file Bug 1624266 - P2 - windows; (obsolete) —

Depends on D69164

(In reply to Lars T Hansen [:lth] from comment #5)

The idea of creating a new JSProto for this makes me very, very nervous so I think that should be a last resort. Surely there must be a better way. Can you attach your WIP patch and I can take a look when I have more context?

I don't have enough understanding on JS engine so that was just an arbitrary asking :)

I updated a part of them to the bug. (I also have changes on workers and worklet, but I think wip patches here are enough for providing context)

After patches here, window.SharedArrayBuffer is undefined when window.crossOriginIsolated is false. However, they would cause new WebAssembly.Memory({ shared:true, initial:1, maximum:1 }).buffer to fail as well.

From my understanding, these patches propagate the crossOriginIsolated to the RealmCreationOption. The remaining work is to let the JS engine decide when should it hide the CTOR of SAB.

Please let me know if you want me to provide more detailed. Thanks!

Flags: needinfo?(lhansen)

(I will check comment 6 tomorrow)

Steven, can you find some JS engine person to look into this further? I'm swamped and will not get to this for a while (after Easter more likely than not), and it's not really a wasm matter anyway. Happy to help review anything when it's finished.

Flags: needinfo?(lhansen) → needinfo?(sdetar)

Ted, do you know who on our team could help with this?

Flags: needinfo?(sdetar) → needinfo?(tcampbell)

waldo, can you take ownership of this bug and work on it as a priority?

Flags: needinfo?(tcampbell) → needinfo?(jwalden)

So the above rev is hypothetically a patch for this. If I test the JS shell, dbg/dist/bin/js --shared-memory=on, I get:

[jwalden@find-waldo-now after]$ dbg/dist/bin/js --shared-memory=on
js> typeof newGlobal().SharedArrayBuffer
"undefined"
js> typeof newGlobal({enableCoopAndCoep: true}).SharedArrayBuffer
"function"

But the browser doesn't actually work with this -- if shared memory is preffed on (as it is in nightly), the constructor is always present.

This appears to be because cx->realm()->creationOptions().getCoopAndCoepEnabled() depends not upon the dom.postMessage.sharedArrayBuffer.withCOOP_COEP preference alleged in comment 0, but instead upon browser.tabs.remote.useCrossOriginOpenerPolicy and browser.tabs.remote.useCrossOriginEmbedderPolicy.

So, what's supposed to be the story for this? Why are there three separate preferences for this? What do I actually want to be gating on? If this really demands Yet Another RealmOptions option, I am going to need serious explanation of the exact precise differences between these things, to attempt to cogently document such distinction when adding it.

Flags: needinfo?(jwalden)

Hey Jeff, sorry that things were somewhat unclear. I had forgotten that CrossOriginIsolated is already all over our code and can be used. That is what we want to gate this on.

In particular, that means that by default globalThis.SharedArrayBuffer is undefined, but once the COOP and COEP headers are set appropriately, it will not be.

(In other words, all of Tom Tung's comments are on point.)

Attachment #9140680 - Attachment description: Bug 1624266 - When JS shared-memory support is enabled, only define the global SharedArrayBuffer constructor property if COOP and COEP support is enabled. → Bug 1624266 - When JS shared-memory support is enabled, only define the global SharedArrayBuffer constructor property if COOP and COEP support is enabled and they request cross-origin isolation. r=arai, r=smaug, r=baku

Okay, attachment 9140680 [details] seems to work as far as the test in attachment 9137380 [details] goes. I've added changes to make workers deal with this correctly, too (as required to make that test pass in all its flavors).

However, the patch isn't quite yet a complete fix, because some uncertain number of existing web-platform-tests presume a global SharedArrayBuffer property. Some of those maybe want to use COOP/COEP; others may want to perform the get-constructor incantation; some (I'm looking at test_worker_interfaces.html and test_worker_interfaces_secureContext.html at minimum) will want to be duplicated for the cross-origin-isolated case.

But this patch seems like it ought cover all the parts that are in my JS-engine wheelhouse, and either the test fixes I can deal with in a second patch (to be folded into the first for landing of the combined change), or someone else can pick up the baton. Either way's fine with me, just let me know what makes the most sense.

Here's a try run that should ultimately point out all the tests that will need updating, different preferences set, etc. It's far from clean. But it does seem to be enough to indicate the patch checks out, as far as it was intended to check out.

The /html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/ WPT tests that are failing have COOP/COEP set though (see their corresponding .headers resource).

Waldo is actively working on this bug, not me, so change the assignee. :)

Assignee: ttung → jwalden
Attachment #9140680 - Attachment description: Bug 1624266 - When JS shared-memory support is enabled, only define the global SharedArrayBuffer constructor property if COOP and COEP support is enabled and they request cross-origin isolation. r=arai, r=smaug, r=baku → Bug 1624266 - Allow the SharedArrayBuffer global constructor property to be optionally omitted from a new global object. r=arai
Attachment #9141143 - Attachment description: Bug 1624266 - Add code to page/worker code so that the global "SharedArrayBuffer" property can be trivially omitted from page/worker global objects by changing how two C++ variables are initialized. r=baku! → Bug 1624266 - Add code to page/worker/worklet code so that the global "SharedArrayBuffer" property can be trivially omitted from their global objects by changing how a single C++ variable for each case is initialized. r=baku!
Attachment #9140680 - Attachment description: Bug 1624266 - Allow the SharedArrayBuffer global constructor property to be optionally omitted from a new global object. r=arai → Bug 1624266 - Allow the SharedArrayBuffer global constructor property to be optionally omitted from a new global object. r=arai!
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/7258f2cac07e
Allow the SharedArrayBuffer global constructor property to be optionally omitted from a new global object.  r=arai,baku
https://hg.mozilla.org/integration/autoland/rev/e1d33e7f9cae
Add code to page/worker/worklet code so that the global "SharedArrayBuffer" property can be trivially omitted from their global objects by changing how a single C++ variable for each case is initialized.  r=baku

Okay, basic setup patches queued for landing -- leaving open to actually figure out how to feed COOP/COEP data into the boolean variables just added (or at least just the normal-page one, I filed bugs for the worklet cases as that's not shipping code yet)...

FWIW, I noted in passing the reuse-global code in nsGlobalOuterWindow.cpp. I'm not sure if I ever understood precisely how window reuse works. But this defineSharedArrayBufferConstructor() boolean is burned into the global object at its creation. At least conceivably, that could be a concern for reuse -- although given how many other bits of state can get reused, I'm assuming there's not actually a concern here and competent people know what they're doing with with that...

Keywords: leave-open
Assignee: jwalden → ttung
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/c842217ce33a
Remove a debug fprintf that crept into previous changes.
See Also: → 1630877
Attachment #9137380 - Attachment is obsolete: true
Attachment #9137381 - Attachment is obsolete: true
Attachment #9137382 - Attachment is obsolete: true

Depends on D71536

If I apply a patch like:

diff --git a/dom/base/nsGlobalWindowOuter.cpp b/dom/base/nsGlobalWindowOuter.cpp
--- a/dom/base/nsGlobalWindowOuter.cpp
+++ b/dom/base/nsGlobalWindowOuter.cpp
@@ -2225,17 +2225,17 @@ nsresult nsGlobalWindowOuter::SetNewDocu
       //
       // We set this value to |true| to replicate pre-existing behavior.  In the
       // future, bug 1624266 will assign the correct COOP/COEP-respecting value
       // here.  When that change is made, corresponding code for workers in
       // WorkerPrivate.cpp must also be updated.  (Ideally both paint and audio
       // worklets -- bug 1630876 and bug 1630877 -- would be fixed at the same
       // time, but fixing them has lower priorit because they're not shipping
       // yet.)
-      bool aDefineSharedArrayBufferConstructor = true;
+      bool aDefineSharedArrayBufferConstructor = false;

       // Every script context we are initialized with must create a
       // new global.
       rv = CreateNativeGlobalForInner(
           cx, newInnerWindow, aDocument->GetDocumentURI(),
           aDocument->NodePrincipal(), &newInnerGlobal,
           ComputeIsSecureContext(aDocument),
           aDefineSharedArrayBufferConstructor);
diff --git a/dom/tests/mochitest/general/test_interfaces.js b/dom/tests/mochitest/general/test_interfaces.js
--- a/dom/tests/mochitest/general/test_interfaces.js
+++ b/dom/tests/mochitest/general/test_interfaces.js
@@ -85,16 +85,17 @@ var ecmaGlobals = [
   { name: "ReferenceError", insecureContext: true },
   { name: "Reflect", insecureContext: true },
   { name: "RegExp", insecureContext: true },
   { name: "Set", insecureContext: true },
   {
     name: "SharedArrayBuffer",
     insecureContext: true,
     earlyBetaOrEarlier: true,
+    disabled: true,
   },
   { name: "String", insecureContext: true },
   { name: "Symbol", insecureContext: true },
   { name: "SyntaxError", insecureContext: true },
   { name: "TypedObject", insecureContext: true, nightly: true },
   { name: "TypeError", insecureContext: true },
   { name: "Uint16Array", insecureContext: true },
   { name: "Uint32Array", insecureContext: true },

to current m-c, dom/tests/mochitest/general/test_interfaces.html would fail. And I got message:

dom/tests/mochitest/general/test_interfaces.html
  FAIL If this is failing: DANGER, are you sure you want to expose the new interface SharedArrayBuffer to all webpages as a property on the window? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals)
    SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
    runTest@dom/tests/mochitest/general/test_interfaces.js:1476:7
    @dom/tests/mochitest/general/test_interfaces.js:1515:1
  FAIL SharedArrayBuffer is exposed as an own property on the window but tests false for "in" in the global scope
    SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
    runTest@dom/tests/mochitest/general/test_interfaces.js:1484:7
    @dom/tests/mochitest/general/test_interfaces.js:1515:1
  FAIL SharedArrayBuffer is exposed as an own property on the window but has no property descriptor in the global scope
    SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
    runTest@dom/tests/mochitest/general/test_interfaces.js:1488:7
    @dom/tests/mochitest/general/test_interfaces.js:1515:1

That means it failed at here. That probably indicates that D70990 doesn't stop Object.getOwnPropertyNames(window) from getting SharedArrayBuffer if the aDefineSharedArrayBufferConstructor is false in CreateNativeGlobalForInner.

I assume that disabling the constructor of SharedArrayBuffer if ${global}.crossOriginIsolated is false implies that SharedArrayBuffer shouldn't be exposed as a property to window when ${global}.crossOriginIsolated is false. (Anne, please correct me if I am wrong here).

I guess we might need another patch for preventing getting the property of SharedArrayBuffer for window when creationOptions.setDefineSharedArrayBufferConstructor(false).

Jeff, would you mind taking a short look at this? Thanks in advance!

Flags: needinfo?(jwalden)

(In reply to Tom Tung [:tt, :ttung] from comment #35)
To elaborate more, the same thing (being able to get SharedArrayBuffer from Object.getOwnPropertyNames(worker)) doesn't happen on worker because I didn't get the same issue on dom/workers/test/test_worker_interfaces.html

Yeah, the global object's SharedArrayBuffer own property has to vary on cross-origin isolated, that's all this bug is really about. Perhaps the constructor talk was a bit confusing, but we typically call those specific properties constructors as well in web platform land.

Ideally we turn test_interfaces.js into a WPT as well so if other browsers get these details wrong they'll notice.

Attachment #9141655 - Attachment description: Bug 1624266 - Compute whether should the CTOR SharedArrayBuffer be defined for Workers; → Bug 1624266 - Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Workers;
Attachment #9141656 - Attachment description: Bug 1624266 - Compute whether should the CTOR SharedArrayBuffer be defined for Worklets; → Bug 1624266 - Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Worklets;
Attachment #9141657 - Attachment description: Bug 1624266 - Update the wpt test; → Bug 1624266 - Update the related wpt test;

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

If I apply a patch like:
[omitted]
to current m-c, dom/tests/mochitest/general/test_interfaces.html would fail. And I got message:

dom/tests/mochitest/general/test_interfaces.html
  FAIL If this is failing: DANGER, are you sure you want to expose the new interface SharedArrayBuffer to all webpages as a property on the window? Do not make a change to this file without a  review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals)
    SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
    runTest@dom/tests/mochitest/general/test_interfaces.js:1476:7
    @dom/tests/mochitest/general/test_interfaces.js:1515:1
  FAIL SharedArrayBuffer is exposed as an own property on the window but tests false for "in" in the global scope
    SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
    runTest@dom/tests/mochitest/general/test_interfaces.js:1484:7
    @dom/tests/mochitest/general/test_interfaces.js:1515:1
  FAIL SharedArrayBuffer is exposed as an own property on the window but has no property descriptor in the global scope
    SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
    runTest@dom/tests/mochitest/general/test_interfaces.js:1488:7
    @dom/tests/mochitest/general/test_interfaces.js:1515:1

Bah, I am a failure, I knew/remembered this was more complicated than it seemed from watching the global/globalThis patching. I have only tested this patch in the shell somewhat, but I think it may do the trick for the browser -- please test, and if it's not enough, cancel the review request I have optimistically added to it by someone who actually is competent in this code. :-)

Flags: needinfo?(jwalden)
Blocks: 1631808
Attachment #9141654 - Attachment description: Bug 1624266 - Compute whether should the CTOR SharedArrayBuffer be defined for Window; → Bug 1624266 - Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Window;

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=625ee1314f45db9f703fa0a15fdc10785b860478

There are some js ref test failures and hopefully, they are the last issue that needs to be resolved in this bug.
For instance:

REFTEST TEST-UNEXPECTED-FAIL | js/src/tests/test262/built-ins/ArrayBuffer/prototype/byteLength/this-is-sharedarraybuffer.js | Unknown file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/test262/shell.js:414: uncaught exception: Test262Error: `this` cannot be a SharedArrayBuffer Expected a TypeError but got a ReferenceError item 1

However, I cannot reproduce that on my local machine. What I got is:

python js/src/tests/jstests.py  objdir/dist/bin/js test262/built-ins/ArrayBuffer/prototype/byteLength/this-is-sharedarraybuffer.js
[1|0|0|0] 100% ==========================================================>|   1.0s
PASS

Anyway, it looks like that's because the constructor of SharedArrayBuffer isn't exposed to the js shell when it's not CrossOriginIsolated? (D70990 looks like the constructor of SharedArrayBuffer is exposed by default though)
I assumed what Window/Worker/Worklet pass into the creationOption shouldn't affect these tests, so I guess my patches shouldn't be related to.

Jeff, sorry for nagging you again, but maybe you have more ideas about what's going on inside?

Note that the D71730 on my try push is the initial version, but I guess that doesn't matter with these test failures. (I found them before D71730)

Flags: needinfo?(jwalden)

The JS shell always exposes SharedArrayBuffer, unless it's in a newGlobal() global that has the constructor turned off. This is distinct from in web pages, that only get it if cross-origin-isolated.

Most of test262 probably assumes always-exposure.

If you run jstests in the shell -- as you are doing locally -- the constructor's going to always be there. But on treeherder, the R(J#) jobs are running jstests in the browser. So those test262 tests are going to be relying on a constructor that won't always be there. (And the harness might even mean it's never there. I'm not sure.)

Functionally, this is an inadequacy of the jsreftest harness. I'll do some investigation, and/or find people who know things.

So stepping back for a second...

Have you actually gotten TC39 on board with this proposal to have some realms contain a global SharedArrayBuffer property and some not? If you have, then probably existing test262 tests need to be annotated to indicate opt-in, opt-out to the provision of the constructor. (There's an annotation to indicate a test requires SharedArrayBuffer functionality, but that's not quite the same as saying the global simply isn't defined.)

Right now we load all JS tests in essentially identical fashion. But if we want some tests to be cross-site-isolated, we're going to have to load them a different way, on a per-test basis. Moreover, from what I can tell jsreftest currently assumes all tests are loaded from the same origin (or possibly same origin but HTTP/HTTPS both, not sure) -- and there's even a jsreftest flag to say "run tests against this remote URL" which is inherently not amenable to this.

This seems to exist in possible tension with the historical 1JS idea, of JS being JS wherever it is. And if this change requires mucking with tests and test harnesses both, that seems 1JS-adjacent-enough that we should be getting TC39 buy-in before we run too hard down this road, including having to do harness work that runs counter to the current test idiom.

Flags: needinfo?(jwalden)

(In reply to Jeff Walden [:Waldo] from comment #43)

If you run jstests in the shell -- as you are doing locally -- the constructor's going to always be there. But on treeherder, the R(J#) jobs are running jstests in the browser. So those test262 tests are going to be relying on a constructor that won't always be there. (And the harness might even mean it's never there. I'm not sure.)

I see. That explains why I couldn't be able to reproduce test failures on my machine.

Functionally, this is an inadequacy of the jsreftest harness. I'll do some investigation, and/or find people who know things.

Thanks!

(In reply to Jeff Walden [:Waldo] from comment #44)

...
Have you actually gotten TC39 on board with this proposal to have some realms contain a global SharedArrayBuffer property and some not? If you have, then probably existing test262 tests need to be annotated to indicate opt-in, opt-out to the provision of the constructor. (There's an annotation to indicate a test requires SharedArrayBuffer functionality, but that's not quite the same as saying the global simply isn't defined.)

I'm not sure if we haven't proposed this on TC39 or not. I guess we haven't. Redirect this question to Anne since he knows more about spec.

Right now we load all JS tests in essentially identical fashion. But if we want some tests to be cross-site-isolated, we're going to have to load them a different way, on a per-test basis. Moreover, from what I can tell jsreftest currently assumes all tests are loaded from the same origin (or possibly same origin but HTTP/HTTPS both, not sure) -- and there's even a jsreftest flag to say "run tests against this remote URL" which is inherently not amenable to this.

This seems to exist in possible tension with the historical 1JS idea, of JS being JS wherever it is. And if this change requires mucking with tests and test harnesses both, that seems 1JS-adjacent-enough that we should be getting TC39 buy-in before we run too hard down this road, including having to do harness work that runs counter to the current test idiom.

If that requires a non-small change on the harness, I wouldn't suggest doing that since gating SharedArrayBuffer on the constructor if it's not crossOriginIsoalted is a short-term solution that proposed by google folks.

A naive question to this (I don't have much experience in JS engine world so the question might be stupid), we have had javascript.options.shared_memory to decide whether expose both SharedArrayBuffer and Atomic or not. [It's only on by default on early Beta or Early] (https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/modules/libpref/init/all.js#1213) and we plan to expose these two on Release in bug 1606624. So, I suppose there should have something similar to this already. (I meant that mucking with tests and test harnesses if SharedArrayBuffer is not exposed). Can we somehow utilize that?

Flags: needinfo?(annevk)

Jeff, see https://github.com/tc39/ecma262/issues/1435 and https://github.com/tc39/ecma262/pull/1903. TC39's idea seem to be to leave it entirely up to hosts/implementations. I asked for a clearer contract, but "normative-optional" seems to be it for now. There's a thing similar to this with weak references, cleanupSome will likely not be available in all agents. And agents also have [[CanBlock]] which differs depending on the agent's characteristics. So to some extent 1JS already requires zooming out a bit. But yeah, TC39 is definitely aware and one of the editors (Shu, on behalf of V8) proposed this whole thing to begin with.

Flags: needinfo?(annevk)

Have you actually gotten TC39 on board with this proposal to have some realms contain a global SharedArrayBuffer property and some not?

TC39 is definitely aware, as Anne said. We got consensus for it in the March 2020 meeting.

I've understood 1JS historically to be more about going against modes within JS itself, not that the JS host environment capabilities are identical. As we started speccing closer-to-the-metal features like SABs and now WeakRefs, I think the interpretation of 1JS being "JS host environments must provide all things specified in ecma262" is just gone. The high-order bit is how the web deals with the normative optionality, and as Anne said, you already have to zoom out a bit to see "1JS".

I think giving hosts normative optionality is probably the right thing for the web, so the web platform (and other hosts) can constrain features at the cost of developer to serve users better. [[CanBlock]] is the most obvious candidate here, but the general "make it hard to do expensive sync work on the main thread" is a pretty prevalent theme and may even affect syntactic features like top-level await in ServiceWorkers. The danger that made TC39 come up with the 1JS idea is still relevant though, and the web platform should take care to not introduce too many such "profiles" of JS features.

Edit: For the web, pragmatically, I should say even if TC39 held fast to its 1JS motto in your interpretation, it doesn't make too much of a difference for the web developer. What's "core JS language" and what's a web API is a line that only clearly exists for those of us involved in web standards. The web devs really do not know at large, judging by the studies I've seen, nor is it important for them to know. So, even if the core language was exactly the same in all hosts, the capabilities of different environments within the web (main thread, worker thread, service workers, COOP+COEP or not) are already different, and devs already treat them differently.

Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/e9e01656bfea
Uh, actually properly do not define (or pretend to the definition of) the global "SharedArrayBuffer" constructor in all potential probing circumstances, rather than do it halfway and incompetently.  r=evilpie

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

A naive question to this (I don't have much experience in JS engine world so the question might be stupid), we have had javascript.options.shared_memory to decide whether expose both SharedArrayBuffer and Atomic or not. [It's only on by default on early Beta or Early] (https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/modules/libpref/init/all.js#1213) and we plan to expose these two on Release in bug 1606624. So, I suppose there should have something similar to this already. (I meant that mucking with tests and test harnesses if SharedArrayBuffer is not exposed). Can we somehow utilize that?

So, we have had thing like: |reftest| skip-if(!this.hasOwnProperty('SharedArrayBuffer')) -- SharedArrayBuffer is not enabled unconditionally in current ref test in JS. I will check if I can figure out why this doesn't work today.

Blocks: 1630877

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

So, we have had thing like: |reftest| skip-if(!this.hasOwnProperty('SharedArrayBuffer')) -- SharedArrayBuffer is not enabled unconditionally in current ref test in JS. I will check if I can figure out why this doesn't work today.

I push all patches that haven't been landed yet on the top of m-c with printing information to try and here is the result .
I quote what I printed in the log:

[task 2020-04-27T08:17:32.697Z] 08:17:32     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/jsreftest.html?test=test262/built-ins/ArrayBuffer/prototype/byteLength/this-is-sharedarraybuffer.js | 6496 / 12215 (53%)
[task 2020-04-27T08:17:32.733Z] 08:17:32     INFO - [TT] false
[task 2020-04-27T08:17:32.733Z] 08:17:32     INFO - [TT] [object Window]
[task 2020-04-27T08:17:32.733Z] 08:17:32     INFO - TEST-INFO |  FAILED!  uncaught exception: Test262Error: `this` cannot be a SharedArrayBuffer Expected a TypeError but got a ReferenceError

So, this.hasOwnProperty('SharedArrayBuffer') is false while running the test but the test wasn't skipped. Which against the annoation .

That probably means there is an issue around test harness for test262. (e.g. the skip-if stuff doesn't work as expected?)

Jeff, sorry for nagging you again, but maybe you know something about it, or please redirect the needinfo to someone that is familiar with that. Thanks!

I guess we don't have to run the jsreftest in COOP+COEP environment at this moment, but we can fix the issue for skip-if and thus skip these tests to land all existing patches?

Flags: needinfo?(jwalden)

Test262 tests are run through the standard reftest framework (https://searchfox.org/mozilla-central/source/layout/tools/reftest). I think this line is responsible for evaluating "skip-if" annotations. Maybe someone with more knowledge about the reftest runner can chime in.

I suspect the issue could be related to the this is different when this.hasOwnProperty('SharedArrayBuffer') is evaluated. I wonder if this is the js shell at the first access and the second access is window so that the test was skipped while evaluating the "skip-if". However, it needs someone who knows about the code to verify.

Yes, the two thises are very different. The manifest is parsed entirely ahead of time, and conditions are checked entirely ahead of time -- outside of the page that would hypothetically be used to run the test.

The right way to simply skip individual tests in the browser is to add them to js/src/tests/jstests.list as skip-if(!xulRuntime.shell) items, with a comment by them pointing to this bug. That quick stopgap shouldn't demand additional input from me at this exact moment.

Flags: needinfo?(jwalden)

I will land D71534 D71535 D71536 D71537 D71573 D73260 D73428 once D73428 is r+

Attachment #9144983 - Attachment is obsolete: true
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37458d545c57
Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Window; r=nika
https://hg.mozilla.org/integration/autoland/rev/c34a7bef5ebe
Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Workers; r=baku
https://hg.mozilla.org/integration/autoland/rev/b6f67798f384
Use IsSharedMemoryAllowed to decide whether should the CTOR SharedArrayBuffer be defined for Worklets; r=baku
https://hg.mozilla.org/integration/autoland/rev/581f1654554f
Update the related wpt test; r=annevk
https://hg.mozilla.org/integration/autoland/rev/a3fc233f03b1
Update some mochitests for testing interfaces; r=smaug
https://hg.mozilla.org/integration/autoland/rev/0dd92d74c63a
Disable the js reftests that fail because the SharedArrayBuffer's CTOR is not exposed; r=jwalden
Blocks: 1635176

I'm closing this bug because:

  • The implementation for "Conditionally hide the SharedArrayBuffer constructor" is landed
  • This bug blocks bug 1606624
  • The remaining work is tracked in bug 1635176

Please feel free to reopen this

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Duplicate of this bug: 1630876
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: