Bug 220663 – [GPUProcess] Mark IOSurface backing for ImageBufferBackends as owned by the WebProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Claim ownership of IOSurface backing for ImageBufferBackends in the WebProcess. If we don't do this, all IOSurface memory gets attributed to the GPUProcess, making it a prime candidate for jetsam.
We need to attribute memory usage to the WebProcesses being served by the GPUProcess as much as possible and this is a step in this direction.

[Chris Dumez]

Created attachment 417719 [details]
Patch

[Tim Horton]

Comment on attachment 417719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417719&action=review

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:52
> +    if (result != kIOReturnSuccess)

This should be in ImageBufferShareableMappedIOSurfaceBackend, and /only/ in ImageBufferShareableMappedIOSurfaceBackend. ImageBufferShareableIOSurfaceBackend (the unmapped variant) by definition must not IOSurfaceLookupFromMachPort.

[jared]

i need to get some info about this

[Chris Dumez]

(In reply to Tim Horton from comment #2)
> Comment on attachment 417719 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417719&action=review
> 
> > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:52
> > +    if (result != kIOReturnSuccess)
> 
> This should be in ImageBufferShareableMappedIOSurfaceBackend, and /only/ in
> ImageBufferShareableMappedIOSurfaceBackend.
> ImageBufferShareableIOSurfaceBackend (the unmapped variant) by definition
> must not IOSurfaceLookupFromMachPort.

This means that with DOM rendering enabled, we will not have a short term fix for view tiles. I understand that in the long term, we want to stop using IOSurface in the WebProcess. However, in the short term, my proposal was to do this (which works for now).

[Tim Horton]

Comment on attachment 417719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417719&action=review

>>> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:52
>>> +    if (result != kIOReturnSuccess)
>> 
>> This should be in ImageBufferShareableMappedIOSurfaceBackend, and /only/ in ImageBufferShareableMappedIOSurfaceBackend. ImageBufferShareableIOSurfaceBackend (the unmapped variant) by definition must not IOSurfaceLookupFromMachPort.
> 
> This means that with DOM rendering enabled, we will not have a short term fix for view tiles. I understand that in the long term, we want to stop using IOSurface in the WebProcess. However, in the short term, my proposal was to do this (which works for now).

Like I said on Slack (but will write here for wider audience), if you want to do this, you should just make DOM use the mapped backend (temporarily) instead of making the non-mapped case map the surfaces :)

[Chris Dumez]

Created attachment 417722 [details]
Patch

[Chris Dumez]

Created attachment 417723 [details]
Patch

[Chris Dumez]

Created attachment 417727 [details]
Patch

[Chris Dumez]

Created attachment 417752 [details]
WIP Patch

Ok, I have it working from the GPUProcess this time (no need for IOSurface calls in the WebProcess at all). Much better solution for the long term.

I still need to do some clean up: It may look nicer to transfer the ownership in RemoteRenderingBackend::createImageBuffer() or somewhere else that has access to both the PID and IOSurface, instead of doing it in RemoteRenderingBackend::didCreateImageBufferBackend().
Doing it in RemoteRenderingBackend::didCreateImageBufferBackend() is convenient but it needs I need to look up the IOSurface from the handle again.

[Chris Dumez]

Created attachment 417879 [details]
WIP Patch

[Chris Dumez]

Created attachment 417883 [details]
Patch

[Chris Dumez]

Created attachment 417884 [details]
Patch

[Geoffrey Garen]

Comment on attachment 417884 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 417884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417884&action=review

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb:910
> +(allow mach-priv-task-port)

We probably need to find a way to reduce the scope of sandbox change here. This is relatively high privilege thing.

[Geoffrey Garen]

Comment on attachment 417727 [details]
Patch

r=me

[Chris Dumez]

One API test failure on open source api bot:

TestWebKitAPI.GPUProcess.CanvasBasicCrashHandling
        dyld: lazy symbol binding failed: Symbol not found: _IOSurfaceSetOwnership
          Referenced from: /Volumes/Data/worker/API-Tests-iOS-Simulator-EWS/build/WebKitBuild/Release-iphonesimulator/WebKit.framework/WebKit
          Expected in: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks/IOSurface.framework/IOSurface
        
        dyld: Symbol not found: _IOSurfaceSetOwnership
          Referenced from: /Volumes/Data/worker/API-Tests-iOS-Simulator-EWS/build/WebKitBuild/Release-iphonesimulator/WebKit.framework/WebKit
          Expected in: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks/IOSurface.framework/IOSurface

[Chris Dumez]

Created attachment 417912 [details]
Patch

[EWS]

Committed r271627: <https://trac.webkit.org/changeset/271627>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417912 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73378219>