| Summary: | [GPUProcess] Mark IOSurface backing for ImageBufferBackends as owned by the WebProcess | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||
| Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, cmarcelo, ews-watchlist, ggaren, kings14x, sabouhallawa, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=220770 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Chris Dumez
2021-01-15 12:08:18 PST
Created attachment 417719 [details]
Patch
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. i need to get some info about this (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). 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 :) Created attachment 417722 [details]
Patch
Created attachment 417723 [details]
Patch
Created attachment 417727 [details]
Patch
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.
Created attachment 417879 [details]
WIP Patch
Created attachment 417883 [details]
Patch
Created attachment 417884 [details]
Patch
Comment on attachment 417884 [details]
Patch
r=me
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. Comment on attachment 417727 [details]
Patch
r=me
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
Created attachment 417912 [details]
Patch
Committed r271627: <https://trac.webkit.org/changeset/271627> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417912 [details]. |