Use XPC for executable heap plugin services
Created attachment 245838 [details] Patch
Comment on attachment 245838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245838&action=review > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:188 > + if (launchOptions.executableHeap) > + return "com.apple.WebKit.Plugin.32.ExecutableHeap"; Shouldn't we do this in forDevelopment mode too?
(In reply to comment #2) > Comment on attachment 245838 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245838&action=review > > > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:188 > > + if (launchOptions.executableHeap) > > + return "com.apple.WebKit.Plugin.32.ExecutableHeap"; > > Shouldn't we do this in forDevelopment mode too? I don't think we need to. In forDevelopment, we should already re-exec and set the executable heap bit correctly.
Comment on attachment 245838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245838&action=review I suppose I’m not really qualified to review, but I did look it over. > Source/WebKit2/Configurations/PluginService.32.ExecutableHeap.xcconfig:1 > +// Copyright (C) 2013 Apple Inc. All rights reserved. 2013? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:2 > + * Copyright (C) 2013 Apple Inc. All rights reserved. 2013? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:106 > + if (!strcmp(xpc_dictionary_get_string(event, "message-name"), "re-exec")) { No need for null check? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:107 > + ReexecInfo *info = static_cast<ReexecInfo *>(malloc(sizeof(ReexecInfo))); Why malloc instead of new? I don’t se where this gets freed so I assume it intentionally leaks, and seems no real reason to chose malloc over new in that case. > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:121 > + CFRunLoopTimerRef timer = CFRunLoopTimerCreate(NULL, CFAbsoluteTimeGetCurrent(), 0, 0, 0, reexecCallBack, &context); Looks like this timer leaks. How about a CFRelease? I guess we are cavalier about memory since we are going to reexec, but I do see things like adoptOSObject below so I think we are not entirely in the Wild West here. > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:132 > + CFBundleRef webKitBundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebKit")); No need for null check here? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:133 > + CFStringRef entryPointFunctionName = (CFStringRef)CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), CFSTR("WebKitEntryPoint")); No need for null check or type check here? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:136 > + InitializerFunction initializerFunctionPtr = reinterpret_cast<InitializerFunction>(CFBundleGetFunctionPointerForName(webKitBundle, entryPointFunctionName)); Why not use auto so we don’t have to repeat the type name twice? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:153 > + if (!strcmp(xpc_dictionary_get_string(event, "message-name"), "pre-bootstrap")) No need for null check here? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:101 > CFRunLoopTimerContext context = { 0, info, NULL, NULL, NULL }; Why still create the context if it’s not used? > Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.ExecutableHeap.mm:102 > + CFRunLoopTimerRef timer = CFRunLoopTimerCreate(NULL, CFAbsoluteTimeGetCurrent(), 0, 0, 0, reexecCallBack, 0); Looks like the same timer leak again. Also, why 0 instead of nullptr? > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:396 > + // Other end of ref called before we setup the event handler. "set up" should be two words here.
We don't need to do this anymore.