[webkit-reviews] review granted: [Bug 163412] Introduce RunCallbackScope to implement backup incumbent settings object stack : [Attachment 449492] Patch

[bugzilla-daemon at webkit.org]

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.