Bug 191822 – Add SPI to disable JIT in a WKWebView

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Add SPI to disable JIT in a WKWebView

[Alex Christensen]

Created attachment 355229 [details]
Patch

[Geoffrey Garen]

Comment on attachment 355229 [details]
Patch

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

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:120
> +static bool& jitDisabled()
> +{
> +    static bool disabled = false;
> +    return disabled;
> +}

Let's pick a consistent phrase and use it everywhere -- rather than the family of jitDisabled, disableJIT, canUseJIT, and enableJIT.

I usually prefer boolean conditions to be named after the true condition, to avoid mental gymnastics around double negatives when the conditions are false. (E.g., "Enable the JIT" and "Do not enable the JIT" parse OK to me, but "Do not not enable the JIT" is hard for me to parse.)

So, normally I would recommend "setJITEnabled" and "isJITEnabled" as the canonical phrases.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:122
>  static bool allowJIT()

Let's pair this with the function above and call them "setJITEnabled" and "isJITEnabled".

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:125
> +    return processHasEntitlement("dynamic-codesigning") && !jitDisabled();

Note the use of "not not enabled" here, which I would prefer to avoid.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:131
> +void ExecutableAllocator::disableJIT()

This can be the implementation of the setter in the false condition.

> Source/WebKit/UIProcess/WebPageProxy.cpp:6530
> +void WebPageProxy::canUseJIT(CompletionHandler<void(bool)>&& completionHandler)

I'd call this function and its message isJITEnabled.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4771
> +- (void)_canUseJIT:(void(^)(BOOL))completionHandler

isJITEnabled

> Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:71
> +@property (nonatomic) BOOL enableJIT WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

I'd call this JITEnabled getter=isJITEnabled

> Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:51
> +        virtual bool enableJIT() const { return true; }

isJITEnabled

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:178
> +        xpc_dictionary_set_bool(bootstrapMessage.get(), "disable-jit", true);

Maybe it's ok for this message to be "disable-jit". The best alternative I can think of is "set-jit-enabled-false". I'm not sure if that's better.

> Source/WebKit/WebProcess/WebProcess.h:207
> +    void canUseJIT(CompletionHandler<void(bool)>&&);

isJITEnabled

> Source/WebKit/WebProcess/WebProcess.messages.in:144
> +    CanUseJIT() -> (bool canUse) Async

IsJITEnabled

[Geoffrey Garen]

Usually when we have a one way toggle for X we call it “setX” or “clearX”. 

So I guess my recommendation is to name the setter in JSC and the bootstrap xpc message “clearJITEnabled” and “clear-jit-enabled”. 

(I think part of why you chose “disableJIT” might be that it’s not terribly productive to call setJITEnabled(true). So it doesn’t feel right for JSC to offer setJITEnabled(bool).)

[Alex Christensen]

Created attachment 355287 [details]
Patch

[Geoffrey Garen]

Comment on attachment 355287 [details]
Patch

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

r=me

Still a few opportunities to increase naming consistency.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:116
> +static bool s_jitEnabled = true;

s_isJITEnabled

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:128
> +void ExecutableAllocator::setJITEnabled(bool enabled)
> +{
> +    s_jitEnabled = enabled;

Probably worth an early return if the setting didn't change.

> Source/WebKit/UIProcess/WebProcessProxy.h:261
> +    bool JITEnabled() const final;

isJITEnabled()

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:226
> +    bool m_JITEnabled { true };

m_isJITEnabled

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4773
> +    _page->isJITEnabled([completionHandler = makeBlockPtr(completionHandler)] (bool canUse) {

canUse => isJITEnabled

> Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:51
> +        virtual bool JITEnabled() const { return true; }

isJITEnabled

[Alex Christensen]

Created attachment 355300 [details]
Patch

[Alex Christensen]

Comment on attachment 355300 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessProxy.cpp:54
> +#include <JavaScriptCore/VM.h>

This isn't needed.  Won't commit it.

[Saam Barati]

Comment on attachment 355300 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:73
> +    if (initializerMessage && xpc_dictionary_get_bool(initializerMessage, "disable-jit"))

Does this happen before initializeThreading? I believe it does. It’d be good to add such an assertion to the API you added in JSC as well. I think such an assertion is better than relying on the OS only allocating X memory once.

[Alex Christensen]

Comment on attachment 355300 [details]
Patch

Good idea! I'm adding ASSERT(!allocator); at the beginning of ExecutableAllocator::setJITEnabled to remind us when it can be called.

[Alex Christensen]

http://trac.webkit.org/r238388

[Radar WebKit Bug Importer]

<rdar://problem/46176350>