Implement API to enable SharedArrayBuffer and Atomics at run-time

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
This setting applies to each new global environment, since when it is enabled it exposes the SharedArrayBuffer and Atomic top-level names.  The default should always be off.  Changing the value will have no effect on existing global objects.

There's a slight chance that this setting will also have to affect run-time behavior; I think it won't but I don't know for sure (this is a topic in bug 1225039).  If so the setting's value will also need to be stored somewhere where it can be read and won't be affected by future changes to the config flag.
(Assignee)

Updated

3 years ago
Blocks: 1231337
(Assignee)

Comment 1

3 years ago
It will be helpful to keep the #ifdef ENABLE_SHARED_ARRAY_BUFFER for the time being, but to set that value unconditionally (or unconditionally for mainstream platforms) in moz.build.
Seems to me this should be a compartment option, along the lines done in attachment 8696133 [details] [diff] [review] for a different feature.
(Assignee)

Updated

3 years ago
Depends on: 1231624
(Assignee)

Comment 3

3 years ago
There are some additional issues to deal with here:

- there's a missing ifdef in jsapi-tests/testTypedArrays.cpp, it should only test the shared arrays if they are enabled in the build and currently they do not.  A fix is likely to land during the branching-to-aurora.

- there's questionable logic in class lookup, we should catch some API abuse that we're not catching at present but should be catching:

01:35 <Waldo> so we hit |if (!init && !clasp) return true; // JSProto_Null or a compile-time-disabled feature.| in GlobalObject::resolveConstructor
01:36 <Waldo> I wonder if that could be made an assertion, or at least if we should do it sometime not the day before funtimes
01:37 — lth|jetlag wonders how SIMD does this
01:39 <lth|jetlag> SIMD does not have jsapi-tests and may just not be vulnerable
01:53 <Waldo> lth|jetlag: it seems extraordinarily dumb to return true without doing anything, for faked-up prototype keys
01:53 <lth|jetlag> Waldo++
01:53 <Waldo> lth|jetlag: seems like once you've tried to init for a key that's bogus, there's no right answer
01:53 <Waldo> lth|jetlag: I think probably the right thing to do, is to have JS_NewSharedArrayBuffer include code saying No for the disabled case
01:54 <lth|jetlag> hm
01:54 <Waldo> such that we never enter the resolving/creation mechanism at all for it
01:55 <lth|jetlag> what you're suggesting is throwing an error?
01:55 <lth|jetlag> i'm probably in favor
02:04 <Waldo> lth|jetlag: either throw, or assert and thus treat it as API misuse
02:04 <Waldo> lth|jetlag: don't think it makes too much difference, unless we expect an extended period where Gecko and other embedders have to deal with the possibility of non-existence
02:05 <Waldo> in which case we'd have to expose an ENABLE_SHARED_ARRAY_BUFFER macro, or something, to allow embedders to feature-test before potential use
(Assignee)

Updated

3 years ago
Depends on: 1231338
(Assignee)

Updated

3 years ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Seems to me this should be a compartment option, along the lines done in
> attachment 8696133 [details] [diff] [review] for a different feature.

That seems reasonable to me too, but consider the following insanity.  grep for "setVersion(JSVERSION" in all the C++ files in the tree, since the version is set where the shared-memory-enabled attribute would also be set:

./dom/base/nsFrameMessageManager.cpp:1813:         .setVersion(JSVERSION_LATEST);
./dom/base/nsGlobalWindow.cpp:11797:           .setVersion(JSVERSION_DEFAULT);
./dom/events/EventListenerManager.cpp:973:         .setVersion(JSVERSION_DEFAULT)
./dom/jsurl/nsJSProtocolHandler.cpp:270:           .setVersion(JSVERSION_DEFAULT);
./dom/plugins/base/nsNPAPIPlugin.cpp:1420:         .setVersion(JSVERSION_DEFAULT);
./dom/workers/RuntimeService.cpp:1850:    sDefaultJSSettings.chrome.compartmentOptions.setVersion(JSVERSION_LATEST);
./dom/workers/ScriptLoader.cpp:1771:      options.setVersion(JSVERSION_LATEST);
./dom/xbl/nsXBLProtoImplField.cpp:427:         .setVersion(JSVERSION_LATEST);
./dom/xbl/nsXBLProtoImplMethod.cpp:196:         .setVersion(JSVERSION_LATEST);
./dom/xbl/nsXBLProtoImplProperty.cpp:202:             .setVersion(JSVERSION_LATEST);
./dom/xbl/nsXBLProtoImplProperty.cpp:248:             .setVersion(JSVERSION_LATEST);
./dom/xbl/nsXBLPrototypeHandler.cpp:376:         .setVersion(JSVERSION_LATEST);
./ipc/testshell/XPCShellEnvironment.cpp:518:           .setVersion(JSVERSION_LATEST);
./js/src/gdb/gdb-tests.cpp:77:    options.setVersion(JSVERSION_LATEST);
./js/src/jsapi-tests/testFreshGlobalEvalRedefinition.cpp:34:    options.setVersion(JSVERSION_LATEST);
./js/src/jsapi-tests/testGCFinalizeCallback.cpp:126:    options.setVersion(JSVERSION_LATEST);
./js/src/jsapi-tests/testPreserveJitCode.cpp:93:    options.setVersion(JSVERSION_LATEST);
./js/src/jsapi-tests/tests.cpp:82:    options.setVersion(JSVERSION_LATEST);
./js/src/jsapi-tests/testWeakMap.cpp:233:    options.setVersion(JSVERSION_LATEST);
./js/src/jsfun.cpp:773:           .setVersion(JSVERSION_DEFAULT);
./js/src/shell/js.cpp:546:    options.setVersion(JSVERSION_LATEST);
./js/src/shell/js.cpp:2606:    options.setVersion(JSVERSION_DEFAULT);
./js/src/vm/SelfHosting.cpp:1690:    options.setVersion(JSVERSION_LATEST);
./js/xpconnect/loader/mozJSComponentLoader.cpp:546:               .setVersion(JSVERSION_LATEST)
./js/xpconnect/loader/mozJSComponentLoader.cpp:721:               .setVersion(JSVERSION_LATEST)
./js/xpconnect/loader/mozJSSubScriptLoader.cpp:133:           .setVersion(JSVERSION_LATEST);
./js/xpconnect/src/XPCShellImpl.cpp:1482:               .setVersion(JSVERSION_LATEST);
./netwerk/base/ProxyAutoConfig.cpp:650:           .setVersion(JSVERSION_LATEST);
./xpcom/glue/tests/gtest/TestGCPostBarriers.cpp:96:  options.setVersion(JSVERSION_LATEST);

Each one of those is a location where we may, or may not, try to find a pref to enable Atomics and SharedArrayBuffer.  I'm not sure that's a good idea; something more centralized seems reasonable.  But if something more centralized existed, surely the setVersion mess would have been taken care of long ago?

(On the flip side, we don't necessarily want shared memory and atomics in eg SharedWorker and ServiceWorker, see bug 1232973, so maybe this is an opportunity to correct that, but I'm doubtful.)
(In reply to Lars T Hansen [:lth] from comment #4)
> grep for "setVersion(JSVERSION" in all the C++ files in the tree, since the
> version is set where the shared-memory-enabled attribute would also be set:
> 
> [snipped]
> 
> Each one of those is a location where we may, or may not, try to find a pref
> to enable Atomics and SharedArrayBuffer.  I'm not sure that's a good idea;
> something more centralized seems reasonable.

Your list applies to places/mechanisms that *run* script.  But SAB's availability is a function of globals/compartments.  Event-handler-running code, for example, has nothing to say about whether SAB is/isn't enabled.

That in mind, the set of spots potentially needing adjustment is all the users of JS_NewGlobalObject.  And there are only a handful of those outside of testing code.

> But if something more
> centralized existed, surely the setVersion mess would have been taken care
> of long ago?

Careful what you assume.  :-)  Options specific to individual globals/compartments are a somewhat new innovation, and it's unsurprising we haven't fully taken advantage of it where we should, yet.
(Assignee)

Comment 6

3 years ago
Attachment #8702488 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

3 years ago
Posted patch bug1231335-part2-asm.patch (obsolete) — Splinter Review
Attachment #8702489 - Flags: review?(luke)
(Assignee)

Comment 8

3 years ago
Attachment #8702490 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

3 years ago
Posted patch bug1231335-part4-shell.patch (obsolete) — Splinter Review
Attachment #8702491 - Flags: review?(jwalden+bmo)
Comment on attachment 8702489 [details] [diff] [review]
bug1231335-part2-asm.patch

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

::: js/src/asmjs/AsmJS.cpp
@@ +2196,5 @@
>          if (!global)
>              return false;
> +        if (!cx_->compartment()->options().getSharedMemoryAndAtomicsEnabled())
> +            return failOffset(parser_.tokenStream.currentToken().pos.begin,
> +                              "shared memory and atomics are disabled by javascript.options.shared_memory in about:config");

If getSharedMemoryAndAtomicsEnabled() controls whether Atomics et al are added to the global object and if dynamic linking is gated on the caller producing real Atomics then is this check necessary?
Attachment #8702489 - Flags: review?(luke) → review+
(Assignee)

Comment 11

3 years ago
(In reply to Luke Wagner [:luke] from comment #10)
> Comment on attachment 8702489 [details] [diff] [review]
> bug1231335-part2-asm.patch
> 
> Review of attachment 8702489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/asmjs/AsmJS.cpp
> @@ +2196,5 @@
> >          if (!global)
> >              return false;
> > +        if (!cx_->compartment()->options().getSharedMemoryAndAtomicsEnabled())
> > +            return failOffset(parser_.tokenStream.currentToken().pos.begin,
> > +                              "shared memory and atomics are disabled by javascript.options.shared_memory in about:config");
> 
> If getSharedMemoryAndAtomicsEnabled() controls whether Atomics et al are
> added to the global object and if dynamic linking is gated on the caller
> producing real Atomics then is this check necessary?

The linking has not previously been gated on that from what I know.  Is that a desirable check to make?

(Linking should currently check that the heap is a SharedArrayBuffer if any Atomics are imported, and that the heap is an ArrayBuffer if no Atomics are imported.)
What I mean is that, if an atomics is imported, then for linking to succeed, a real life Atomics function object must be supplied and I assume, if !getSharedMemoryAndAtomicsEnabled, there will be no way to get such function objects.
(Assignee)

Comment 13

3 years ago
Recording an IRC discussion, ValidateAtomicsBuiltinFunction() checks that if atomics are imported then the global provides the real atomics bindings.  Since the built-in Atomics object won't be on the global object, any such bindings won't be the real ones, and linking should fail.  Thus this patch is redundant.

It would be useful to have a test case to back that up.
Attachment #8702490 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 14

3 years ago
FWIW, it was easy to adapt these patches to the creationOptions / behaviorOptions split.
Comment on attachment 8702488 [details] [diff] [review]
bug1231335-part1-runtime-switch.patch

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

+ assuming I'm missing/forgetting something obvious about the transfer-a-SAB case, which I probably am.  :-)  If not, well, poking you on IRC now.

::: js/src/jsapi.h
@@ +2192,5 @@
>        , invisibleToDebugger_(false)
>        , mergeable_(false)
>        , discardSource_(false)
>        , disableLazyParsing_(false)
> +      , sharedMemoryAndAtomics_(false)

Move all this stuff into CompartmentCreationOptions, please (new on trunk).

@@ +2244,5 @@
>          return *this;
>      }
>  
> +    bool getSharedMemoryAndAtomicsEnabled() const;
> +    CompartmentOptions& setSharedMemoryAndAtomicsEnabled(bool flag);

And these.

::: js/src/jsfriendapi.h
@@ +1654,5 @@
>  
>  /**
> + * Create a new SharedArrayBuffer with the given byte length.  This
> + * may only be called if
> + * cx->compartment()->options().getSharedMemoryAndAtomicsEnabled() is

JS::CompartmentCreationOptionsRef(cx).get...(), as JSContext* is an opaque API.  (Friend API is a bit skeevy, but it *is* still outwardly targeted, to the extent it's targeted.)

::: js/src/vm/SharedArrayObject.cpp
@@ +274,5 @@
> +    // where the constructors are not available.
> +    //
> +    // Such cloning can only happen if the engine is compiled with
> +    // ENABLE_SHARED_ARRAY_BUFFER, so the necessary prototypes will be
> +    // available in this compartment.

So I'm cool with the rest of this patch, modulo tweaks.  But this comment confuses me.

If we have a global with shared/atomics enabled, that's fine.  If we then structured-clone a SAB from that global, into a global without shared/atomics enabled, in order to do so, we need to be able to create a SAB in that global that previously didn't have it.  That means we need to resolve SharedArrayBuffer, which means constructor and prototype are created, and the global property comes into existence.  In a compartment that wasn't told it could have this stuff.

So what am I missing?  Because, that doesn't seem acceptable.  For one, because the capability gets erroneously exposed.  For another, because if someone enumerated the global object they would have observed

@@ +450,5 @@
> +    //
> +    // API abuse will be encountered extremely rarely, so keep it safe
> +    // even in Release.
> +    if (!cx->compartment()->options().getSharedMemoryAndAtomicsEnabled()) {
> +        MOZ_ASSERT(false, "JS_NewSharedArrayBuffer is not available - shared memory is disabled");

Use

  MOZ_ASSERT_UNREACHABLE("atomics and shared memory must be enabled for the current global to create a SharedArrayBuffer");

rather than MOZ_ASSERT with false.

I would be inclined to make it *un*safe in release so that the mode of failure is louder than a seeming OOM/silent failure of a script to continue running, but eh.
Attachment #8702488 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8702491 [details] [diff] [review]
bug1231335-part4-shell.patch

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

::: js/src/shell/js.cpp
@@ +2601,5 @@
>      JS_GlobalObjectTraceHook
>  };
>  
> +static void
> +StandardCompartmentOptions(JS::CompartmentOptions& options)

Tack a "Set" or something on the front of this so it's clearer the method performs a mutation.

@@ +4320,5 @@
>          buf->addReference();
> +        // This is effectively a clone operation and its availability
> +        // does not depend on whether shared memory is enabled in the
> +        // current compartment or not: If the originator could produce
> +        // a SharedArrayBuffer, we will import it here.

This sounds similarly dubious as making SharedArrayBufferObject::New work inside structured cloning.  I would add a check at the top of this method, I think, and just have a super-early error if shared memory isn't enabled.

@@ +6421,5 @@
> +#ifdef ENABLE_SHARED_ARRAY_BUFFER
> +    disableSharedMemory = op.getBoolOption("no-shared-memory");
> +    enableSharedMemory = op.getBoolOption("shared-memory");
> +    if (disableSharedMemory && enableSharedMemory) {
> +        fprintf(stderr, "You can't use both --no-shared-memory and --shared-memory.\n");

Seems to me we should only have one on/off flag (defaulting off), a la --ion-licm=on/off and a host of other commandline flags, than two mutually incompatible flags with this sort of weird handling.
Attachment #8702491 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 17

3 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> Comment on attachment 8702488 [details] [diff] [review]
> bug1231335-part1-runtime-switch.patch
> 
> Review of attachment 8702488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> ::: js/src/vm/SharedArrayObject.cpp
> @@ +274,5 @@
> > +    // where the constructors are not available.
> > +    //
> > +    // Such cloning can only happen if the engine is compiled with
> > +    // ENABLE_SHARED_ARRAY_BUFFER, so the necessary prototypes will be
> > +    // available in this compartment.
> 
> If we have a global with shared/atomics enabled, that's fine.  If we then
> structured-clone a SAB from that global, into a global without
> shared/atomics enabled, in order to do so, we need to be able to create a
> SAB in that global that previously didn't have it.  That means we need to
> resolve SharedArrayBuffer, which means constructor and prototype are
> created, and the global property comes into existence.  In a compartment
> that wasn't told it could have this stuff.
> 
> So what am I missing?  Because, that doesn't seem acceptable.  For one,
> because the capability gets erroneously exposed.  For another, because if
> someone enumerated the global object they would have observed

Recording the outcome of an IRC discussion: this and other smartness will be removed in favor of errors.  In the case of structured cloning we will throw at the receiving / deserializing end if the compartment does not enable shared memory, since the sending end won't necessarily know and it's going to be an ultra-rare issue anyway (you have to explicitly construct test cases and manually jog the settings at the right time to encounter it).
(Assignee)

Comment 18

3 years ago
Re-review since substantially different.  This gets rid of the shared-memory-enabled check altogether and relies on the linker.  The bulk of this patch is now a rewritten gating test case.
Attachment #8702489 - Attachment is obsolete: true
Attachment #8704221 - Flags: review?(luke)
(Assignee)

Comment 19

3 years ago
Re-review since r-, with all requested changes.
Attachment #8702491 - Attachment is obsolete: true
Attachment #8704222 - Flags: review?(jwalden+bmo)

Updated

3 years ago
Attachment #8704221 - Flags: review?(luke) → review+
(Assignee)

Comment 20

3 years ago
Try runs are logged on bug 1231337.
Comment on attachment 8704222 [details] [diff] [review]
bug1231335-part4-shell.patch

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

::: js/src/shell/js.cpp
@@ +4331,5 @@
>      PR_Lock(sharedArrayBufferMailboxLock);
>      SharedArrayRawBuffer* buf = sharedArrayBufferMailbox;
>      if (buf) {
>          buf->addReference();
> +        // Shared memory is enabled globally in the shell, there can't be a worker

s/,/:/ to avoid a run-on sentence.  :-)
Attachment #8704222 - Flags: review?(jwalden+bmo) → review+
Depends on: 1239605
(Assignee)

Updated

3 years ago
See Also: → 1239666
You need to log in before you can comment on or make changes to this bug.