Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GraphicsLayerWC: site isolation support #12723

Merged

Conversation

fujii
Copy link
Contributor

@fujii fujii commented Apr 14, 2023

dc5a423

GraphicsLayerWC: site isolation support
https://bugs.webkit.org/show_bug.cgi?id=254902

Reviewed by Don Olmstead.

This is the initial support of site isolation for GraphicsLayerWC.
Cocoa port (aka RemoteLayerTree and GraphicsLayerCARemote) transfers a
layer tree to UI process, GraphicsLayerWC transfers a layer tree to
GPU process. GPU process renders a remote child frame into a texture,
and attach the texture into the content of iframe layer of the parent
frame's layer tree. Added WCRemoteFrameHostLayerManager class to
manage a HashMap from a LayerHostingContextIdentifier to a
BitmapTexture.

This has a problem now. Updating remote child frame's layer tree
doesn't trigger the parent frame's repaint. For the workaround, you
need to manually click a browser window of MiniBrowser to repaint.
Keep http/tests/site-isolation tests skipped for WinCairo.

MiniBrowser has to enable SiteIsolationEnabled option to test it by
using the internal settings menu item or adding the following code.
> WKPreferencesSetBoolValueForKeyForTesting(preferences.get(), true, createWKString("SiteIsolationEnabled").get());

Fixed a bug of TextureMapperGL::endPainting. It should restore a
previous frame buffer that TextureMapperGL::beginPainting saved.
TextureMapperGL supports the offscreen rendering mode for
WebKitTestRunner pixel dump mode. It's not a problem for
WebKitTestRunner because it renders a layer tree always to a texture.
In site-isolation, however, remote frame's layer tree renders to a
texture and main frame layer tree to a frame buffer. After rendering
to a texture, it has to restore the previous frame buffer.

* Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:
(WebCore::TextureMapperGL::endPainting): Restore the previous framebuffer.
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:
(WebKit::GPUConnectionToWebProcess::didClose):
* Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:
(WebKit::remoteGraphicsStreamWorkQueue):
* Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.h:
* Source/WebKit/GPUProcess/graphics/wc/WCRemoteFrameHostLayerManager.cpp: Added.
(WebKit::WCRemoteFrameHostLayerManager::singleton):
(WebKit::WCRemoteFrameHostLayerManager::acquireRemoteFrameHostLayer):
(WebKit::WCRemoteFrameHostLayerManager::releaseRemoteFrameHostLayer):
(WebKit::WCRemoteFrameHostLayerManager::updateTexture):
(WebKit::WCRemoteFrameHostLayerManager::removeAllLayersForProcess):
* Source/WebKit/GPUProcess/graphics/wc/WCRemoteFrameHostLayerManager.h: Added.
* Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:
(WebKit::WCScene::update):
* Source/WebKit/PlatformPlayStation.cmake:
* Source/WebKit/PlatformWin.cmake:
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.cpp:
(WebKit::DrawingAreaWC::DrawingAreaWC):
(WebKit::DrawingAreaWC::updateRootLayers):
(WebKit::DrawingAreaWC::setRootCompositingLayer):
(WebKit::DrawingAreaWC::addRootFrame):
(WebKit::DrawingAreaWC::attachViewOverlayGraphicsLayer):
(WebKit::DrawingAreaWC::updatePreferences):
(WebKit::DrawingAreaWC::shouldUseTiledBackingForFrameView const):
(WebKit::DrawingAreaWC::updateGeometryWC):
(WebKit::DrawingAreaWC::isCompositingMode):
(WebKit::DrawingAreaWC::sendUpdateAC):
(WebKit::DrawingAreaWC::rootLayerInfoWithFrameIdentifier):
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.h:
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:
(WebKit::GraphicsLayerWC::setContentsToPlatformLayerHost):
(WebKit::GraphicsLayerWC::flushCompositingStateForThisLayerOnly):
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.h:
* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.h:
(WebKit::WCLayerUpdateInfo::encode const):
(WebKit::WCLayerUpdateInfo::decode):
(WebKit::WCUpdateInfo::encode const):
(WebKit::WCUpdateInfo::decode):

Canonical link: https://commits.webkit.org/265099@main

f90ac07

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt   πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2   πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@fujii fujii requested a review from cdumez as a code owner April 14, 2023 02:53
@fujii fujii self-assigned this Apr 14, 2023
@fujii fujii added the WebKit2 Bugs relating to the WebKit2 API layer label Apr 14, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 14, 2023
@fujii fujii removed the merging-blocked Applied to prevent a change from being merged label Apr 14, 2023
@fujii fujii force-pushed the eng/GraphicsLayerWC-site-isolation branch from 103c71f to 9063076 Compare April 14, 2023 06:29
@fujii fujii marked this pull request as draft May 10, 2023 00:02
@fujii fujii force-pushed the eng/GraphicsLayerWC-site-isolation branch from 9063076 to ff68a79 Compare June 2, 2023 06:29
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 2, 2023
@fujii fujii removed the merging-blocked Applied to prevent a change from being merged label Jun 7, 2023
@fujii fujii force-pushed the eng/GraphicsLayerWC-site-isolation branch from ff68a79 to 5f75b6f Compare June 7, 2023 02:19
@fujii fujii marked this pull request as ready for review June 7, 2023 02:23
Copy link
Contributor

@achristensen07 achristensen07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for being engaged in this development. It is good to know that site isolation won't be just a Cocoa-specific development, which is important for the health of the project.

Do you know what it would take to get it so that we could unskip the test and have the drawing driven in all processes? That would probably be worth finding out. Also, the test times out occasionally on macOS for an unknown reason, and I am hoping to fix that soonish.

For a currently unknown reason, on macOS one isolated iframe draws fine but the second isolated iframe does not draw. I was hoping to figure that out and fix it before cementing the architecture in even more by adding another port's implementation.

One thing I had to make sure I did right is attach the layer and sublayer correctly if the parent layer was created first and if the child layer was created first, which required two places where we stitched layers together. I'm not sure I see that in your code.

Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.cpp Outdated Show resolved Hide resolved
rootLayer.viewOverlayRootLayer->flushCompositingState(visibleRect);

m_commitQueue->dispatch([this, weakThis = WeakPtr(*this), stateID = m_backingStoreStateID, updateInfo = std::exchange(m_updateInfo, { }), willCallDisplayDidRefresh]() mutable {
flushLayerImageBuffers(updateInfo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably check weakThis before using this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WeakPtr isn't thread safe. I should nothing for the WeakPtr in another thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the fix.

Source/WebKit/WebProcess/WebPage/wc/WCUpateInfo.h Outdated Show resolved Hide resolved
@fujii
Copy link
Contributor Author

fujii commented Jun 7, 2023

Do you know what it would take to get it so that we could unskip the test and have the drawing driven in all processes? That would probably be worth finding out. Also, the test times out occasionally on macOS for an unknown reason, and I am hoping to fix that soonish.

For the WinCairo problem, I will do it as as a follow-up.
I don't know WinCairo problem also have the same timeout problem because WinCairo isn't able to test yet.

For a currently unknown reason, on macOS one isolated iframe draws fine but the second isolated iframe does not draw. I was hoping to figure that out and fix it before cementing the architecture in even more by adding another port's implementation.

Do you mean a page containing two cross-site iframes or two pages containing a cross-site iframe?
For WinCairo, I think it works fine for a page containing two cross-site iframes, but it's hitting an assertion failure for the case of two pages containing a cross-site iframe.
WebPageProxy::resetState destroys a DrawingArea after a page navigation, however the DrawingArea is still listening IPC message because SubframePageProxy calls startReceivingMessages but it's in a page cache.

One thing I had to make sure I did right is attach the layer and sublayer correctly if the parent layer was created first and if the child layer was created first, which required two places where we stitched layers together. I'm not sure I see that in your code.

This patch supports both scenarios.
If a parent layer is created before the remote frame layer tree is created,

  1. The parent layer creates a new WCRemoteFrameHostLayer with WCRemoteFrameHostLayerManager::acquireRemoteFrameHostLayer
  2. The remote frame layer tree renders to a texture and set it to the WCRemoteFrameHostLayer by using WCRemoteFrameHostLayerManager::updateTexture

If a remote frame layer tree is created before the parent layer is created

  1. The remote frame layer tree renders to a texture and create a new WCRemoteFrameHostLayer and set a texture by using WCRemoteFrameHostLayerManager::updateTexture
  2. The parent layer takes the ownership of the WCRemoteFrameHostLayer by using WCRemoteFrameHostLayerManager::acquireRemoteFrameHostLayer

@fujii fujii force-pushed the eng/GraphicsLayerWC-site-isolation branch from 5f75b6f to e3fdd98 Compare June 8, 2023 01:34
@fujii fujii force-pushed the eng/GraphicsLayerWC-site-isolation branch from e3fdd98 to 62db346 Compare June 8, 2023 21:49
@fujii
Copy link
Contributor Author

fujii commented Jun 8, 2023

For the WinCairo problem, I will do it as as a follow-up. I don't know WinCairo problem also have the same timeout problem because WinCairo isn't able to test yet.

It's able to run the test. Due to the repainting problem, it is flaky test failure. But, no timeout failure.

python.exe ./Tools/Scripts/run-webkit-tests --debug --no-retry-failures http/tests/site-isolation/basic-iframe.html --iterations=1000 -f --run-singly

It tuned out web process is leaking. The number of web processes is gradually increasing and crash eventually without --run-singly.

@fujii fujii force-pushed the eng/GraphicsLayerWC-site-isolation branch from 62db346 to f90ac07 Compare June 13, 2023 02:22
@fujii fujii added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 13, 2023
https://bugs.webkit.org/show_bug.cgi?id=254902

Reviewed by Don Olmstead.

This is the initial support of site isolation for GraphicsLayerWC.
Cocoa port (aka RemoteLayerTree and GraphicsLayerCARemote) transfers a
layer tree to UI process, GraphicsLayerWC transfers a layer tree to
GPU process. GPU process renders a remote child frame into a texture,
and attach the texture into the content of iframe layer of the parent
frame's layer tree. Added WCRemoteFrameHostLayerManager class to
manage a HashMap from a LayerHostingContextIdentifier to a
BitmapTexture.

This has a problem now. Updating remote child frame's layer tree
doesn't trigger the parent frame's repaint. For the workaround, you
need to manually click a browser window of MiniBrowser to repaint.
Keep http/tests/site-isolation tests skipped for WinCairo.

MiniBrowser has to enable SiteIsolationEnabled option to test it by
using the internal settings menu item or adding the following code.
> WKPreferencesSetBoolValueForKeyForTesting(preferences.get(), true, createWKString("SiteIsolationEnabled").get());

Fixed a bug of TextureMapperGL::endPainting. It should restore a
previous frame buffer that TextureMapperGL::beginPainting saved.
TextureMapperGL supports the offscreen rendering mode for
WebKitTestRunner pixel dump mode. It's not a problem for
WebKitTestRunner because it renders a layer tree always to a texture.
In site-isolation, however, remote frame's layer tree renders to a
texture and main frame layer tree to a frame buffer. After rendering
to a texture, it has to restore the previous frame buffer.

* Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:
(WebCore::TextureMapperGL::endPainting): Restore the previous framebuffer.
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:
(WebKit::GPUConnectionToWebProcess::didClose):
* Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.cpp:
(WebKit::remoteGraphicsStreamWorkQueue):
* Source/WebKit/GPUProcess/graphics/wc/RemoteWCLayerTreeHost.h:
* Source/WebKit/GPUProcess/graphics/wc/WCRemoteFrameHostLayerManager.cpp: Added.
(WebKit::WCRemoteFrameHostLayerManager::singleton):
(WebKit::WCRemoteFrameHostLayerManager::acquireRemoteFrameHostLayer):
(WebKit::WCRemoteFrameHostLayerManager::releaseRemoteFrameHostLayer):
(WebKit::WCRemoteFrameHostLayerManager::updateTexture):
(WebKit::WCRemoteFrameHostLayerManager::removeAllLayersForProcess):
* Source/WebKit/GPUProcess/graphics/wc/WCRemoteFrameHostLayerManager.h: Added.
* Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:
(WebKit::WCScene::update):
* Source/WebKit/PlatformPlayStation.cmake:
* Source/WebKit/PlatformWin.cmake:
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.cpp:
(WebKit::DrawingAreaWC::DrawingAreaWC):
(WebKit::DrawingAreaWC::updateRootLayers):
(WebKit::DrawingAreaWC::setRootCompositingLayer):
(WebKit::DrawingAreaWC::addRootFrame):
(WebKit::DrawingAreaWC::attachViewOverlayGraphicsLayer):
(WebKit::DrawingAreaWC::updatePreferences):
(WebKit::DrawingAreaWC::shouldUseTiledBackingForFrameView const):
(WebKit::DrawingAreaWC::updateGeometryWC):
(WebKit::DrawingAreaWC::isCompositingMode):
(WebKit::DrawingAreaWC::sendUpdateAC):
(WebKit::DrawingAreaWC::rootLayerInfoWithFrameIdentifier):
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.h:
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:
(WebKit::GraphicsLayerWC::setContentsToPlatformLayerHost):
(WebKit::GraphicsLayerWC::flushCompositingStateForThisLayerOnly):
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.h:
* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.h:
(WebKit::WCLayerUpdateInfo::encode const):
(WebKit::WCLayerUpdateInfo::decode):
(WebKit::WCUpdateInfo::encode const):
(WebKit::WCUpdateInfo::decode):

Canonical link: https://commits.webkit.org/265099@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/GraphicsLayerWC-site-isolation branch from f90ac07 to dc5a423 Compare June 13, 2023 03:14
@webkit-commit-queue
Copy link
Collaborator

Committed 265099@main (dc5a423): https://commits.webkit.org/265099@main

Reviewed commits have been landed. Closing PR #12723 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 13, 2023
@webkit-commit-queue webkit-commit-queue merged commit dc5a423 into WebKit:main Jun 13, 2023
@fujii fujii deleted the eng/GraphicsLayerWC-site-isolation branch June 30, 2023 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit2 Bugs relating to the WebKit2 API layer
Projects
None yet
6 participants