[WebGPU] Non-owning getters have the wrong lifetime

[litherum]

6c4c981

[WebGPU] Non-owning getters have the wrong lifetime
https://bugs.webkit.org/show_bug.cgi?id=250958
rdar://104518638

Reviewed by Dean Jackson.

There are 2 places in WebGPU where objects have getter methods that return internally-retained objects:
1. Device::getQueue() is supposed to return the same queue object every time you call it, and
2. PresentationContext::getCurrentTexture() is supposed to return the same texture object every time you call it within
       the same frame.

Let's call this pattern "Owner" and "Owned." The Owner is supposed to retain its Owned. Easy peasy, right?

Well, it gets trickier because:
1. We have a corresponding set of Impl objects in PAL, each of which is supposed to maintain a strong reference to its
       corresponding object in WebGPU.framework.
2. Objects exposed by WebGPU.framework are not reference counted.

So, naively, we would have:

  +-----------+
  | OwnerImpl |
  +-----------+
        |     \
        |      \
        |       \
        |        V
        |        +-----------+
        |        | OwnedImpl |
        |        +-----------+
        |             |
~~~~~~~~|~~~~~~~~~~~~~|~~~~~~~~~~ WebGPU.framework boundary
        V             |
    +-------+         |
    | Owner |         |
    +-------+         |
             \        |
              \       |
               \      |
                V     V  BANG!!! EXPLOSION!!!
                 +-------+
                 | Owned |
                 +-------+

The above design can't actually work, because Owned isn't reference counted. So, instead, we can introduce a reference-
counted facade on top of Owner, to look like this:

  +-----------+
  | OwnerImpl |
  +-----------+
        |     \
        |      \
        |       \
        |        V
        |        +-----------+
        |        | OwnedImpl |
        |        +-----------+
        |          |
        |          |
        V          V
      +--------------+
      | OwnerWrapper |
      +--------------+
        |
        |
~~~~~~~~|~~~~~~~~~~~~~~~~~~~~~~~~ WebGPU.framework boundary
        V
    +-------+
    | Owner |
    +-------+
             \
              \
               \
                V
                 +-------+
                 | Owned |
                 +-------+

This design has all the properties we want:
1. All WebGPU.framework objects have a single owner
2. Any strong reference to the OwnerImpl keeps the Owner alive
3. Any strong reference to the OwnedImpl keeps the Owned alive
4. There are no reference cycles

The OwnedImpl doesn't actually call any functions on the OwnerWrapper; the only reason it refs it is to make the above
requirements hold.

There are 2 other possible designs which satisfy the requirements: 1. Have OwnedImpl delegate its ref() and deref()
calls to the OwnerImpl, and 2. Make WebGPU.framework objects reference counted. I chose this patch's design over (1)
because (1) is significantly more complicated and I'm more likely to make a mistake with that design. I chose this
patch's design over (2) because I didn't want to change the semantic behavior of the WebGPU.h objects.

* Source/WebCore/PAL/PAL.xcodeproj/project.pbxproj:
* Source/WebCore/PAL/pal/CMakeLists.txt:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.cpp:
(PAL::WebGPU::DeviceImpl::DeviceImpl):
(PAL::WebGPU::DeviceImpl::destroy):
(PAL::WebGPU::DeviceImpl::createBuffer):
(PAL::WebGPU::DeviceImpl::createTexture):
(PAL::WebGPU::DeviceImpl::createSurfaceTexture):
(PAL::WebGPU::DeviceImpl::createSampler):
(PAL::WebGPU::DeviceImpl::createBindGroupLayout):
(PAL::WebGPU::DeviceImpl::createPipelineLayout):
(PAL::WebGPU::DeviceImpl::createBindGroup):
(PAL::WebGPU::DeviceImpl::createShaderModule):
(PAL::WebGPU::DeviceImpl::createComputePipeline):
(PAL::WebGPU::DeviceImpl::createRenderPipeline):
(PAL::WebGPU::DeviceImpl::createComputePipelineAsync):
(PAL::WebGPU::DeviceImpl::createRenderPipelineAsync):
(PAL::WebGPU::DeviceImpl::createCommandEncoder):
(PAL::WebGPU::DeviceImpl::createRenderBundleEncoder):
(PAL::WebGPU::DeviceImpl::createQuerySet):
(PAL::WebGPU::DeviceImpl::pushErrorScope):
(PAL::WebGPU::DeviceImpl::popErrorScope):
(PAL::WebGPU::DeviceImpl::setLabelInternal):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceWrapper.cpp: Copied from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.cpp.
(PAL::WebGPU::DeviceWrapper::DeviceWrapper):
(PAL::WebGPU::DeviceWrapper::~DeviceWrapper):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceWrapper.h: Copied from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.h.
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.cpp:
(PAL::WebGPU::PresentationContextImpl::~PresentationContextImpl):
(PAL::WebGPU::PresentationContextImpl::configure):
(PAL::WebGPU::PresentationContextImpl::unconfigure):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUQueueImpl.cpp:
(PAL::WebGPU::QueueImpl::QueueImpl):
(PAL::WebGPU::QueueImpl::~QueueImpl):
(PAL::WebGPU::QueueImpl::submit):
(PAL::WebGPU::QueueImpl::onSubmittedWorkDone):
(PAL::WebGPU::QueueImpl::writeBuffer):
(PAL::WebGPU::QueueImpl::writeTexture):
(PAL::WebGPU::QueueImpl::setLabelInternal):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUQueueImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainWrapper.cpp: Renamed from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.cpp.
(PAL::WebGPU::SwapChainWrapper::SwapChainWrapper):
(PAL::WebGPU::SwapChainWrapper::~SwapChainWrapper):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainWrapper.h: Renamed from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.h.
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUTextureImpl.cpp:
(PAL::WebGPU::TextureImpl::TextureImpl):
(PAL::WebGPU::TextureImpl::~TextureImpl):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUTextureImpl.h:

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

1ea320e

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash e582ec9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	✅ 🛠 tv	⏳ 🧪 mac-wk2
	✅ 🛠 tv-sim	🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash d609891)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

[litherum]

There's an alternative way of doing this: instead of retaining the +0 bind group, we can retain the wrapper of the pipeline object which owns the +0 bind group. Maybe that's better than this approach? It would allow us to not expose more WebGPU.framework API additions, which is always good.

[litherum]

Mike and I both agree that this approach is better, but requires more thought about how to make this work with the C++ type system. Marking as draft for now.

[litherum]

Thinking about this more, this is exactly the reason I created PAL::WebGPU::DeviceHolderImpl:

• PAL::WebGPU::DeviceImpl has a method called queue(), which is supposed to return a non-owning reference to the queue that the device itself owns.
• Retaining the returned object needs to keep it alive - queues are reference counted objects
• The naive way of accomplishing this is to make the queue have a strong reference to the device; that way, as long as the queue has a positive refcount, the device will stay alive, so its owned queue will stay alive; all is good.
• But we can't do that, because that means there's a retain cycle between the device and its queue!

I think DeviceHolder is an antipattern; instead, I've seen another pattern in WebCore to provide custom implementations of ref() and deref() methods. Maybe we could do something like:

void ref() const
{
    otherObject->ref();
} 
void deref() const
{
    otherObject->deref();
}

[litherum]

Oh, that's not going to work, because ref() is not virtual, so clients will end up calling Queue::ref() instead of QueueImpl::ref() which is the one that does the redirection.

Do we want to make ref() virtual? We could; there are a bunch of places in WebCore that do that. But maybe there's a better solution.

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 88f7797)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

[litherum]

I think DeviceHolder is an antipattern

Maybe not.... maybe it's smart? 🤔

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 6a2f82a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk1	🧪 api-gtk
⏳ 🛠 🧪 jsc	❌ 🛠 tv	❌ 🧪 mac-wk2
	❌ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	❌ 🛠 watch	❌ 🧪 mac-wk2-stress
	❌ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 1ab5f46)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	🛠 ios	🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	🛠 ios-sim	🛠 mac-AS-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	🧪 ios-wk2	🧪 api-mac	⏳ 🧪 gtk-wk2
	🧪 api-ios	🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	🛠 tv	🧪 mac-wk2
	🛠 tv-sim	🧪 mac-AS-debug-wk2
	⏳ 🛠 watch	🧪 mac-wk2-stress
	🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 5088ac2)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash c274f96)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	🧪 ios-wk2	✅ 🧪 api-mac	🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on current version of this PR (hash 1ea320e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

[webkit-commit-queue]

Committed 259867@main (6c4c981): https://commits.webkit.org/259867@main

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