Bug 141139 – Use XPC for executable heap plugin services

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Sam Weinig]

Use XPC for executable heap plugin services

[Sam Weinig]

Created attachment 245838 [details]
Patch

[Alexey Proskuryakov]

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?

[Sam Weinig]

(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.

[Darin Adler]

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.

[Sam Weinig]

We don't need to do this anymore.