Bug 229559 - Enable SharedArrayBuffer support when COOP/COEP headers are used
Summary: Enable SharedArrayBuffer support when COOP/COEP headers are used
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
: 218943 218944 (view as bug list)
Depends on: 229465 229501
Blocks: 228137 228755
  Show dependency treegraph
 
Reported: 2021-08-26 08:44 PDT by Chris Dumez
Modified: 2021-09-09 02:42 PDT (History)
24 users (show)

See Also:


Attachments
WIP patch (4.38 KB, patch)
2021-08-26 08:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (6.24 KB, patch)
2021-08-26 09:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (10.05 KB, patch)
2021-08-26 09:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (42.31 KB, patch)
2021-08-26 11:24 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (44.54 KB, patch)
2021-08-26 14:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (47.99 KB, patch)
2021-08-26 15:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (52.15 KB, patch)
2021-08-26 16:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (now with API tests) (72.83 KB, patch)
2021-08-27 09:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (68.71 KB, patch)
2021-08-27 10:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.27 KB, patch)
2021-08-27 12:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (90.09 KB, patch)
2021-08-27 14:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (90.80 KB, patch)
2021-08-27 17:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (101.83 KB, patch)
2021-08-30 08:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (102.55 KB, patch)
2021-08-31 13:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (102.81 KB, patch)
2021-08-31 16:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-08-26 08:44:23 PDT
Re-enable SharedArrayBuffer when COOP/COEP are used.
Comment 1 Radar WebKit Bug Importer 2021-08-26 08:44:46 PDT
<rdar://problem/82391945>
Comment 2 Chris Dumez 2021-08-26 08:55:44 PDT
Created attachment 436515 [details]
WIP patch
Comment 3 Chris Dumez 2021-08-26 09:13:14 PDT
Created attachment 436517 [details]
WIP patch
Comment 4 Chris Dumez 2021-08-26 09:37:46 PDT
Created attachment 436527 [details]
WIP patch
Comment 5 Chris Dumez 2021-08-26 11:24:21 PDT
Created attachment 436535 [details]
WIP patch
Comment 6 Chris Dumez 2021-08-26 14:57:41 PDT
Created attachment 436573 [details]
WIP patch
Comment 7 Chris Dumez 2021-08-26 15:49:53 PDT
Created attachment 436580 [details]
WIP patch
Comment 8 Yusuke Suzuki 2021-08-26 15:54:35 PDT
*** Bug 218944 has been marked as a duplicate of this bug. ***
Comment 9 Chris Dumez 2021-08-26 16:48:26 PDT
Created attachment 436588 [details]
WIP patch
Comment 10 Yusuke Suzuki 2021-08-26 18:45:32 PDT
*** Bug 218943 has been marked as a duplicate of this bug. ***
Comment 11 Chris Dumez 2021-08-27 09:47:07 PDT
Created attachment 436636 [details]
WIP patch (now with API tests)
Comment 12 Chris Dumez 2021-08-27 10:06:51 PDT
Created attachment 436639 [details]
WIP patch
Comment 13 Chris Dumez 2021-08-27 12:23:38 PDT
Created attachment 436656 [details]
Patch
Comment 14 Chris Dumez 2021-08-27 14:43:30 PDT
Created attachment 436676 [details]
Patch
Comment 15 Chris Dumez 2021-08-27 17:00:10 PDT
Created attachment 436694 [details]
Patch
Comment 16 Ryosuke Niwa 2021-08-27 21:10:00 PDT
Comment on attachment 436694 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:957
> +    if (currentCoopEnforcementResult->crossOriginOpenerPolicy.value == CrossOriginOpenerPolicyValue::SameOriginPlusCOEP)
> +        return NeedsBrowsingContextGroupSwitch::YesWithCrossOriginIsolation;
> +    return NeedsBrowsingContextGroupSwitch::YesWithoutCrossOriginIsolation;

How about
BrowsingContextGroup::StayInGroup
BrowsingContextGroup::NewSharedGroup
BrowsingContextGroup::NewIsolatedGroup

> Source/WebKit/UIProcess/WebProcessProxy.h:136
> +    enum class IsCrossOriginIsolated : bool { No, Yes };

How about enum class CrossOriginMode : bool { Shared, Isolated };
Comment 17 Chris Dumez 2021-08-30 08:09:48 PDT
Created attachment 436772 [details]
Patch
Comment 18 Alex Christensen 2021-08-31 12:20:32 PDT
Comment on attachment 436772 [details]
Patch

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

> Source/WebKit/ChangeLog:24
> +        JSC Options need to be set before JSC::initialize() is called, which occurs

This doesn't seem to be true with the code.  It seems to be set immediately after JSC::initialize but before other things.

> Source/WebCore/dom/ScriptExecutionContext.cpp:84
> +static CrossOriginMode globalCrossOriginMode { CrossOriginMode::Shared };

Since this is written to by the main thread and read from by multiple threads, should it be std::atomic?  Right now it's written only during process initialization, so it's probably no big deal.

> Source/WebCore/loader/DocumentLoader.cpp:954
> +        return BrowsingContextGroup::StayInGroup;

I think BrowsingContextGroup isn't a great name for this.  It's more of a policy action than a group, which makes me think of PageGroup or some other kind of container.  
"BrowsingContextGroupSwitchDecision" or something

> Source/WebCore/page/DOMWindow.idl:-82
> -    // FIXME: Implement 'originIsolated'

So I guess this isn't a thing any more, replaced by window.crossOriginIsolated, right?

> Source/WebKit/Shared/WebProcessCreationParameters.h:139
> +    WebCore::CrossOriginMode crossOriginMode { false }; // Cross-origin isolation via COOP+COEP headers.

false is the wrong type.  I think this should be { WebCore::CrossOriginMode::CrossOriginMode }

> Source/WebKit/UIProcess/WebProcessProxy.h:446
> +    bool shouldEnableSharedArrayBuffer() const final { return m_crossOriginMode == WebCore::CrossOriginMode::Isolated; }

Will there be other uses of this?  We may want to do other things like increase the resolution of DOMHighResTimeStamp.  I think this should be called isCrossOriginIsolated(), but no strong feelings.  It is currently used for enabling SharedArrayBuffer.

> LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-embedder-policy/cross-origin-isolated-permission.https-expected.txt:5
> +FAIL frame: origin = https://localhost:9443, value = (\) assert_equals: expected false but got true

Why did this used to pass and it now fails?
Comment 19 Chris Dumez 2021-08-31 12:30:17 PDT
Comment on attachment 436772 [details]
Patch

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

>> Source/WebKit/ChangeLog:24
>> +        JSC Options need to be set before JSC::initialize() is called, which occurs
> 
> This doesn't seem to be true with the code.  It seems to be set immediately after JSC::initialize but before other things.

I did what Mark Lam suggested and it is working. I think my changelog is just wrong. Will fix.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:84
>> +static CrossOriginMode globalCrossOriginMode { CrossOriginMode::Shared };
> 
> Since this is written to by the main thread and read from by multiple threads, should it be std::atomic?  Right now it's written only during process initialization, so it's probably no big deal.

As you said, it is only set during process initialization so I don't think this is strictly necessary.

>> Source/WebCore/loader/DocumentLoader.cpp:954
>> +        return BrowsingContextGroup::StayInGroup;
> 
> I think BrowsingContextGroup isn't a great name for this.  It's more of a policy action than a group, which makes me think of PageGroup or some other kind of container.  
> "BrowsingContextGroupSwitchDecision" or something

OK, I had one naming in my original patch (NeedsBrowsingContextGroupSwitch { No, YesWithCrossOriginIsolation, YesWithoutCrossOriginIsolation };

But Ryosuke didn't like it so I went with the exact naming he suggested.

>> Source/WebCore/page/DOMWindow.idl:-82
>> -    // FIXME: Implement 'originIsolated'
> 
> So I guess this isn't a thing any more, replaced by window.crossOriginIsolated, right?

Right, replaced by self.crossOriginIsolated.

>> Source/WebKit/Shared/WebProcessCreationParameters.h:139
>> +    WebCore::CrossOriginMode crossOriginMode { false }; // Cross-origin isolation via COOP+COEP headers.
> 
> false is the wrong type.  I think this should be { WebCore::CrossOriginMode::CrossOriginMode }

Oops, indeed. Got it wrong when I did the renaming that Ryosuke suggested.

>> Source/WebKit/UIProcess/WebProcessProxy.h:446
>> +    bool shouldEnableSharedArrayBuffer() const final { return m_crossOriginMode == WebCore::CrossOriginMode::Isolated; }
> 
> Will there be other uses of this?  We may want to do other things like increase the resolution of DOMHighResTimeStamp.  I think this should be called isCrossOriginIsolated(), but no strong feelings.  It is currently used for enabling SharedArrayBuffer.

I already have a follow-up patch almost ready for DOMHighResTimeStamp. That patch will rely on ScriptExecutionContext::crossOriginMode() since it is in WebCore.

>> LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-embedder-policy/cross-origin-isolated-permission.https-expected.txt:5
>> +FAIL frame: origin = https://localhost:9443, value = (\) assert_equals: expected false but got true
> 
> Why did this used to pass and it now fails?

I explained in the changelog. This test relies on Permissions-Policy HTTP header (https://w3c.github.io/webappsec-permissions-policy/) which we don't support.
We used to pass by luck because self.crossOriginIsolated was returning false unconditionally.
Comment 20 Chris Dumez 2021-08-31 12:34:43 PDT
Comment on attachment 436772 [details]
Patch

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

>>> Source/WebKit/ChangeLog:24
>>> +        JSC Options need to be set before JSC::initialize() is called, which occurs
>> 
>> This doesn't seem to be true with the code.  It seems to be set immediately after JSC::initialize but before other things.
> 
> I did what Mark Lam suggested and it is working. I think my changelog is just wrong. Will fix.

My code seems correct, so does the changelog. According to Mark Lam, I need to set the option *after* calling JSC::Options::initialize() but *before* JSC::initialize() is called.
Comment 21 Alex Christensen 2021-08-31 12:40:53 PDT
Oh, you're right.  JSC::initialize and JSC::Options::initialize are different things.
Comment 22 Chris Dumez 2021-08-31 13:21:55 PDT
Created attachment 436934 [details]
Patch
Comment 23 Chris Dumez 2021-08-31 16:01:19 PDT
Created attachment 436957 [details]
Patch
Comment 24 EWS 2021-08-31 16:53:24 PDT
Committed r281832 (241165@main): <https://commits.webkit.org/241165@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436957 [details].