Bug 182605 – Silence MAP_JIT warning for Network Process

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

The Network process periodically invokes JavaScriptCore to process Proxy Auto-Config (PAC) files. We don't support JIT compilation in the Network Process, which triggers spurious warnings when JavaScriptCore attempts to invoke the JIT. The warning messages are harmless, but they create log spam that makes it difficult to notice actual problems.

This patch silences this expected warning in the Network Process.

[Alexey Proskuryakov]

30354260

[Brent Fulgham]

<rdar://problem/35368975>

[Brent Fulgham]

I meant this:
<rdar://problem/30354260>

[Mark Lam]

<rdar://problem/38271229>

[Tadeu Zagallo]

Created attachment 342190 [details]
Patch

Comment on attachment 342190 [details]
Patch

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

> Source/WTF/wtf/cocoa/Entitlements.cpp:40
> +    auto cfEntitlement = adoptCF(CFStringCreateWithCString(CFAllocatorGetDefault(), entitlement, kCFStringEncodingUTF8));

We can just use kCFAllocatorDefault here. But also, would it make more sense for processHasEntitlement() to take a WTF String?

[Mark Lam]

Comment on attachment 342190 [details]
Patch

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

If you haven't already done so, please test this by adding a RELEASE_ASSERT(processHasEntitlement("dynamic-codesigning")) in allowJIT(), build for iOS, run it, and confirm that it doesn't crash.

r=me with fixes and testing.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:122
> +#endif
> +    return true;

This "return true" should be in an #else section.  Otherwise, you'll have unreachable code on PLATFORM(IOS).

> Source/WTF/wtf/cocoa/Entitlements.cpp:51
> +}

nit: add // namespace WTF

> Source/WTF/wtf/cocoa/Entitlements.h:32
> +}

nit: I would prefer that we add a "// namespace WTF" here.  This doesn't really matter right now, but if more lines get added above to take this } further away from the opening {, then the comment helps provide some context.  I'd just prefer to do it all the time to make the pairing clear.

[Mark Lam]

Comment on attachment 342190 [details]
Patch

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

>> Source/WTF/wtf/cocoa/Entitlements.cpp:40
>> +    auto cfEntitlement = adoptCF(CFStringCreateWithCString(CFAllocatorGetDefault(), entitlement, kCFStringEncodingUTF8));
> 
> We can just use kCFAllocatorDefault here. But also, would it make more sense for processHasEntitlement() to take a WTF String?

I think it's equally valid to just take a C string (as is) in this case, unless you know of a specific reason to switch to a WTF String.

(In reply to Mark Lam from comment #8)
> Comment on attachment 342190 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342190&action=review
> 
> >> Source/WTF/wtf/cocoa/Entitlements.cpp:40
> >> +    auto cfEntitlement = adoptCF(CFStringCreateWithCString(CFAllocatorGetDefault(), entitlement, kCFStringEncodingUTF8));
> > 
> > We can just use kCFAllocatorDefault here. But also, would it make more sense for processHasEntitlement() to take a WTF String?
> 
> I think it's equally valid to just take a C string (as is) in this case,
> unless you know of a specific reason to switch to a WTF String.

I made the comment because, just looking at this patch, I see an existing caller in possession of an NSString. That caller will now have to get a C string out of it, and then the callee will end up allocating a brand new CFString for it. Further (and perhaps this can be fixed independently), the callee is not even sharing the string buffer but instead copying it. For the new caller being introduced in this patch, I think either interface (C string or WTF String) would work just as well.

[Joseph Pecoraro]

Comment on attachment 342190 [details]
Patch

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

>>> Source/WTF/wtf/cocoa/Entitlements.cpp:40
>>> +    auto cfEntitlement = adoptCF(CFStringCreateWithCString(CFAllocatorGetDefault(), entitlement, kCFStringEncodingUTF8));
>> 
>> We can just use kCFAllocatorDefault here. But also, would it make more sense for processHasEntitlement() to take a WTF String?
> 
> I think it's equally valid to just take a C string (as is) in this case, unless you know of a specific reason to switch to a WTF String.

Lets just keep a C string to cut down on conversions. Everywhere we call it now we use some literal anyways.

> Source/WebKit/Shared/mac/SandboxUtilities.mm:80
>  bool processHasEntitlement(NSString *entitlement)

I'd suggest just replacing the existing call sites of:

    WebKit::processHasEntitlement

With your new WTF version and removing the duplication.

[Joseph Pecoraro]

Comment on attachment 342190 [details]
Patch

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

>>>>> Source/WTF/wtf/cocoa/Entitlements.cpp:40
>>>>> +    auto cfEntitlement = adoptCF(CFStringCreateWithCString(CFAllocatorGetDefault(), entitlement, kCFStringEncodingUTF8));
>>>> 
>>>> We can just use kCFAllocatorDefault here. But also, would it make more sense for processHasEntitlement() to take a WTF String?
>>> 
>>> I think it's equally valid to just take a C string (as is) in this case, unless you know of a specific reason to switch to a WTF String.
>> 
>> I made the comment because, just looking at this patch, I see an existing caller in possession of an NSString. That caller will now have to get a C string out of it, and then the callee will end up allocating a brand new CFString for it. Further (and perhaps this can be fixed independently), the callee is not even sharing the string buffer but instead copying it. For the new caller being introduced in this patch, I think either interface (C string or WTF String) would work just as well.
> 
> Lets just keep a C string to cut down on conversions. Everywhere we call it now we use some literal anyways.

The existing callsites using an NSString should just go away. Those callsites should just use WTF::processHasEntitlement and WebKit::processHasEntitlement should go away. Everyone I saw just uses literals when calling this. C string or WTF String are fine. Though given WTF -> CFString would do CFStringCreateWithCStringNoCopy you could probably use that here if you don't use WTF String.

[Tadeu Zagallo]

Created attachment 342202 [details]
Patch

[Mark Lam]

Comment on attachment 342202 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 342202 [details]
Patch

Rejecting attachment 342202 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
no-shadow -Wno-four-char-constants -Wno-conversion -Wconstant-conversion -Wint-conversion -Wbool-conversion -Wenum-conversion -Wsign-compare -Wno-shorten-64-to-32 -Wnewline-eof -Wno-c++11-extensions -DNDEBUG -DENABLE_3D_TRANSFORMS -DENABLE_APPLE_PAY -DENABLE_APPLICATION_MANIFEST -DENABLE_ATTACHMENT_ELEMENT -DENABLE_AVF_CAPTIONS -DENABLE_CACHE_PARTITIONING -DENABLE_CHANNEL_MESSAGING -DENABLE_CONTENT_FILTERING -DENABLE_CSS_ANIMATIONS_LEVEL_2 -DENABLE_CSS_BOX_DECORATION_BREAK -DENABLE_CSS_COMPOSITING -DENABLE_CSS_SCROLL_SNAP -DENABLE_CSS_SELECTORS_LEVEL4 -DENABLE_CSS_TRAILING_WORD -DENABLE_CURSOR_VISIBILITY -DENABLE_DASHBOARD_SUPPORT -DENABLE_DATACUE_VALUE -DENABLE_EXPERIMENTAL_FEATURES -DENABLE_FILTERS_LEVEL_2 -DENABLE_FTL_JIT -DENABLE_FULLSCREEN_API -DENABLE_GAMEPAD -DENABLE_GEOLOCATION -DENABLE_ICONDATABASE -DENABLE_INDEXED_DATABASE -DENABLE_INDEXED_DATABASE_IN_WORKERS -DENABLE_INTERSECTION_OBSERVER -DENABLE_INTL -DENABLE_INTL_NUMBER_FORMAT_TO_PARTS -DENABLE_INTL_PLURAL_RULES -DENABLE_JS_ASYNC_ITERATION -DENABLE_KEYBOARD_CODE_ATTRIBUTE -DENABLE_KEYBOARD_KEY_ATTRIBUTE -DENABLE_LEGACY_CSS_VENDOR_PREFIXES -DENABLE_LEGACY_CUSTOM_PROTOCOL_MANAGER -DENABLE_LEGACY_ENCRYPTED_MEDIA -DENABLE_MATHML -DENABLE_MEDIA_CONTROLS_SCRIPT -DENABLE_MEDIA_SOURCE -DENABLE_MEDIA_STREAM -DENABLE_METER_ELEMENT -DENABLE_MOUSE_CURSOR_SCALE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SUBTLE_CRYPTO -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_USERSELECT_ALL -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_RTC -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.12 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore -I. -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/System.framework/PrivateHeaders --system-header-prefix=unicode/ -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/JavaScriptCorePrefix-ceviypvlpozbbhfgkscwtnuodydm/JavaScriptCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource80.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource80.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource80.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource80.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource2-mm.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource2-mm.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/8063521

[Tadeu Zagallo]

Created attachment 342206 [details]
Patch for landing

[Saam Barati]

Comment on attachment 342206 [details]
Patch for landing

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

> Source/WebKit/ChangeLog:10
> +        Check that the current process has the correct entitlements before
> +        trying to allocate JIT memory to silence warnings.

style nit: You shouldn't copy-paste the same changelog text in every changelog. The different changelog entries should contain what you did in that subdirectory of the project.

So this should say something like:
use the new WTF API for this instead of the one defined in WebKit.

> Source/WebKit/ChangeLog:13
> +        (WebKit::processHasEntitlement): Move the implementation down to WTF to share with JavaScriptCore

This seems out of date.

[Tadeu Zagallo]

Created attachment 342208 [details]
Patch for landing

[Saam Barati]

Comment on attachment 342208 [details]
Patch for landing

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

> Source/WTF/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

needs to be updated (ditto below)

[Tadeu Zagallo]

Comment on attachment 342208 [details]
Patch for landing

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

>> Source/WTF/ChangeLog:7
>> +        Reviewed by NOBODY (OOPS!).
> 
> needs to be updated (ditto below)

Commit queue should automatically update it, right?

[Saam Barati]

(In reply to Tadeu Zagallo from comment #19)
> Comment on attachment 342208 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342208&action=review
> 
> >> Source/WTF/ChangeLog:7
> >> +        Reviewed by NOBODY (OOPS!).
> > 
> > needs to be updated (ditto below)
> 
> Commit queue should automatically update it, right?

Only if someone r+ the patch first.

[Tadeu Zagallo]

Created attachment 342212 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 342212 [details]
Patch for landing

Clearing flags on attachment: 342212

Committed r232604: <https://trac.webkit.org/changeset/232604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.