Skip to content

Commit

Permalink
[UnifiedPDF] Occasional crashes under IOSurface construction in the w…
Browse files Browse the repository at this point in the history
…eb content process

https://bugs.webkit.org/show_bug.cgi?id=268439

Reviewed by Said Abou-Hallawa.

LayerPool will often return layers that contain existing RemoteLayerBackingStore instances.
Before reusing the RemoteLayerBackingStore, we need to make sure it's of the correct subclass
for the layer, once the layer is reconfigured.

* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::createForLayer):
(WebKit::RemoteLayerBackingStore::processModelForLayer):
Hoist model picking and RLBS creation into RLBS.

* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStoreCollection.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStoreCollection.mm:
(WebKit::RemoteLayerBackingStoreCollection::createRemoteLayerBackingStore): Deleted.
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithInProcessRenderingBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithRemoteRenderingBackingStore.h:
Make it possible to query a RLBS' "process model" (essentially which subclass it is,
this is technically duplicative with the is... methods, but handier for comparison).

* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.mm:
(WebKit::PlatformCALayerRemote::ensureBackingStore):
Avoid reusing the incorrect RLBS subclass.

Canonical link: https://commits.webkit.org/273848@main
  • Loading branch information
hortont424 committed Jan 31, 2024
1 parent 7bd3873 commit 82c681c
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ class RemoteLayerBackingStore : public CanMakeWeakPtr<RemoteLayerBackingStore> {
RemoteLayerBackingStore(PlatformCALayerRemote*);
virtual ~RemoteLayerBackingStore();

static std::unique_ptr<RemoteLayerBackingStore> createForLayer(PlatformCALayerRemote*);

enum class Type : bool {
IOSurface,
Bitmap
Expand All @@ -91,6 +93,10 @@ class RemoteLayerBackingStore : public CanMakeWeakPtr<RemoteLayerBackingStore> {
virtual bool isRemoteLayerWithRemoteRenderingBackingStore() const { return false; }
virtual bool isRemoteLayerWithInProcessRenderingBackingStore() const { return false; }

enum class ProcessModel : uint8_t { InProcess, Remote };
virtual ProcessModel processModel() const = 0;
static ProcessModel processModelForLayer(PlatformCALayerRemote*);

struct Parameters {
Type type { Type::Bitmap };
WebCore::FloatSize size;
Expand Down
19 changes: 19 additions & 0 deletions Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
#import "RemoteLayerTreeHost.h"
#import "RemoteLayerTreeLayers.h"
#import "RemoteLayerTreeNode.h"
#import "RemoteLayerWithInProcessRenderingBackingStore.h"
#import "RemoteLayerWithRemoteRenderingBackingStore.h"
#import "ShareableBitmap.h"
#import "SwapBuffersDisplayRequirement.h"
#import "WebCoreArgumentCoders.h"
Expand Down Expand Up @@ -93,6 +95,16 @@ void flushAndCollectHandles(HashMap<RemoteImageBufferSetIdentifier, std::unique_

}

std::unique_ptr<RemoteLayerBackingStore> RemoteLayerBackingStore::createForLayer(PlatformCALayerRemote* layer)
{
switch (processModelForLayer(layer)) {
case ProcessModel::Remote:
return makeUnique<RemoteLayerWithRemoteRenderingBackingStore>(layer);
case ProcessModel::InProcess:
return makeUnique<RemoteLayerWithInProcessRenderingBackingStore>(layer);
}
}

RemoteLayerBackingStore::RemoteLayerBackingStore(PlatformCALayerRemote* layer)
: m_layer(layer)
, m_lastDisplayTime(-MonotonicTime::infinity())
Expand Down Expand Up @@ -130,6 +142,13 @@ void flushAndCollectHandles(HashMap<RemoteImageBufferSetIdentifier, std::unique_
clearBackingStore();
}

RemoteLayerBackingStore::ProcessModel RemoteLayerBackingStore::processModelForLayer(PlatformCALayerRemote* layer)
{
if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM) && !layer->needsPlatformContext())
return ProcessModel::Remote;
return ProcessModel::InProcess;
}

#if !LOG_DISABLED
static bool hasValue(const ImageBufferBackendHandle& backendHandle)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ class RemoteLayerBackingStoreCollection : public CanMakeWeakPtr<RemoteLayerBacki
bool backingStoreWillBeDisplayed(RemoteLayerBackingStore&);
void backingStoreBecameUnreachable(RemoteLayerBackingStore&);

std::unique_ptr<RemoteLayerBackingStore> createRemoteLayerBackingStore(PlatformCALayerRemote*);

virtual void prepareBackingStoresForDisplay(RemoteLayerTreeTransaction&);
virtual bool paintReachableBackingStoreContents();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,6 @@
}
}

std::unique_ptr<RemoteLayerBackingStore> RemoteLayerBackingStoreCollection::createRemoteLayerBackingStore(PlatformCALayerRemote* layer)
{
// We currently only create a single type of backing store based on the global setting, but
// it should be fine to mix both types in the same collection.
if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM) && !layer->needsPlatformContext())
return makeUnique<RemoteLayerWithRemoteRenderingBackingStore>(layer);
return makeUnique<RemoteLayerWithInProcessRenderingBackingStore>(layer);
}

bool RemoteLayerBackingStoreCollection::paintReachableBackingStoreContents()
{
bool anyNonEmptyDirtyRegion = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class RemoteLayerWithInProcessRenderingBackingStore : public RemoteLayerBackingS
using RemoteLayerBackingStore::RemoteLayerBackingStore;

bool isRemoteLayerWithInProcessRenderingBackingStore() const final { return true; }
ProcessModel processModel() const final { return ProcessModel::InProcess; }

void prepareToDisplay() final;
void createContextAndPaintContents() final;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class RemoteLayerWithRemoteRenderingBackingStore : public RemoteLayerBackingStor
RemoteLayerWithRemoteRenderingBackingStore(PlatformCALayerRemote*);

bool isRemoteLayerWithRemoteRenderingBackingStore() const final { return true; }
ProcessModel processModel() const final { return ProcessModel::Remote; }

void prepareToDisplay() final;
void clearBackingStore() final;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,11 +236,24 @@
void PlatformCALayerRemote::ensureBackingStore()
{
ASSERT(owner());

ASSERT(m_properties.backingStoreAttached);

if (!m_properties.backingStoreOrProperties.store && m_context)
m_properties.backingStoreOrProperties.store = m_context->backingStoreCollection().createRemoteLayerBackingStore(this);
bool needsNewBackingStore = [&] {
if (!m_context)
return false;

if (!m_properties.backingStoreOrProperties.store)
return true;

// A layer pulled out of a pool may have existing backing store which we mustn't reuse if it lives in the wrong process.
if (m_properties.backingStoreOrProperties.store->processModel() != RemoteLayerBackingStore::processModelForLayer(this))
return true;

return false;
}();

if (needsNewBackingStore)
m_properties.backingStoreOrProperties.store = RemoteLayerBackingStore::createForLayer(this);

updateBackingStore();
}
Expand Down

0 comments on commit 82c681c

Please sign in to comment.