Bug 222391 – Set ownership of IOSurfaces from the GPUProcess instead of the WebProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Set ownership of IOSurfaces from the GPUProcess instead of the WebProcess. The WebProcess should stop using IOSurface API/SPI for better sandboxing.

[Chris Dumez]

Created attachment 421484 [details]
WIP Patch

Will check the processes memory footprint tomorrow but at least the ownership transfer calls are running in the GPUProcess and succeeding \o/

[Chris Dumez]

Created attachment 421533 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 421533 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=222391

Radar

> Source/WTF/wtf/PlatformHave.h:307
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 120000) \
> +    || (((PLATFORM(IOS) && !PLATFORM(IOS_FAMILY_SIMULATOR)) || PLATFORM(MACCATALYST)) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000) \
> +    || (PLATFORM(WATCHOS) && !PLATFORM(IOS_FAMILY_SIMULATOR) && __WATCH_OS_VERSION_MIN_REQUIRED >= 80000) \
> +    || (PLATFORM(APPLETV) && !PLATFORM(IOS_FAMILY_SIMULATOR) && __TV_OS_VERSION_MIN_REQUIRED >= 150000)

These are getting out of control.

[Radar WebKit Bug Importer]

<rdar://problem/74748353>

[Chris Dumez]

Created attachment 421536 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 421536 [details]
Patch

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

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:65
> +#if HAVE(IOSURFACE_SET_OWNERSHIP_IDENTITY)
> +    void setProcessOwnership(task_id_token_t newOwner)
> +    {
> +        if (m_backend)
> +            m_backend->setProcessOwnership(newOwner);
> +    }
> +#endif

I think this should be moved to RemoteImageBuffer.

> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:226
> +#if HAVE(IOSURFACE_SET_OWNERSHIP_IDENTITY)
> +void ImageBufferIOSurfaceBackend::setProcessOwnership(task_id_token_t processIdentityToken)
> +{
> +    ASSERT(m_surface);
> +    m_surface->setOwnershipIdentity(processIdentityToken);
> +}
> +#endif

I think this should be moved to ImageBufferShareableMappedIOSurfaceBackend.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 421533 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421533&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=222391
> 
> Radar
> 
> > Source/WTF/wtf/PlatformHave.h:307
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 120000) \
> > +    || (((PLATFORM(IOS) && !PLATFORM(IOS_FAMILY_SIMULATOR)) || PLATFORM(MACCATALYST)) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000) \
> > +    || (PLATFORM(WATCHOS) && !PLATFORM(IOS_FAMILY_SIMULATOR) && __WATCH_OS_VERSION_MIN_REQUIRED >= 80000) \
> > +    || (PLATFORM(APPLETV) && !PLATFORM(IOS_FAMILY_SIMULATOR) && __TV_OS_VERSION_MIN_REQUIRED >= 150000)
> 
> These are getting out of control.

Probably would have said !SIM only once, at the end, but otherwise this is sadly a perfect model of how it should be done for everyone :)

[Chris Dumez]

(In reply to Said Abou-Hallawa from comment #6)
> Comment on attachment 421536 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421536&action=review
> 
> > Source/WebCore/platform/graphics/ConcreteImageBuffer.h:65
> > +#if HAVE(IOSURFACE_SET_OWNERSHIP_IDENTITY)
> > +    void setProcessOwnership(task_id_token_t newOwner)
> > +    {
> > +        if (m_backend)
> > +            m_backend->setProcessOwnership(newOwner);
> > +    }
> > +#endif
> 
> I think this should be moved to RemoteImageBuffer.
> 
> > Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:226
> > +#if HAVE(IOSURFACE_SET_OWNERSHIP_IDENTITY)
> > +void ImageBufferIOSurfaceBackend::setProcessOwnership(task_id_token_t processIdentityToken)
> > +{
> > +    ASSERT(m_surface);
> > +    m_surface->setOwnershipIdentity(processIdentityToken);
> > +}
> > +#endif
> 
> I think this should be moved to ImageBufferShareableMappedIOSurfaceBackend.

I don't get it, it seems ImageBufferShareableMappedIOSurfaceBackend has a ImageBufferBackendHandle but I don't see an IOSurface. I need an IOSurface to set ownership.
How do I do that from ImageBufferShareableMappedIOSurfaceBackend?

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Said Abou-Hallawa from comment #6)
> > Comment on attachment 421536 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=421536&action=review
> > 
> > > Source/WebCore/platform/graphics/ConcreteImageBuffer.h:65
> > > +#if HAVE(IOSURFACE_SET_OWNERSHIP_IDENTITY)
> > > +    void setProcessOwnership(task_id_token_t newOwner)
> > > +    {
> > > +        if (m_backend)
> > > +            m_backend->setProcessOwnership(newOwner);
> > > +    }
> > > +#endif
> > 
> > I think this should be moved to RemoteImageBuffer.
> > 
> > > Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:226
> > > +#if HAVE(IOSURFACE_SET_OWNERSHIP_IDENTITY)
> > > +void ImageBufferIOSurfaceBackend::setProcessOwnership(task_id_token_t processIdentityToken)
> > > +{
> > > +    ASSERT(m_surface);
> > > +    m_surface->setOwnershipIdentity(processIdentityToken);
> > > +}
> > > +#endif
> > 
> > I think this should be moved to ImageBufferShareableMappedIOSurfaceBackend.
> 
> I don't get it, it seems ImageBufferShareableMappedIOSurfaceBackend has a
> ImageBufferBackendHandle but I don't see an IOSurface. I need an IOSurface
> to set ownership.
> How do I do that from ImageBufferShareableMappedIOSurfaceBackend?

Oh, looks like I got confused with ImageBufferShareableIOSurfaceBackend. Let me give that another try.

[Chris Dumez]

Created attachment 421550 [details]
Patch

[Chris Dumez]

Created attachment 421564 [details]
Patch

[EWS]

Committed r273655: <https://commits.webkit.org/r273655>

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

[Truitt Savell]

Reverted r273655 for reason:

Broke internal Mac

Committed r273700 (234721@main): <https://commits.webkit.org/234721@main>

[Chris Dumez]

Created attachment 422247 [details]
Patch

[EWS]

Committed r273903: <https://commits.webkit.org/r273903>

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