[webkit-reviews] review denied: [Bug 234155] [Shadow Realms] Use WebCore module loaders for shadow realm importValue : [Attachment 448861] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Joseph Griego <jgriego at igalia.com>'s
request for review:
Bug 234155: [Shadow Realms] Use WebCore module loaders for shadow realm
importValue
https://bugs.webkit.org/show_bug.cgi?id=234155

Attachment 448861: Patch

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




--- Comment #17 from Darin Adler <darin at apple.com> ---
Comment on attachment 448861
  --> https://bugs.webkit.org/attachment.cgi?id=448861
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448861&action=review

This looks great, but I see a lot of room for small improvements, especially in
coding style.

I am setting review- only because of the infinite loop I found in
JSDOMGlobalObject::deriveShadowRealmGlobalObject.

> Source/JavaScriptCore/runtime/ShadowRealmObject.h:51
> +    static ShadowRealmObject* create(VM&, Structure*, JSGlobalObject*);

Worth considering whether we want to pass both VM& and JSGlobalObject*. Since
you can quickly and efficiently get the VM from the global object, sometimes we
choose to pass only the global object. But this might not be one of those
places.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:272
> +    if (inherits<JSShadowRealmGlobalScopeBase>(vm()))
> +	   return jsCast<const
JSShadowRealmGlobalScopeBase*>(this)->scriptExecutionContext();

Slightly puzzled by the way this list is sorted; not quite alphabetical. Good
that we put the two from "normal web browsing" first, I guess, for performance
optimization reasons maybe?

Eventually we will have to change this so it walks the inheritance chain only
once rather than 6 times in a row. Hoping this is not yet a performance
bottleneck.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:514
> +    if (globalObject->inherits<JSShadowRealmGlobalScopeBase>(vm))
> +	   return &jsCast<const
JSShadowRealmGlobalScopeBase*>(globalObject)->wrapped().moduleLoader();

Same comments.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:579
> +JSC::JSGlobalObject*
JSDOMGlobalObject::deriveShadowRealmGlobalObject(JSC::VM& vm,
JSC::JSGlobalObject* globalObject)

Irritating that this function takes JSGlobalObject and needs to "know" it's a
JSDOMGlobalObject? I know many other functions do that too, but it’s a
frustrating pattern. All those runtime checks for something that is guaranteed.
But maybe there’s no simple solution because of the method table.

Also, why take both a vm and a globalObject? Would be nice to not take the vm
unless it’s an important optimization. Eventually all the extra arguments start
to make functions harder to read.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:596
> +	   auto doc = downcast<Document>(context);

The abbreviation "doc" is a bit of an anti-pattern. We use words instead of
abbreviations most of the time. I suggest document.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:605
> +	   while (!doc->isTopDocument()) {
> +	       auto candidate = doc->parentDocument();
> +	       auto topGlobal =
jsCast<JSDOMGlobalObject*>(candidate->globalObject());
> +	       if (&topGlobal->world() == &domGlobalObject->world()) {
> +		   doc = candidate;
> +		   domGlobalObject = topGlobal;
> +	       }
> +	   }

This loop looks wrong in a risky way. If the worlds are ever not equal, it
becomes an infinite loop, since nothing changes doc! And this proves we don’t
have extensive-enough test coverage; it might be difficult to construct a test
for this, but we need one.

I think it would be nice if we could find a way to write it as a for loop.

Also the cast to JSDOMGlobalObject is safe, but why would we have to think
about that here. It would be best to change the return type of globalObject in
Document, or even some lower base class, so we don’t have to do the cast here.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:611
> +    Structure* structure = JSShadowRealmGlobalScope::createStructure(vm,
nullptr, JSC::jsNull());

I suggest auto here.

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:42
> +class ScriptModuleLoader;

I don’t see ScriptModuleLoader being newly used in this file, so why did we
have to add this forward declaration.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:117
> +    auto obj = jsCast<const
JSShadowRealmGlobalScopeBase*>(object)->incubating();

WebKit coding style says we use words, not abbreviations. I know we are using
"obj" to dodge the argument name "object". How about calling the local variable
"scope" or calling the argument "untypedObject" or "globalObject"?

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:42
> +    template<typename, JSC::SubspaceAccess>
> +    static void subspaceFor(JSC::VM&) { RELEASE_ASSERT_NOT_REACHED(); }

Frustrating that we have to do a runtime assertion here rather than an error at
compile time or link time? Is there some way to do that? Also, I would suggest
putting this all on a single line to make the class definition easier to read.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:50
> +    const JSDOMGlobalObject* incubating() const;

Not thrilled with an adjective, "incubating" as the name for a getter. I
suppose "wrapped" already follows that pattern, but consider "incubatingRealm"
instead.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:54
> +    JSDOMGlobalObject* incubating()
> +    {
> +	   return const_cast<JSDOMGlobalObject*>(const_cast<const
JSShadowRealmGlobalScopeBase*>(this)->incubating());
> +    }

Can we put the body of this inline function at the end of the file rather than
in the middle of the class? I think the class definition becomes easier to read
if we are not interspersing code with declarations.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:58
> +    static const JSC::GlobalObjectMethodTable s_globalObjectMethodTable;

Does this need to be public? Can it be private or protected instead?

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:66
> +    static bool supportsRichSourceInfo(const JSC::JSGlobalObject*);
> +    static bool shouldInterruptScript(const JSC::JSGlobalObject*);
> +    static bool shouldInterruptScriptBeforeTimeout(const
JSC::JSGlobalObject*);
> +    static JSC::RuntimeFlags javaScriptRuntimeFlags(const
JSC::JSGlobalObject*);
> +    static JSC::ScriptExecutionStatus
scriptExecutionStatus(JSC::JSGlobalObject*, JSC::JSObject*);
> +    static void queueMicrotaskToEventLoop(JSC::JSGlobalObject&,
Ref<JSC::Microtask>&&);
> +    static void reportViolationForUnsafeEval(JSC::JSGlobalObject*,
JSC::JSString*);

These can all be private, I believe, rather than public.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:80
> +    auto loader = WTF::makeUnique<ScriptModuleLoader>(m_context,
m_ownerType);

The "WTF::" prefix should not be needed here.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:257
> +    if (m_shadowRealmGlobal) {
> +	   RELEASE_AND_RETURN(scope,
moduleRecord->evaluate(m_shadowRealmGlobal, awaitedValue, resumeMode));
> +    } else if (m_ownerType == OwnerType::Document) {

WebKit coding style says no braces here.

> Source/WebCore/bindings/js/ScriptModuleLoader.h:58
> +    std::unique_ptr<ScriptModuleLoader>
shadowRealmLoader(JSC::JSGlobalObject* realmGlobal) const;

Why does this return std::unique_ptr instead of UniqueRef? Can it return
nullptr?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2887
> +    } elsif ($interfaceName eq "ShadowRealmGlobalScope" ||
$codeGenerator->InheritsInterface($interface, "WorkerGlobalScope") ||
$codeGenerator->InheritsInterface($interface, "WorkletGlobalScope")) {

This code is getting repetitive. I suggest a helper for this set of three names
for use in the 5 or 6 places we have them in this patch.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:46
> +{ }

I think our style says to put these on subsequent lines, rather than one one
line.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:63
> +JSShadowRealmGlobalScopeBase* ShadowRealmGlobalScope::wrapper()
> +{
> +    return m_wrapper.get();
> +}

If we can do it without adding includes, I suggest putting this as an inline
function body into the header.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:65
> +ShadowRealmGlobalScope::~ShadowRealmGlobalScope() { }

If we need to explicitly put this in the .cpp file, and I am not sure we do,
please use "= default".

> Source/WebCore/page/ShadowRealmGlobalScope.h:28
> +#include <JavaScriptCore/RuntimeFlags.h>

Why does this file include Strong.h? It can be important to minimize includes
in headers, so please double check there’s nothing extra here.

> Source/WebCore/page/ShadowRealmGlobalScope.h:29
> +#include <JavaScriptCore/Strong.h>

Why does this file include Strong.h? It can be important to minimize includes
in headers, so please double check there’s nothing extra here.

> Source/WebCore/page/ShadowRealmGlobalScope.h:36
> +namespace JSC { class VM; }

This is nice and attractive, but not the correct WebKit style. But also, this
file includes Strong.h, so there’s no need for this forward declaration. But
maybe if you remove the Strong.h include you will need to keep this and it
would be good to use the normal WebKit style.

> Source/WebCore/page/ShadowRealmGlobalScope.h:43
> +class JSShadowRealmGlobalScopeBase;
> +class JSDOMGlobalObject;
> +class ScriptModuleLoader;
> +class ScriptExecutionContext;

These should be sorted alphabetically.

> Source/WebCore/page/ShadowRealmGlobalScope.h:50
> +    static RefPtr<ShadowRealmGlobalScope> tryCreate(JSDOMGlobalObject*,
ScriptModuleLoader*);

This should take JSDOMGlobalObject&, not JSDOMGlobalObject*.

Also, why does this take a ScriptModuleLoader*? Can’t this class call
scriptModuleLoader? Would that be a layering violation?

> Source/WebCore/page/ShadowRealmGlobalScope.h:58
> +    ShadowRealmGlobalScope(JSDOMGlobalObject*, ScriptModuleLoader*);

Ditto.

> Source/WebCore/page/ShadowRealmGlobalScope.h:64
> +    JSC::Weak<JSShadowRealmGlobalScopeBase> m_wrapper { };
> +    std::unique_ptr<ScriptModuleLoader> m_moduleLoader { };

These "{ }" are not needed. Non-scalars like these are initialized without
doing anything explicit. On the other hand, m_parentLoader should have "{
nullptr }" after it, even if it’s dead code, because it’s harmless and unlikely
to generate any extra code, and it’s good to be able to see that nothing is
uninitialized without reading the bodies of all constructors.

> Source/WebCore/page/ShadowRealmGlobalScope.idl:27
> + */
> +
> +
> +[

Extra blank line here. Please use only one.

> Source/WebCore/page/ShadowRealmGlobalScope.idl:33
> +  readonly attribute ShadowRealmGlobalScope self;

Please indent 4 unless there is a reason not to.

> LayoutTests/ChangeLog:7
> +	   Improve test coverage a bit for shadow realms, especially running
> +	   nested, in worker contexts, or in iframes.

Can these tests be added to Web Platform Tests instead of WebKit-specific
tests?

I personally find our WebKit-specific tests easier to read and maintain, but
Web Platform Tests server an important additional purpose of helping us achieve
interoperability between web browsers, and so they are strongly preferred.