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.
30354260
<rdar://problem/35368975>
I meant this: <rdar://problem/30354260>
<rdar://problem/38271229>
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?
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 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.
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 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.
Created attachment 342202 [details] Patch
Comment on attachment 342202 [details] Patch r=me
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
Created attachment 342206 [details] Patch for landing
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.
Created attachment 342208 [details] Patch for landing
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 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?
(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.
Created attachment 342212 [details] Patch for landing
Comment on attachment 342212 [details] Patch for landing Clearing flags on attachment: 342212 Committed r232604: <https://trac.webkit.org/changeset/232604>
All reviewed patches have been landed. Closing bug.