Bug 212069 - Re-enable SharedArrayBuffer for JSC shell and Testers
Summary: Re-enable SharedArrayBuffer for JSC shell and Testers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 185038 197191 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-19 06:03 PDT by Sergey Rubanov
Modified: 2021-08-12 11:14 PDT (History)
29 users (show)

See Also:


Attachments
Patch (93.49 KB, patch)
2020-11-04 23:29 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (103.94 KB, patch)
2020-11-05 02:05 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (99.09 KB, patch)
2020-11-05 19:52 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (100.85 KB, patch)
2020-11-05 20:59 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (104.07 KB, patch)
2020-11-06 00:17 PST, Yusuke Suzuki
keith_miller: review+
keith_miller: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Rubanov 2020-05-19 06:03:22 PDT
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
Comment 1 Jarred Sumner 2020-08-11 13:05:05 PDT
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.
Comment 2 Yusuke Suzuki 2020-11-04 00:29:22 PST
*** Bug 197191 has been marked as a duplicate of this bug. ***
Comment 3 Yusuke Suzuki 2020-11-04 01:57:14 PST
*** Bug 185038 has been marked as a duplicate of this bug. ***
Comment 4 Yusuke Suzuki 2020-11-04 23:29:55 PST
Created attachment 413257 [details]
Patch
Comment 5 Anne van Kesteren 2020-11-05 01:50:57 PST
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.
Comment 6 Simon Pieters (:zcorpan) 2020-11-05 01:53:06 PST
Cross-origin isolated is https://bugs.webkit.org/show_bug.cgi?id=215407 - as Anne said, I believe it should be blocking this bug.
Comment 7 Yusuke Suzuki 2020-11-05 02:05:41 PST
Created attachment 413275 [details]
Patch
Comment 8 Yusuke Suzuki 2020-11-05 11:14:43 PST
(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.
Comment 9 Yusuke Suzuki 2020-11-05 11:21:13 PST
And possibly we will enable it for JavaScriptCore.framework.
Comment 10 Yusuke Suzuki 2020-11-05 19:52:51 PST
Created attachment 413388 [details]
Patch
Comment 11 Yusuke Suzuki 2020-11-05 20:59:55 PST
Created attachment 413398 [details]
Patch
Comment 12 Yusuke Suzuki 2020-11-06 00:17:02 PST
Created attachment 413408 [details]
Patch
Comment 13 Anne van Kesteren 2020-11-06 00:29:50 PST
Thanks for confirming Yusuke, exciting!
Comment 14 Keith Miller 2020-11-06 08:26:15 PST
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...
Comment 15 Yusuke Suzuki 2020-11-06 11:04:13 PST
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 :)
Comment 16 Yusuke Suzuki 2020-11-06 12:23:56 PST
Committed r269531: <https://trac.webkit.org/changeset/269531>
Comment 17 Radar WebKit Bug Importer 2020-11-06 12:24:34 PST
<rdar://problem/71129125>