Bug 105674 – Ensure autorelease pool exists when calling WKNSProcessInfoProcessAssertionWithTypes on Mac

• Component: WebKit2
• Status: RESOLVED FIXED

[Kiran Muppala]

WebKit2 child processes on Mac call the WebKitSystemInterface function WKNSProcessInfoProcessAssertionWithTypes, to take a process visible assertion during initialization, which for non XPC Service processes such as NetworkProcess, PluginProcess happens prior to the main runloop being started and hence a autorelease pool being established.  Thus, the autoreleased assertion object returned by the first call is leaked and hence the process does not subsequently go to sleep even after releasing the assertion.

To ensure the assertion object is correctly released, the call to the above function should be enclosed by a @autoreleasepool block.

[Kiran Muppala]

<rdar://problem/12928750>

[Kiran Muppala]

Created attachment 180585 [details]
Patch

[Sam Weinig]

I don't see how this is being called before initialization.  Can you point to where that is being done?

[Kiran Muppala]

(In reply to comment #3)
> I don't see how this is being called before initialization.  Can you point to where that is being done?

Thanks for taking a look at this patch.  The very first call to ChildProcess::setApplicationIsOccluded() happens from ChildProcess::platformInitialize().  Which in turn happens before the runloop starts.  For example, here is the bottom portion of NetworkProcessMain() from NetworkProcessMainMac.mm

    InitWebCoreSystemInterface();
    JSC::initializeThreading();
    WTF::initializeMainThread();
    RunLoop::initializeMainRunLoop();

    // Initialize the network process connection.
    NetworkProcess::shared().initialize(CoreIPC::Connection::Identifier(serverPort), RunLoop::main());

    [NSApplication sharedApplication];

    RunLoop::run();

    return 0;
} 

The call to NetworkProcess::shared() happens prior to RunLoop::run().  The former call results in ChildProcess object being constructed, which in turn calls ChildProcess::platformInitialize().

WebProcess doesn't run into this because xpc_main itself starts the runloop.  On the flip side, WebProcess also doesn't take an assertion until the bootstrap message is received.  It is preferable to take the assertion prior to the runloop starting so that the process doesn't need a subsequent message to keep it from being suppressed.

[Sam Weinig]

Ah! In that case, I think it would better to add the @autoreleasepool to NetworkProcessMain.  We already have this in WebProcessInitialization.mm.

Why do we wait for the bootstrap message to do this if it is preferable to do it earlier?

[Kiran Muppala]

(In reply to comment #5)
> Ah! In that case, I think it would better to add the @autoreleasepool to NetworkProcessMain.  We already have this in WebProcessInitialization.mm.

I would have to do that for PluginProcess and SharedWorkerProcess too.  Also, it won't be easy to tell why the pool is there.  In WebProcessInitialization.mm the call to [NSString stringWithFormat:] points to the reason immediately.  May be you already considered these.  Would you still prefer the pool be moved to the main routine of the respective processes?  I could also push CoreFoundation folks to provide a non autorelease version of the assertion, or even a plain old c function, just to throw another option.

> 
> Why do we wait for the bootstrap message to do this if it is preferable to do it earlier?

This is purely a limitation introduced by the assertion method being SPI and not API.  WebProcessServiceForWebKitDevelopment does not load WKSI until the bootstrap message is received.  But, to receive the message xpc_main() has to be called in the first place.  WebProcessService doesn't have this problem, so I could take an assertion there before xpc_main().  But, since it is not critical do to that, the process will be un suppressed by a incoming xpc message, both services take an assertion when handling the bootstrap message.

[Kiran Muppala]

(In reply to comment #5)
> Ah! In that case, I think it would better to add the @autoreleasepool to NetworkProcessMain.  We already have this in WebProcessInitialization.mm.
> 
Oh, I thought of another option.  How about I move the @autoreleasepool to ChildProcess::platformInitialize?  That way the cost is only incurred once, and I don't have to paste it in three different locations.

[Sam Weinig]

(In reply to comment #7)
> (In reply to comment #5)
> > Ah! In that case, I think it would better to add the @autoreleasepool to NetworkProcessMain.  We already have this in WebProcessInitialization.mm.
> > 
> Oh, I thought of another option.  How about I move the @autoreleasepool to ChildProcess::platformInitialize?  That way the cost is only incurred once, and I don't have to paste it in three different locations.

I think doing it in NetworkProcessMain (and in the other two as well) is the best option.

[Kiran Muppala]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Ah! In that case, I think it would better to add the @autoreleasepool to NetworkProcessMain.  We already have this in WebProcessInitialization.mm.
> > > 
> > Oh, I thought of another option.  How about I move the @autoreleasepool to ChildProcess::platformInitialize?  That way the cost is only incurred once, and I don't have to paste it in three different locations.
> 
> I think doing it in NetworkProcessMain (and in the other two as well) is the best option.

Ok, will upload a updated patch.  Thanks.

[Kiran Muppala]

Created attachment 181088 [details]
Patch

[Kiran Muppala]

(In reply to comment #10)
> Created an attachment (id=181088) [details]
> Patch

Moved @autoreleasepool block to main process initialization method of NetworkProcess, PluginProcess and SharedWorkerProcess.

[Kiran Muppala]

Created attachment 181106 [details]
Patch

[Kiran Muppala]

(In reply to comment #12)
> Created an attachment (id=181106) [details]
> Patch

NetworkProcess initialization code was moved to a separate method with an autorelease pool by https://bugs.webkit.org/show_bug.cgi?id=105946, hence removed changes to NetworkProcess in this patch, which also fixed the merge conflict with previous patch.

[Kiran Muppala]

Created attachment 181108 [details]
Patch

[Kiran Muppala]

(In reply to comment #14)
> Created an attachment (id=181108) [details]
> Patch

Removed unnecessary white space.  No functional change.

[WebKit Review Bot]

Comment on attachment 181108 [details]
Patch

Clearing flags on attachment: 181108

Committed r138752: <http://trac.webkit.org/changeset/138752>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.