Bug 191822 - Add SPI to disable JIT in a WKWebView
Summary: Add SPI to disable JIT in a WKWebView
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
Keywords: InRadar
Depends on:
Reported: 2018-11-17 22:54 PST by Alex Christensen
Modified: 2018-11-19 18:17 PST (History)
7 users (show)

See Also:

Patch (24.82 KB, patch)
2018-11-17 23:11 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.38 KB, patch)
2018-11-19 11:10 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.46 KB, patch)
2018-11-19 17:23 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2018-11-17 22:54:03 PST
Add SPI to disable JIT in a WKWebView
Comment 1 Alex Christensen 2018-11-17 23:11:14 PST
Created attachment 355229 [details]
Comment 2 Geoffrey Garen 2018-11-18 13:02:14 PST
Comment on attachment 355229 [details]

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


> 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; }


> 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)>&&);


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

Comment 3 Geoffrey Garen 2018-11-18 13:25:33 PST
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).)
Comment 4 Alex Christensen 2018-11-19 11:10:51 PST
Created attachment 355287 [details]
Comment 5 Geoffrey Garen 2018-11-19 15:26:05 PST
Comment on attachment 355287 [details]

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


Still a few opportunities to increase naming consistency.

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


> 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;


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


> 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; }

Comment 6 Alex Christensen 2018-11-19 17:23:45 PST
Created attachment 355300 [details]
Comment 7 Alex Christensen 2018-11-19 17:39:46 PST
Comment on attachment 355300 [details]

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.
Comment 8 Saam Barati 2018-11-19 17:42:45 PST
Comment on attachment 355300 [details]

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.
Comment 9 Alex Christensen 2018-11-19 18:13:40 PST
Comment on attachment 355300 [details]

Good idea! I'm adding ASSERT(!allocator); at the beginning of ExecutableAllocator::setJITEnabled to remind us when it can be called.
Comment 10 Alex Christensen 2018-11-19 18:16:43 PST
Comment 11 Radar WebKit Bug Importer 2018-11-19 18:17:51 PST