Crash at com.apple.WebKit: WebKit::RemoteLayerBackingStore::display

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

Crash at com.apple.WebKit: WebKit::RemoteLayerBackingStore::display

[Kimmo Kinnunen]

<rdar://87617695>

[Kimmo Kinnunen]

Created attachment 450614 [details]
Patch

[Kimmo Kinnunen]

Created attachment 450621 [details]
Patch

[Kimmo Kinnunen]

Created attachment 450622 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 450622 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Using UI-side compositing, ollowing chain would be the cause a crash:

"ollowing"

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp:686
> +    ensureBackingStore();

I guess a RemoteLayerBackingStore is the only type of contents we allow on a PlatformCALayerRemote, unlike PlatformCALayer?

I might expect this change to cause us to hit the ASSERT(m_properties.backingStoreAttached) in PlatformCALayerRemote::ensureBackingStore(). Why don't we?

Does ensureBackingStore() make us allocate a bunch of buffers, only to immediately throw them away?

[Kimmo Kinnunen]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 450622 [details]
> Patch
> I guess a RemoteLayerBackingStore is the only type of contents we allow on a
> PlatformCALayerRemote, unlike PlatformCALayer?

Correct. Though PlatformCALayer does not allow many (many as in arbitrary, as in polymorphic) types. It only allows "CALayer backing store types", eg. CGImage or IOSurface.


> I might expect this change to cause us to hit the
> ASSERT(m_properties.backingStoreAttached) in
> PlatformCALayerRemote::ensureBackingStore(). Why don't we?

For PlatformCALayerDisplayDelegate, the purpose of it is to delegate the display of PlatformCALayer. The display means "update backing store", and as such it would be illogical for the display delegate to exist with a layer that does not have backing store attached. This is why GraphicsLayerCA::setContentsDisplayDelegate guarantees it by setting it.

However, there are a lot of mostly implicitly nullptr calls in other places, like the clone setters/unsetters and then the "image as layer" feature. I don't understand why the clone setters don't assert in tests..

> Does ensureBackingStore() make us allocate a bunch of buffers, only to
> immediately throw them away?

No, ensureBackingStore() does not allocate anything. The buffers are allocated during display.

[Kimmo Kinnunen]

Created attachment 450669 [details]
Patch

[EWS]

Committed r289062 (246769@main): <https://commits.webkit.org/246769@main>

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