Diff
Modified: trunk/Source/WebCore/ChangeLog (271756 => 271757)
--- trunk/Source/WebCore/ChangeLog 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Source/WebCore/ChangeLog 2021-01-22 20:40:21 UTC (rev 271757)
@@ -1,3 +1,44 @@
+2021-01-21 Wenson Hsieh <[email protected]>
+
+ DisplayList::Replayer should stop replay and inform clients after encountering an invalid item
+ https://bugs.webkit.org/show_bug.cgi?id=220867
+
+ Reviewed by Chris Dumez.
+
+ Make the `DisplayList` item iterator emit `Optional<ItemHandle>` instead of just `ItemHandle`; in the case of
+ client decoding or item validation failures (see #220710), this item handle will be `WTF::nullopt`. The display
+ list replayer will then handle this by halting replay with `StopReplayReason::InvalidItem`.
+
+ This refactoring will eventually enable `RemoteRenderingBackend` (in the GPU process) to terminate the web
+ content process when we fail to decode a display list item during playback (or encounter an invalid inline
+ item).
+
+ Test: DisplayListTests.InlineItemValidationFailure
+ DisplayListTests.OutOfLineItemDecodingFailure
+
+ * platform/graphics/displaylists/DisplayList.cpp:
+ (WebCore::DisplayList::DisplayList::asText const):
+ (WebCore::DisplayList::DisplayList::dump const):
+ (WebCore::DisplayList::DisplayList::iterator::atEnd const):
+ (WebCore::DisplayList::DisplayList::iterator::updateCurrentItem):
+
+ Add an `m_isValid` flag. This flag is set to `true` initially, and is only set to `false` when we encounter an
+ item that is either invalid or unable to be decoded. Additionally, remove release assertions and the FIXMEs
+ regarding handling item decoding failures.
+
+ * platform/graphics/displaylists/DisplayList.h:
+ (WebCore::DisplayList::DisplayList::iterator::operator* const):
+
+ If the `m_isValid` flag above is set, return `WTF::nullopt` for the item handle.
+
+ * platform/graphics/displaylists/DisplayListReplayer.cpp:
+ (WebCore::DisplayList::Replayer::replay):
+ * platform/graphics/displaylists/DisplayListReplayer.h:
+
+ Rename `DecodingFailure` to `InvalidItem`, since it now applies to both items that are invalid (e.g. contain
+ identifier values of 0 when they shouldn't) as well as items that fail decoding (which, in the case of the GPU
+ process, corresponds to IPC decoding failures).
+
2021-01-22 Chris Dumez <[email protected]>
Crash under FFTFrame::fftSetupForSize()
Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp (271756 => 271757)
--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp 2021-01-22 20:40:21 UTC (rev 271757)
@@ -115,12 +115,12 @@
{
TextStream stream(TextStream::LineMode::MultipleLine, TextStream::Formatting::SVGStyleRect);
for (auto [item, extent, itemSizeInBuffer] : *this) {
- if (!shouldDumpForFlags(flags, item))
+ if (!shouldDumpForFlags(flags, *item))
continue;
TextStream::GroupScope group(stream);
stream << item;
- if (item.isDrawingItem())
+ if (item->isDrawingItem())
stream << " extent " << extent;
}
return stream.release();
@@ -134,7 +134,7 @@
for (auto [item, extent, itemSizeInBuffer] : *this) {
TextStream::GroupScope group(ts);
ts << item;
- if (item.isDrawingItem())
+ if (item->isDrawingItem())
ts << " extent " << extent;
}
ts.startGroup();
@@ -322,7 +322,7 @@
bool DisplayList::iterator::atEnd() const
{
- if (m_displayList.isEmpty())
+ if (m_displayList.isEmpty() || !m_isValid)
return true;
auto& items = *m_displayList.m_items;
@@ -352,20 +352,15 @@
auto dataLength = reinterpret_cast<uint64_t*>(m_cursor)[1];
auto* startOfData = m_cursor + 2 * sizeof(uint64_t);
auto decodedItemHandle = client->decodeItem(startOfData, dataLength, itemType, m_currentBufferForItem);
- if (!decodedItemHandle) {
- // FIXME: Instead of crashing, this needs to fail gracefully and inform the caller.
- // For the time being, this assertion makes bugs in item buffer encoding logic easier
- // to catch by avoiding mysterious out-of-bounds reads and incorrectly aligned items
- // down the line.
- RELEASE_ASSERT_NOT_REACHED();
- }
+ if (UNLIKELY(!decodedItemHandle))
+ m_isValid = false;
+
m_currentBufferForItem[0] = static_cast<uint8_t>(itemType);
m_currentItemSizeInBuffer = 2 * sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), dataLength);
} else {
- if (!ItemHandle { m_cursor }.safeCopy({ m_currentBufferForItem })) {
- // FIXME: Instead of crashing, this needs to fail gracefully and inform the caller.
- RELEASE_ASSERT_NOT_REACHED();
- }
+ if (UNLIKELY(!ItemHandle { m_cursor }.safeCopy({ m_currentBufferForItem })))
+ m_isValid = false;
+
m_currentItemSizeInBuffer = paddedSizeOfTypeAndItem;
}
}
Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h (271756 => 271757)
--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h 2021-01-22 20:40:21 UTC (rev 271757)
@@ -118,7 +118,7 @@
void operator++() { advance(); }
struct Value {
- ItemHandle item;
+ Optional<ItemHandle> item;
Optional<FloatRect> extent;
size_t itemSizeInBuffer { 0 };
};
@@ -126,9 +126,9 @@
Value operator*() const
{
return {
- ItemHandle { m_currentBufferForItem },
+ m_isValid ? makeOptional(ItemHandle { m_currentBufferForItem }) : WTF::nullopt,
m_currentExtent,
- m_currentItemSizeInBuffer
+ m_currentItemSizeInBuffer,
};
}
@@ -152,6 +152,7 @@
uint8_t* m_currentBufferForItem { nullptr };
Optional<FloatRect> m_currentExtent;
size_t m_currentItemSizeInBuffer { 0 };
+ bool m_isValid { true };
};
iterator begin() const { return { *this }; }
Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp (271756 => 271757)
--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp 2021-01-22 20:40:21 UTC (rev 271757)
@@ -141,16 +141,21 @@
continue;
}
+ if (!item) {
+ result.reasonForStopping = StopReplayReason::InvalidItem;
+ break;
+ }
+
LOG_WITH_STREAM(DisplayLists, stream << "applying " << i++ << " " << item);
- if (item.is<MetaCommandChangeDestinationImageBuffer>()) {
+ if (item->is<MetaCommandChangeDestinationImageBuffer>()) {
result.numberOfBytesRead += itemSizeInBuffer;
result.reasonForStopping = StopReplayReason::ChangeDestinationImageBuffer;
- result.nextDestinationImageBuffer = item.get<MetaCommandChangeDestinationImageBuffer>().identifier();
+ result.nextDestinationImageBuffer = item->get<MetaCommandChangeDestinationImageBuffer>().identifier();
break;
}
- if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(item); reasonForStopping) {
+ if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(*item); reasonForStopping) {
result.reasonForStopping = *reasonForStopping;
result.missingCachedResourceIdentifier = WTFMove(missingCachedResourceIdentifier);
break;
@@ -159,8 +164,8 @@
result.numberOfBytesRead += itemSizeInBuffer;
if (UNLIKELY(trackReplayList)) {
- replayList->append(item);
- if (item.isDrawingItem())
+ replayList->append(*item);
+ if (item->isDrawingItem())
replayList->addDrawingItemExtent(WTFMove(extent));
}
}
Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h (271756 => 271757)
--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h 2021-01-22 20:40:21 UTC (rev 271757)
@@ -42,7 +42,7 @@
ReplayedAllItems,
MissingCachedResource,
ChangeDestinationImageBuffer,
- DecodingFailure // FIXME: Propagate decoding errors to display list replay clients through this enum as well.
+ InvalidItem
};
struct ReplayResult {
Modified: trunk/Tools/ChangeLog (271756 => 271757)
--- trunk/Tools/ChangeLog 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Tools/ChangeLog 2021-01-22 20:40:21 UTC (rev 271757)
@@ -1,3 +1,18 @@
+2021-01-21 Wenson Hsieh <[email protected]>
+
+ DisplayList::Replayer should stop replay and inform clients after encountering an invalid item
+ https://bugs.webkit.org/show_bug.cgi?id=220867
+
+ Reviewed by Chris Dumez.
+
+ * TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:
+
+ Adjust a few API tests, since the item handle in the `DisplayList` iterator is now optional.
+
+ * TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp:
+
+ Add a couple of new API tests to exercise display list item decoding and validation failures.
+
2021-01-22 Kimmo Kinnunen <[email protected]>
Retried tests are run without --prefer-integrated-gpu
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp (271756 => 271757)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp 2021-01-22 20:40:21 UTC (rev 271757)
@@ -79,41 +79,41 @@
bool observedUnexpectedItem = false;
for (auto [handle, extent, size] : list) {
- switch (handle.type()) {
+ switch (handle->type()) {
case ItemType::SetStrokeThickness: {
- EXPECT_FALSE(handle.isDrawingItem());
- EXPECT_TRUE(handle.is<SetStrokeThickness>());
- auto& item = handle.get<SetStrokeThickness>();
+ EXPECT_FALSE(handle->isDrawingItem());
+ EXPECT_TRUE(handle->is<SetStrokeThickness>());
+ auto& item = handle->get<SetStrokeThickness>();
EXPECT_EQ(item.thickness(), 1.5);
break;
}
case ItemType::FillPath: {
- EXPECT_TRUE(handle.isDrawingItem());
- EXPECT_TRUE(handle.is<FillPath>());
- auto& item = handle.get<FillPath>();
+ EXPECT_TRUE(handle->isDrawingItem());
+ EXPECT_TRUE(handle->is<FillPath>());
+ auto& item = handle->get<FillPath>();
EXPECT_EQ(item.path().platformPath(), path.platformPath());
break;
}
case ItemType::FillRectWithGradient: {
- EXPECT_TRUE(handle.isDrawingItem());
- EXPECT_TRUE(handle.is<FillRectWithGradient>());
- auto& item = handle.get<FillRectWithGradient>();
+ EXPECT_TRUE(handle->isDrawingItem());
+ EXPECT_TRUE(handle->is<FillRectWithGradient>());
+ auto& item = handle->get<FillRectWithGradient>();
EXPECT_EQ(item.rect(), FloatRect(1., 1., 10., 10.));
EXPECT_EQ(&item.gradient(), gradient.ptr());
break;
}
case ItemType::SetInlineFillColor: {
- EXPECT_FALSE(handle.isDrawingItem());
- EXPECT_TRUE(handle.is<SetInlineFillColor>());
- auto& item = handle.get<SetInlineFillColor>();
+ EXPECT_FALSE(handle->isDrawingItem());
+ EXPECT_TRUE(handle->is<SetInlineFillColor>());
+ auto& item = handle->get<SetInlineFillColor>();
EXPECT_EQ(item.color(), Color::red);
break;
}
#if ENABLE(INLINE_PATH_DATA)
case ItemType::StrokeInlinePath: {
- EXPECT_TRUE(handle.isDrawingItem());
- EXPECT_TRUE(handle.is<StrokeInlinePath>());
- auto& item = handle.get<StrokeInlinePath>();
+ EXPECT_TRUE(handle->isDrawingItem());
+ EXPECT_TRUE(handle->is<StrokeInlinePath>());
+ auto& item = handle->get<StrokeInlinePath>();
const auto path = item.path();
auto& line = path.inlineData<LineData>();
EXPECT_EQ(line.start, FloatPoint(0, 0));
@@ -148,10 +148,10 @@
list.append<FillRectWithColor>(FloatRect { 0, 0, 100, 100 }, Color::black);
for (auto [handle, extent, size] : list) {
- EXPECT_EQ(handle.type(), ItemType::FillRectWithColor);
- EXPECT_TRUE(handle.is<FillRectWithColor>());
+ EXPECT_EQ(handle->type(), ItemType::FillRectWithColor);
+ EXPECT_TRUE(handle->is<FillRectWithColor>());
- auto& item = handle.get<FillRectWithColor>();
+ auto& item = handle->get<FillRectWithColor>();
EXPECT_EQ(item.color().asInline(), Color::black);
EXPECT_EQ(item.rect(), FloatRect(0, 0, 100, 100));
}
@@ -226,7 +226,7 @@
Vector<ItemType> itemTypes;
for (auto [handle, extent, size] : shallowCopy)
- itemTypes.append(handle.type());
+ itemTypes.append(handle->type());
EXPECT_FALSE(shallowCopy.isEmpty());
EXPECT_EQ(itemTypes.size(), 4U);
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp (271756 => 271757)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp 2021-01-22 20:10:01 UTC (rev 271756)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp 2021-01-22 20:40:21 UTC (rev 271757)
@@ -37,11 +37,13 @@
using DisplayList::DisplayList;
using namespace DisplayList;
+constexpr size_t globalItemBufferCapacity = 1 << 12;
+static uint8_t globalItemBuffer[globalItemBufferCapacity];
+constexpr CGFloat contextWidth = 100;
+constexpr CGFloat contextHeight = 100;
+
TEST(DisplayListTests, ReplayWithMissingResource)
{
- constexpr CGFloat contextWidth = 100;
- constexpr CGFloat contextHeight = 100;
-
FloatRect contextBounds { 0, 0, contextWidth, contextHeight };
auto colorSpace = adoptCF(CGColorSpaceCreateWithName(kCGColorSpaceSRGB));
@@ -79,6 +81,79 @@
}
}
+TEST(DisplayListTests, OutOfLineItemDecodingFailure)
+{
+ static ItemBufferIdentifier globalBufferIdentifier = ItemBufferIdentifier::generate();
+
+ class ReadingClient : public ItemBufferReadingClient {
+ private:
+ Optional<ItemHandle> WARN_UNUSED_RETURN decodeItem(const uint8_t*, size_t, ItemType type, uint8_t*) final
+ {
+ EXPECT_EQ(type, ItemType::FillPath);
+ return WTF::nullopt;
+ }
+ };
+
+ class WritingClient : public ItemBufferWritingClient {
+ private:
+ ItemBufferHandle createItemBuffer(size_t capacity) final
+ {
+ EXPECT_LT(capacity, globalItemBufferCapacity);
+ return { globalBufferIdentifier, globalItemBuffer, globalItemBufferCapacity };
+ }
+
+ RefPtr<SharedBuffer> encodeItem(ItemHandle) const final { return SharedBuffer::create(); }
+ void didAppendData(const ItemBufferHandle&, size_t, DidChangeItemBuffer) final { }
+ };
+
+ FloatRect contextBounds { 0, 0, contextWidth, contextHeight };
+
+ auto colorSpace = adoptCF(CGColorSpaceCreateWithName(kCGColorSpaceSRGB));
+ auto cgContext = adoptCF(CGBitmapContextCreate(nullptr, contextWidth, contextHeight, 8, 4 * contextWidth, colorSpace.get(), kCGImageAlphaPremultipliedLast));
+ GraphicsContext context { cgContext.get() };
+
+ DisplayList list;
+ WritingClient writer;
+ list.setItemBufferClient(&writer);
+
+ Path path;
+ path.moveTo({ 10., 10. });
+ path.addLineTo({ 50., 50. });
+ path.addLineTo({ 10., 10. });
+ list.append<SetInlineStrokeColor>(Color::blue);
+ list.append<FillPath>(WTFMove(path));
+
+ DisplayList shallowCopy {{ ItemBufferHandle { globalBufferIdentifier, globalItemBuffer, list.sizeInBytes() } }};
+ ReadingClient reader;
+ shallowCopy.setItemBufferClient(&reader);
+
+ Replayer replayer { context, list };
+ auto result = replayer.replay();
+ EXPECT_GT(result.numberOfBytesRead, 0U);
+ EXPECT_EQ(result.nextDestinationImageBuffer, WTF::nullopt);
+ EXPECT_EQ(result.missingCachedResourceIdentifier, WTF::nullopt);
+ EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItem);
+}
+
+TEST(DisplayListTests, InlineItemValidationFailure)
+{
+ FloatRect contextBounds { 0, 0, contextWidth, contextHeight };
+
+ auto colorSpace = adoptCF(CGColorSpaceCreateWithName(kCGColorSpaceSRGB));
+ auto cgContext = adoptCF(CGBitmapContextCreate(nullptr, contextWidth, contextHeight, 8, 4 * contextWidth, colorSpace.get(), kCGImageAlphaPremultipliedLast));
+ GraphicsContext context { cgContext.get() };
+
+ DisplayList list;
+ list.append<FlushContext>(FlushIdentifier { });
+
+ Replayer replayer { context, list };
+ auto result = replayer.replay();
+ EXPECT_EQ(result.numberOfBytesRead, 0U);
+ EXPECT_EQ(result.nextDestinationImageBuffer, WTF::nullopt);
+ EXPECT_EQ(result.missingCachedResourceIdentifier, WTF::nullopt);
+ EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItem);
+}
+
} // namespace TestWebKitAPI
#endif // USE(CG)
