Bug 212069 – Re-enable SharedArrayBuffer for JSC shell and Testers

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Sergey Rubanov]

SharedArrayBuffer was disabled in https://bugs.webkit.org/show_bug.cgi?id=181266 due to Spectre and Meltdown vulnarabilities.

I haven't found tracking bug for re-enabling SAB again, so I created this one.

Chrome re-enabled SAB in v67 https://bugs.chromium.org/p/chromium/issues/detail?id=821270
Firefox re-enabled SAB in v78 https://bugzilla.mozilla.org/show_bug.cgi?id=1606624

[Jarred Sumner]

As someone building a game, I would love to see Safari/WebKit re-enable SharedArrayBuffer. Sharing data between web workers and the main thread without SharedArrayBuffer is much slower, even though Safari is faster at a lot of things than other browsers.

[Yusuke Suzuki]

*** Bug 197191 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

*** Bug 185038 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 413257 [details]
Patch

[Anne van Kesteren]

Yusuke, I might be missing something but it doesn't seem like this patch guards it with cross-origin isolated (as defined in the HTML Standard). That's somewhat important if you want to enable this in a safe manner.

[Simon Pieters (:zcorpan)]

Cross-origin isolated is https://bugs.webkit.org/show_bug.cgi?id=215407 - as Anne said, I believe it should be blocking this bug.

[Yusuke Suzuki]

Created attachment 413275 [details]
Patch

[Yusuke Suzuki]

(In reply to Anne van Kesteren from comment #5)
> Yusuke, I might be missing something but it doesn't seem like this patch
> guards it with cross-origin isolated (as defined in the HTML Standard).
> That's somewhat important if you want to enable this in a safe manner.

(In reply to Simon Pieters from comment #6)
> Cross-origin isolated is https://bugs.webkit.org/show_bug.cgi?id=215407 - as
> Anne said, I believe it should be blocking this bug.

We will implement it, and for SharedArrayBuffer / Atomics, we will enable it for WebKitTestRunner etc.
Once cross-origin isolation is done, we just flip runtime flag to expose.

[Yusuke Suzuki]

And possibly we will enable it for JavaScriptCore.framework.

[Yusuke Suzuki]

Created attachment 413388 [details]
Patch

[Yusuke Suzuki]

Created attachment 413398 [details]
Patch

[Yusuke Suzuki]

Created attachment 413408 [details]
Patch

[Anne van Kesteren]

Thanks for confirming Yusuke, exciting!

[Keith Miller]

Comment on attachment 413408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413408&action=review

r=me with some nits, if you fix the style issues.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2725
> +
> +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadIndexingType)
> +                || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadConstantCache)
> +                || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)
> +                || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadType))
> +                return false;

Is there a reason to not include all exits (other than Uncountable and friends) here?

> Source/JavaScriptCore/jsc.cpp:3102
> +    fprintf(stderr, "  --can-block-is-false       Make main thread's [[CanBlock]] false\n");

Nit: Maybe phrase this as "Make main thread's Atomics.wait throw"? Can block may be a little obscure and I'm sure I'll forget in the future...

> Tools/Scripts/test262/Runner.pm:708
> +    if (grep $_ eq 'CanBlockIsFalse', @{$data->{flags}}) {
> +        $featureFlags .= ' --can-block-is-false';
> +    }

Yikes, that seems expensive to do on every file...

[Yusuke Suzuki]

Comment on attachment 413408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413408&action=review

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2725
>> +                return false;
> 
> Is there a reason to not include all exits (other than Uncountable and friends) here?

Atomic operation is GetByVal from TypedArray + read-modify-write operation. And it also has Int32 Type checks for index argument.
And we also need FixupPhase's checkArray thing to ensure that input is requested TypedArray.
Those checks may emit these exits. And I need to include OutOfBounds too here. Changed.

>> Source/JavaScriptCore/jsc.cpp:3102
>> +    fprintf(stderr, "  --can-block-is-false       Make main thread's [[CanBlock]] false\n");
> 
> Nit: Maybe phrase this as "Make main thread's Atomics.wait throw"? Can block may be a little obscure and I'm sure I'll forget in the future...

Nice, fixed.

>> Tools/Scripts/test262/Runner.pm:708
>> +    }
> 
> Yikes, that seems expensive to do on every file...

We are already doing flags check to extract `noStrict` etc. So, I don't expect this will change the cost :)

[Yusuke Suzuki]

Committed r269531: <https://trac.webkit.org/changeset/269531>

[Radar WebKit Bug Importer]

<rdar://problem/71129125>