Bug 229559 – Enable SharedArrayBuffer support when COOP/COEP headers are used

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Re-enable SharedArrayBuffer when COOP/COEP are used.

[Radar WebKit Bug Importer]

<rdar://problem/82391945>

[Chris Dumez]

Created attachment 436515 [details]
WIP patch

[Chris Dumez]

Created attachment 436517 [details]
WIP patch

[Chris Dumez]

Created attachment 436527 [details]
WIP patch

[Chris Dumez]

Created attachment 436535 [details]
WIP patch

[Chris Dumez]

Created attachment 436573 [details]
WIP patch

[Chris Dumez]

Created attachment 436580 [details]
WIP patch

[Yusuke Suzuki]

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

[Chris Dumez]

Created attachment 436588 [details]
WIP patch

[Yusuke Suzuki]

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

[Chris Dumez]

Created attachment 436636 [details]
WIP patch (now with API tests)

[Chris Dumez]

Created attachment 436639 [details]
WIP patch

[Chris Dumez]

Created attachment 436656 [details]
Patch

[Chris Dumez]

Created attachment 436676 [details]
Patch

[Chris Dumez]

Created attachment 436694 [details]
Patch

[Ryosuke Niwa]

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 };

[Chris Dumez]

Created attachment 436772 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

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.

[Chris Dumez]

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.

[Alex Christensen]

Oh, you're right.  JSC::initialize and JSC::Options::initialize are different things.

[Chris Dumez]

Created attachment 436934 [details]
Patch

[Chris Dumez]

Created attachment 436957 [details]
Patch

[EWS]

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].