Bug 236003 - Crash at com.apple.WebKit: WebKit::RemoteLayerBackingStore::display
Summary: Crash at com.apple.WebKit: WebKit::RemoteLayerBackingStore::display
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-01 22:40 PST by Kimmo Kinnunen
Modified: 2022-02-03 11:19 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.79 KB, patch)
2022-02-01 23:25 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2022-02-01 23:41 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (6.13 KB, patch)
2022-02-01 23:44 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (6.15 KB, patch)
2022-02-02 10:28 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2022-02-01 22:40:27 PST
Crash at com.apple.WebKit: WebKit::RemoteLayerBackingStore::display
Comment 1 Kimmo Kinnunen 2022-02-01 22:41:14 PST
<rdar://87617695>
Comment 2 Kimmo Kinnunen 2022-02-01 23:25:41 PST
Created attachment 450614 [details]
Patch
Comment 3 Kimmo Kinnunen 2022-02-01 23:41:01 PST
Created attachment 450621 [details]
Patch
Comment 4 Kimmo Kinnunen 2022-02-01 23:44:24 PST
Created attachment 450622 [details]
Patch
Comment 5 Simon Fraser (smfr) 2022-02-02 09:18:40 PST
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?
Comment 6 Kimmo Kinnunen 2022-02-02 10:24:35 PST
(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.
Comment 7 Kimmo Kinnunen 2022-02-02 10:28:10 PST
Created attachment 450669 [details]
Patch
Comment 8 EWS 2022-02-03 11:19:26 PST
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].