Bug 182605

Summary: Silence MAP_JIT warning for Network Process
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, joepeck, mark.lam, mitz, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Description Brent Fulgham 2018-02-08 08:29:53 PST
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.
Comment 1 Alexey Proskuryakov 2018-02-15 10:58:57 PST
30354260
Comment 2 Brent Fulgham 2018-03-09 09:34:30 PST
<rdar://problem/35368975>
Comment 3 Brent Fulgham 2018-03-09 09:34:55 PST
I meant this:
<rdar://problem/30354260>
Comment 4 Mark Lam 2018-06-06 16:04:38 PDT
<rdar://problem/38271229>
Comment 5 Tadeu Zagallo 2018-06-07 11:00:03 PDT
Created attachment 342190 [details]
Patch
Comment 6 mitz 2018-06-07 11:16:12 PDT
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?
Comment 7 Mark Lam 2018-06-07 11:16:30 PDT
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.
Comment 8 Mark Lam 2018-06-07 11:19:48 PDT
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.
Comment 9 mitz 2018-06-07 11:28:28 PDT
(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.
Comment 10 Joseph Pecoraro 2018-06-07 11:28:50 PDT
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.
Comment 11 Joseph Pecoraro 2018-06-07 12:03:51 PDT
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.
Comment 12 Tadeu Zagallo 2018-06-07 12:44:12 PDT
Created attachment 342202 [details]
Patch
Comment 13 Mark Lam 2018-06-07 12:47:15 PDT
Comment on attachment 342202 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2018-06-07 13:00:03 PDT
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
Comment 15 Tadeu Zagallo 2018-06-07 14:22:25 PDT
Created attachment 342206 [details]
Patch for landing
Comment 16 Saam Barati 2018-06-07 14:38:47 PDT
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.
Comment 17 Tadeu Zagallo 2018-06-07 14:58:31 PDT
Created attachment 342208 [details]
Patch for landing
Comment 18 Saam Barati 2018-06-07 14:59:29 PDT
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)
Comment 19 Tadeu Zagallo 2018-06-07 15:16:03 PDT
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?
Comment 20 Saam Barati 2018-06-07 15:21:47 PDT
(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.
Comment 21 Tadeu Zagallo 2018-06-07 15:28:44 PDT
Created attachment 342212 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2018-06-07 16:14:10 PDT
Comment on attachment 342212 [details]
Patch for landing

Clearing flags on attachment: 342212

Committed r232604: <https://trac.webkit.org/changeset/232604>
Comment 23 WebKit Commit Bot 2018-06-07 16:14:12 PDT
All reviewed patches have been landed.  Closing bug.