Summary: | Enable SharedArrayBuffer support when COOP/COEP headers are used | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, alecflett, annulen, anthony.bowker, beidson, bugmail, darin, esprehn+autocc, ews-watchlist, ggaren, gyuyoung.kim, hotaru, japhet, jsbell, kangil.han, kkinnunen, kondapallykalyan, mark.lam, rniwa, ryuan.choi, sam, sergio, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Bug Depends on: | 229465, 229501 | ||||||||||||||||||||||||||||||||||
Bug Blocks: | 228137, 228755 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2021-08-26 08:44:23 PDT
Created attachment 436515 [details]
WIP patch
Created attachment 436517 [details]
WIP patch
Created attachment 436527 [details]
WIP patch
Created attachment 436535 [details]
WIP patch
Created attachment 436573 [details]
WIP patch
Created attachment 436580 [details]
WIP patch
*** Bug 218944 has been marked as a duplicate of this bug. *** Created attachment 436588 [details]
WIP patch
*** Bug 218943 has been marked as a duplicate of this bug. *** Created attachment 436636 [details]
WIP patch (now with API tests)
Created attachment 436639 [details]
WIP patch
Created attachment 436656 [details]
Patch
Created attachment 436676 [details]
Patch
Created attachment 436694 [details]
Patch
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 }; Created attachment 436772 [details]
Patch
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 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 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. Oh, you're right. JSC::initialize and JSC::Options::initialize are different things. Created attachment 436934 [details]
Patch
Created attachment 436957 [details]
Patch
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]. |