[webkit-reviews] review granted: [Bug 163412] Introduce RunCallbackScope to implement backup incumbent settings object stack : [Attachment 449492] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 24 16:19:08 PST 2022
Geoffrey Garen <ggaren at apple.com> has granted Alexey Shvayka
<ashvayka at apple.com>'s request for review:
Bug 163412: Introduce RunCallbackScope to implement backup incumbent settings
object stack
https://bugs.webkit.org/show_bug.cgi?id=163412
Attachment 449492: Patch
https://bugs.webkit.org/attachment.cgi?id=449492&action=review
--- Comment #62 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 449492
--> https://bugs.webkit.org/attachment.cgi?id=449492
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=449492&action=review
r=me
Seems like there's work to make EWS happy before landing.
> Source/JavaScriptCore/interpreter/Interpreter.cpp:757
> + SetForScope<JSGlobalObject*>
changeJSONPGlobalObject(vm.JSONPGlobalObject, globalObject);
Neat simplification.
> Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:58
> +
internalField(Field::IncumbentGlobalObject).setWithoutWriteBarrier(&JSC::incumb
entGlobalObject(globalObject, vm.topCallFrame));
Makes me sad, but I guess we have to do it to match the spec.
> Source/JavaScriptCore/runtime/RunCallbackScope.cpp:52
> + if (isSameOriginDomain(topCallFrameGlobalObject,
m_incumbentGlobalObject) == TriState::True)
> + return m_incumbentGlobalObject;
I wonder if we should add an ASSERT_WITH_SECURITY_IMPLICATION here. We don't
expect these to be different. Something like:
auto isSameOriginDomain =
topCallFrameGlobalObject->globalObjectMethodTable()->isSameOriginDomain(topCall
FrameGlobalObject, m_incumbentGlobalObject) == TriState::True;
ASSERT_WITH_SECURITY_IMPLICATION(isSameOriginDomain);
if (isSameOriginDomain)
return m_incumbentGlobalObject;
Anyway, I like this compromise. It seems to ensure that the disappointing
complexity of the incumbent global object spec can only create compatibility
issues, and not security issues.
> Source/JavaScriptCore/runtime/RunCallbackScope.h:33
> +class JS_EXPORT_PRIVATE RunCallbackScope final {
I wonder if we should name this "AuthorCallbackScope" to clarify that you
should deploy it any time you call into code that the spec would label "author"
code. Or some other word to indicate "I'm calling out to author-specific
JavaScript".
I'm not sure that "Run" adds information, so "CallbackScope" might also be OK.
More information about the webkit-reviews
mailing list