[webkit-reviews] review denied: [Bug 229559] Enable SharedArrayBuffer support when COOP/COEP headers are used : [Attachment 436772] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 229559: Enable SharedArrayBuffer support when COOP/COEP headers are used
https://bugs.webkit.org/show_bug.cgi?id=229559

Attachment 436772: Patch

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




--- Comment #18 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 436772
  --> https://bugs.webkit.org/attachment.cgi?id=436772
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/c
ross-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?