Title: [269640] trunk
Revision
269640
Author
wenson_hs...@apple.com
Date
2020-11-10 12:30:46 -0800 (Tue, 10 Nov 2020)

Log Message

REGRESSION (r269525): Many layout tests crash when run under ASan
https://bugs.webkit.org/show_bug.cgi?id=218733
<rdar://problem/71206273>

Reviewed by Tim Horton.

Source/WebCore:

When running tests under ASan, many layout tests crash under `Vector::asanBufferSizeWillChangeTo`, which (when
ASan is enabled) will crash when the start of the buffer is misaligned to 8 bytes. When appending `DrawGlyph`
display list items that have Vectors with inline capacities, the start of a buffer may end up at an offset that
is not aligned to 8 bytes, since all display list items are currently laid out back-to-back, with a single byte
to represent the item type and each item's data following immediately thereafter.

To address this, adjust display list item buffer logic to make sure that for each item that is appended to an
item buffer, both the item type and the item itself are aligned to 8 bytes.

* platform/graphics/displaylists/DisplayList.cpp:
(WebCore::DisplayList::DisplayList::iterator::updateCurrentItem):
* platform/graphics/displaylists/DisplayListItemBuffer.cpp:
(WebCore::DisplayList::ItemHandle::copyTo const):
(WebCore::DisplayList::ItemBuffer::swapWritableBufferIfNeeded):
(WebCore::DisplayList::ItemBuffer::appendEncodedData):
(WebCore::DisplayList::ItemBuffer::appendDataAndLength): Deleted.
* platform/graphics/displaylists/DisplayListItemBuffer.h:
(WebCore::DisplayList::ItemHandle::type const):
(WebCore::DisplayList::ItemHandle::get const):
(WebCore::DisplayList::ItemBuffer::append):
(WebCore::DisplayList::ItemBuffer::uncheckedAppend):
(WebCore::DisplayList::ItemHandle::size const): Deleted.
* platform/graphics/displaylists/DisplayListItemType.cpp:
(WebCore::DisplayList::sizeOfItemInBytes):
(WebCore::DisplayList::paddedSizeOfTypeAndItemInBytes):

Change `sizeOfItemInBytes` to `paddedSizeOfTypeAndItemInBytes`, and make it include both the padded size of an
item type (8 bytes), the size of the item itself, and the minimum amount of padding after the item that's
required to make sure the total size is aligned to 8 bytes.

* platform/graphics/displaylists/DisplayListItemType.h:

Source/WebKit:

Construct the item 8 bytes after the handle offset, instead of 1 byte (also, add a FIXME indicating that we some
way of achieving this without requiring WebKit2 to be aware of an item handle's memory layout).

* GPUProcess/graphics/RemoteRenderingBackend.h:
(WebKit::RemoteRenderingBackend::decodeAndCreate):

Tools:

* TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:
(TestWebKitAPI::TEST):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (269639 => 269640)


--- trunk/Source/WebCore/ChangeLog	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebCore/ChangeLog	2020-11-10 20:30:46 UTC (rev 269640)
@@ -1,3 +1,43 @@
+2020-11-10  Wenson Hsieh  <wenson_hs...@apple.com>
+
+        REGRESSION (r269525): Many layout tests crash when run under ASan
+        https://bugs.webkit.org/show_bug.cgi?id=218733
+        <rdar://problem/71206273>
+
+        Reviewed by Tim Horton.
+
+        When running tests under ASan, many layout tests crash under `Vector::asanBufferSizeWillChangeTo`, which (when
+        ASan is enabled) will crash when the start of the buffer is misaligned to 8 bytes. When appending `DrawGlyph`
+        display list items that have Vectors with inline capacities, the start of a buffer may end up at an offset that
+        is not aligned to 8 bytes, since all display list items are currently laid out back-to-back, with a single byte
+        to represent the item type and each item's data following immediately thereafter.
+
+        To address this, adjust display list item buffer logic to make sure that for each item that is appended to an
+        item buffer, both the item type and the item itself are aligned to 8 bytes.
+
+        * platform/graphics/displaylists/DisplayList.cpp:
+        (WebCore::DisplayList::DisplayList::iterator::updateCurrentItem):
+        * platform/graphics/displaylists/DisplayListItemBuffer.cpp:
+        (WebCore::DisplayList::ItemHandle::copyTo const):
+        (WebCore::DisplayList::ItemBuffer::swapWritableBufferIfNeeded):
+        (WebCore::DisplayList::ItemBuffer::appendEncodedData):
+        (WebCore::DisplayList::ItemBuffer::appendDataAndLength): Deleted.
+        * platform/graphics/displaylists/DisplayListItemBuffer.h:
+        (WebCore::DisplayList::ItemHandle::type const):
+        (WebCore::DisplayList::ItemHandle::get const):
+        (WebCore::DisplayList::ItemBuffer::append):
+        (WebCore::DisplayList::ItemBuffer::uncheckedAppend):
+        (WebCore::DisplayList::ItemHandle::size const): Deleted.
+        * platform/graphics/displaylists/DisplayListItemType.cpp:
+        (WebCore::DisplayList::sizeOfItemInBytes):
+        (WebCore::DisplayList::paddedSizeOfTypeAndItemInBytes):
+
+        Change `sizeOfItemInBytes` to `paddedSizeOfTypeAndItemInBytes`, and make it include both the padded size of an
+        item type (8 bytes), the size of the item itself, and the minimum amount of padding after the item that's
+        required to make sure the total size is aligned to 8 bytes.
+
+        * platform/graphics/displaylists/DisplayListItemType.h:
+
 2020-11-10  Antti Koivisto  <an...@apple.com>
 
         [LFC][Integration] Move caret rect computation out of iterator

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp (269639 => 269640)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp	2020-11-10 20:30:46 UTC (rev 269640)
@@ -340,13 +340,12 @@
         m_currentExtent = WTF::nullopt;
 
     auto* client = items.m_readingClient;
-    auto sizeOfTypeAndItem = sizeOfItemInBytes(itemType) + sizeof(ItemType);
-    m_currentBufferForItem = sizeOfTypeAndItem <= sizeOfFixedBufferForCurrentItem ? m_fixedBufferForCurrentItem : reinterpret_cast<uint8_t*>(fastMalloc(sizeOfTypeAndItem));
+    auto paddedSizeOfTypeAndItem = paddedSizeOfTypeAndItemInBytes(itemType);
+    m_currentBufferForItem = paddedSizeOfTypeAndItem <= sizeOfFixedBufferForCurrentItem ? m_fixedBufferForCurrentItem : reinterpret_cast<uint8_t*>(fastMalloc(paddedSizeOfTypeAndItem));
     if (!isInlineItem(itemType) && client) {
-        auto sizeOfData = reinterpret_cast<size_t*>(m_cursor + sizeof(ItemType))[0];
-        auto* startOfPadding = m_cursor + sizeof(ItemType) + sizeof(size_t);
-        size_t padding = (alignof(uint64_t) - reinterpret_cast<uintptr_t>(startOfPadding) % alignof(uint64_t)) % alignof(uint64_t);
-        auto decodedItemHandle = client->decodeItem(startOfPadding + padding, sizeOfData, itemType, m_currentBufferForItem);
+        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
@@ -355,10 +354,10 @@
             RELEASE_ASSERT_NOT_REACHED();
         }
         m_currentBufferForItem[0] = static_cast<uint8_t>(itemType);
-        m_currentItemSizeInBuffer = sizeof(ItemType) + sizeof(size_t) + padding + sizeOfData;
+        m_currentItemSizeInBuffer = 2 * sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), dataLength);
     } else {
         ItemHandle { m_cursor }.copyTo({ m_currentBufferForItem });
-        m_currentItemSizeInBuffer = sizeOfTypeAndItem;
+        m_currentItemSizeInBuffer = paddedSizeOfTypeAndItem;
     }
 }
 

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.cpp (269639 => 269640)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.cpp	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.cpp	2020-11-10 20:30:46 UTC (rev 269640)
@@ -555,255 +555,256 @@
 {
     auto itemType = type();
     destination.data[0] = static_cast<uint8_t>(itemType);
+    auto itemOffset = destination.data + sizeof(uint64_t);
     switch (itemType) {
     case ItemType::ClipOutToPath: {
-        new (destination.data + sizeof(ItemType)) ClipOutToPath(get<ClipOutToPath>());
+        new (itemOffset) ClipOutToPath(get<ClipOutToPath>());
         return;
     }
     case ItemType::ClipPath: {
-        new (destination.data + sizeof(ItemType)) ClipPath(get<ClipPath>());
+        new (itemOffset) ClipPath(get<ClipPath>());
         return;
     }
     case ItemType::ClipToDrawingCommands: {
-        new (destination.data + sizeof(ItemType)) ClipToDrawingCommands(get<ClipToDrawingCommands>());
+        new (itemOffset) ClipToDrawingCommands(get<ClipToDrawingCommands>());
         return;
     }
     case ItemType::DrawFocusRingPath: {
-        new (destination.data + sizeof(ItemType)) DrawFocusRingPath(get<DrawFocusRingPath>());
+        new (itemOffset) DrawFocusRingPath(get<DrawFocusRingPath>());
         return;
     }
     case ItemType::DrawFocusRingRects: {
-        new (destination.data + sizeof(ItemType)) DrawFocusRingRects(get<DrawFocusRingRects>());
+        new (itemOffset) DrawFocusRingRects(get<DrawFocusRingRects>());
         return;
     }
     case ItemType::DrawGlyphs: {
-        new (destination.data + sizeof(ItemType)) DrawGlyphs(get<DrawGlyphs>());
+        new (itemOffset) DrawGlyphs(get<DrawGlyphs>());
         return;
     }
     case ItemType::DrawImage: {
-        new (destination.data + sizeof(ItemType)) DrawImage(get<DrawImage>());
+        new (itemOffset) DrawImage(get<DrawImage>());
         return;
     }
     case ItemType::DrawImageBuffer: {
-        new (destination.data + sizeof(ItemType)) DrawImageBuffer(get<DrawImageBuffer>());
+        new (itemOffset) DrawImageBuffer(get<DrawImageBuffer>());
         return;
     }
     case ItemType::DrawLinesForText: {
-        new (destination.data + sizeof(ItemType)) DrawLinesForText(get<DrawLinesForText>());
+        new (itemOffset) DrawLinesForText(get<DrawLinesForText>());
         return;
     }
     case ItemType::DrawNativeImage: {
-        new (destination.data + sizeof(ItemType)) DrawNativeImage(get<DrawNativeImage>());
+        new (itemOffset) DrawNativeImage(get<DrawNativeImage>());
         return;
     }
     case ItemType::DrawPath: {
-        new (destination.data + sizeof(ItemType)) DrawPath(get<DrawPath>());
+        new (itemOffset) DrawPath(get<DrawPath>());
         return;
     }
     case ItemType::DrawPattern: {
-        new (destination.data + sizeof(ItemType)) DrawPattern(get<DrawPattern>());
+        new (itemOffset) DrawPattern(get<DrawPattern>());
         return;
     }
     case ItemType::DrawTiledImage: {
-        new (destination.data + sizeof(ItemType)) DrawTiledImage(get<DrawTiledImage>());
+        new (itemOffset) DrawTiledImage(get<DrawTiledImage>());
         return;
     }
     case ItemType::DrawTiledScaledImage: {
-        new (destination.data + sizeof(ItemType)) DrawTiledScaledImage(get<DrawTiledScaledImage>());
+        new (itemOffset) DrawTiledScaledImage(get<DrawTiledScaledImage>());
         return;
     }
     case ItemType::FillCompositedRect: {
-        new (destination.data + sizeof(ItemType)) FillCompositedRect(get<FillCompositedRect>());
+        new (itemOffset) FillCompositedRect(get<FillCompositedRect>());
         return;
     }
     case ItemType::FillPath: {
-        new (destination.data + sizeof(ItemType)) FillPath(get<FillPath>());
+        new (itemOffset) FillPath(get<FillPath>());
         return;
     }
     case ItemType::FillRectWithColor: {
-        new (destination.data + sizeof(ItemType)) FillRectWithColor(get<FillRectWithColor>());
+        new (itemOffset) FillRectWithColor(get<FillRectWithColor>());
         return;
     }
     case ItemType::FillRectWithGradient: {
-        new (destination.data + sizeof(ItemType)) FillRectWithGradient(get<FillRectWithGradient>());
+        new (itemOffset) FillRectWithGradient(get<FillRectWithGradient>());
         return;
     }
     case ItemType::FillRectWithRoundedHole: {
-        new (destination.data + sizeof(ItemType)) FillRectWithRoundedHole(get<FillRectWithRoundedHole>());
+        new (itemOffset) FillRectWithRoundedHole(get<FillRectWithRoundedHole>());
         return;
     }
     case ItemType::FillRoundedRect: {
-        new (destination.data + sizeof(ItemType)) FillRoundedRect(get<FillRoundedRect>());
+        new (itemOffset) FillRoundedRect(get<FillRoundedRect>());
         return;
     }
     case ItemType::PutImageData: {
-        new (destination.data + sizeof(ItemType)) PutImageData(get<PutImageData>());
+        new (itemOffset) PutImageData(get<PutImageData>());
         return;
     }
     case ItemType::SetLineDash: {
-        new (destination.data + sizeof(ItemType)) SetLineDash(get<SetLineDash>());
+        new (itemOffset) SetLineDash(get<SetLineDash>());
         return;
     }
     case ItemType::SetState: {
-        new (destination.data + sizeof(ItemType)) SetState(get<SetState>());
+        new (itemOffset) SetState(get<SetState>());
         return;
     }
     case ItemType::StrokePath: {
-        new (destination.data + sizeof(ItemType)) StrokePath(get<StrokePath>());
+        new (itemOffset) StrokePath(get<StrokePath>());
         return;
     }
     case ItemType::ApplyDeviceScaleFactor: {
-        new (destination.data + sizeof(ItemType)) ApplyDeviceScaleFactor(get<ApplyDeviceScaleFactor>());
+        new (itemOffset) ApplyDeviceScaleFactor(get<ApplyDeviceScaleFactor>());
         return;
     }
 #if USE(CG)
     case ItemType::ApplyFillPattern: {
-        new (destination.data + sizeof(ItemType)) ApplyFillPattern(get<ApplyFillPattern>());
+        new (itemOffset) ApplyFillPattern(get<ApplyFillPattern>());
         return;
     }
     case ItemType::ApplyStrokePattern: {
-        new (destination.data + sizeof(ItemType)) ApplyStrokePattern(get<ApplyStrokePattern>());
+        new (itemOffset) ApplyStrokePattern(get<ApplyStrokePattern>());
         return;
     }
 #endif
     case ItemType::BeginTransparencyLayer: {
-        new (destination.data + sizeof(ItemType)) BeginTransparencyLayer(get<BeginTransparencyLayer>());
+        new (itemOffset) BeginTransparencyLayer(get<BeginTransparencyLayer>());
         return;
     }
     case ItemType::ClearRect: {
-        new (destination.data + sizeof(ItemType)) ClearRect(get<ClearRect>());
+        new (itemOffset) ClearRect(get<ClearRect>());
         return;
     }
     case ItemType::ClearShadow: {
-        new (destination.data + sizeof(ItemType)) ClearShadow(get<ClearShadow>());
+        new (itemOffset) ClearShadow(get<ClearShadow>());
         return;
     }
     case ItemType::Clip: {
-        new (destination.data + sizeof(ItemType)) Clip(get<Clip>());
+        new (itemOffset) Clip(get<Clip>());
         return;
     }
     case ItemType::ClipOut: {
-        new (destination.data + sizeof(ItemType)) ClipOut(get<ClipOut>());
+        new (itemOffset) ClipOut(get<ClipOut>());
         return;
     }
     case ItemType::ConcatenateCTM: {
-        new (destination.data + sizeof(ItemType)) ConcatenateCTM(get<ConcatenateCTM>());
+        new (itemOffset) ConcatenateCTM(get<ConcatenateCTM>());
         return;
     }
     case ItemType::DrawDotsForDocumentMarker: {
-        new (destination.data + sizeof(ItemType)) DrawDotsForDocumentMarker(get<DrawDotsForDocumentMarker>());
+        new (itemOffset) DrawDotsForDocumentMarker(get<DrawDotsForDocumentMarker>());
         return;
     }
     case ItemType::DrawEllipse: {
-        new (destination.data + sizeof(ItemType)) DrawEllipse(get<DrawEllipse>());
+        new (itemOffset) DrawEllipse(get<DrawEllipse>());
         return;
     }
     case ItemType::DrawLine: {
-        new (destination.data + sizeof(ItemType)) DrawLine(get<DrawLine>());
+        new (itemOffset) DrawLine(get<DrawLine>());
         return;
     }
     case ItemType::DrawRect: {
-        new (destination.data + sizeof(ItemType)) DrawRect(get<DrawRect>());
+        new (itemOffset) DrawRect(get<DrawRect>());
         return;
     }
     case ItemType::EndTransparencyLayer: {
-        new (destination.data + sizeof(ItemType)) EndTransparencyLayer(get<EndTransparencyLayer>());
+        new (itemOffset) EndTransparencyLayer(get<EndTransparencyLayer>());
         return;
     }
     case ItemType::FillEllipse: {
-        new (destination.data + sizeof(ItemType)) FillEllipse(get<FillEllipse>());
+        new (itemOffset) FillEllipse(get<FillEllipse>());
         return;
     }
 #if ENABLE(INLINE_PATH_DATA)
     case ItemType::FillInlinePath: {
-        new (destination.data + sizeof(ItemType)) FillInlinePath(get<FillInlinePath>());
+        new (itemOffset) FillInlinePath(get<FillInlinePath>());
         return;
     }
 #endif
     case ItemType::FillRect: {
-        new (destination.data + sizeof(ItemType)) FillRect(get<FillRect>());
+        new (itemOffset) FillRect(get<FillRect>());
         return;
     }
     case ItemType::FlushContext: {
-        new (destination.data + sizeof(ItemType)) FlushContext(get<FlushContext>());
+        new (itemOffset) FlushContext(get<FlushContext>());
         return;
     }
     case ItemType::MetaCommandSwitchTo: {
-        new (destination.data + sizeof(ItemType)) MetaCommandSwitchTo(get<MetaCommandSwitchTo>());
+        new (itemOffset) MetaCommandSwitchTo(get<MetaCommandSwitchTo>());
         return;
     }
     case ItemType::PaintFrameForMedia: {
-        new (destination.data + sizeof(ItemType)) PaintFrameForMedia(get<PaintFrameForMedia>());
+        new (itemOffset) PaintFrameForMedia(get<PaintFrameForMedia>());
         return;
     }
     case ItemType::Restore: {
-        new (destination.data + sizeof(ItemType)) Restore(get<Restore>());
+        new (itemOffset) Restore(get<Restore>());
         return;
     }
     case ItemType::Rotate: {
-        new (destination.data + sizeof(ItemType)) Rotate(get<Rotate>());
+        new (itemOffset) Rotate(get<Rotate>());
         return;
     }
     case ItemType::Save: {
-        new (destination.data + sizeof(ItemType)) Save(get<Save>());
+        new (itemOffset) Save(get<Save>());
         return;
     }
     case ItemType::Scale: {
-        new (destination.data + sizeof(ItemType)) Scale(get<Scale>());
+        new (itemOffset) Scale(get<Scale>());
         return;
     }
     case ItemType::SetCTM: {
-        new (destination.data + sizeof(ItemType)) SetCTM(get<SetCTM>());
+        new (itemOffset) SetCTM(get<SetCTM>());
         return;
     }
     case ItemType::SetInlineFillColor: {
-        new (destination.data + sizeof(ItemType)) SetInlineFillColor(get<SetInlineFillColor>());
+        new (itemOffset) SetInlineFillColor(get<SetInlineFillColor>());
         return;
     }
     case ItemType::SetInlineFillGradient: {
-        new (destination.data + sizeof(ItemType)) SetInlineFillGradient(get<SetInlineFillGradient>());
+        new (itemOffset) SetInlineFillGradient(get<SetInlineFillGradient>());
         return;
     }
     case ItemType::SetInlineStrokeColor: {
-        new (destination.data + sizeof(ItemType)) SetInlineStrokeColor(get<SetInlineStrokeColor>());
+        new (itemOffset) SetInlineStrokeColor(get<SetInlineStrokeColor>());
         return;
     }
     case ItemType::SetLineCap: {
-        new (destination.data + sizeof(ItemType)) SetLineCap(get<SetLineCap>());
+        new (itemOffset) SetLineCap(get<SetLineCap>());
         return;
     }
     case ItemType::SetLineJoin: {
-        new (destination.data + sizeof(ItemType)) SetLineJoin(get<SetLineJoin>());
+        new (itemOffset) SetLineJoin(get<SetLineJoin>());
         return;
     }
     case ItemType::SetMiterLimit: {
-        new (destination.data + sizeof(ItemType)) SetMiterLimit(get<SetMiterLimit>());
+        new (itemOffset) SetMiterLimit(get<SetMiterLimit>());
         return;
     }
     case ItemType::SetStrokeThickness: {
-        new (destination.data + sizeof(ItemType)) SetStrokeThickness(get<SetStrokeThickness>());
+        new (itemOffset) SetStrokeThickness(get<SetStrokeThickness>());
         return;
     }
     case ItemType::StrokeEllipse: {
-        new (destination.data + sizeof(ItemType)) StrokeEllipse(get<StrokeEllipse>());
+        new (itemOffset) StrokeEllipse(get<StrokeEllipse>());
         return;
     }
 #if ENABLE(INLINE_PATH_DATA)
     case ItemType::StrokeInlinePath: {
-        new (destination.data + sizeof(ItemType)) StrokeInlinePath(get<StrokeInlinePath>());
+        new (itemOffset) StrokeInlinePath(get<StrokeInlinePath>());
         return;
     }
 #endif
     case ItemType::StrokeRect: {
-        new (destination.data + sizeof(ItemType)) StrokeRect(get<StrokeRect>());
+        new (itemOffset) StrokeRect(get<StrokeRect>());
         return;
     }
     case ItemType::StrokeLine: {
-        new (destination.data + sizeof(ItemType)) StrokeLine(get<StrokeLine>());
+        new (itemOffset) StrokeLine(get<StrokeLine>());
         return;
     }
     case ItemType::Translate: {
-        new (destination.data + sizeof(ItemType)) Translate(get<Translate>());
+        new (itemOffset) Translate(get<Translate>());
         return;
     }
     }
@@ -883,7 +884,7 @@
 
 void ItemBuffer::swapWritableBufferIfNeeded(size_t numberOfBytes)
 {
-    constexpr auto sizeForBufferSwitchItem = sizeof(ItemType) + sizeof(MetaCommandSwitchTo);
+    auto sizeForBufferSwitchItem = paddedSizeOfTypeAndItemInBytes(ItemType::MetaCommandSwitchTo);
     if (m_writtenNumberOfBytes + numberOfBytes + sizeForBufferSwitchItem <= m_writableBuffer.capacity)
         return;
 
@@ -899,16 +900,22 @@
     m_writableBuffer = WTFMove(nextBuffer);
 }
 
-void ItemBuffer::appendDataAndLength(Ref<SharedBuffer>&& data, uint8_t* location)
+void ItemBuffer::appendEncodedData(ItemHandle temporaryItem)
 {
-    reinterpret_cast<size_t*>(location)[0] = data->size();
+    auto data = ""
+    if (!data)
+        return;
 
-    auto* startOfPadding = location + sizeof(size_t);
-    size_t padding = (alignof(uint64_t) - reinterpret_cast<uintptr_t>(startOfPadding) % alignof(uint64_t)) % alignof(uint64_t);
-    auto* startOfData = startOfPadding + padding;
-    memcpy(startOfData, data->dataAsUInt8Ptr(), data->size());
+    auto dataLength = data->size();
+    auto additionalCapacityForEncodedItem = 2 * sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), dataLength);
 
-    m_writtenNumberOfBytes += sizeof(ItemType) + sizeof(size_t) + padding + data->size();
+    swapWritableBufferIfNeeded(additionalCapacityForEncodedItem);
+
+    m_writableBuffer.data[m_writtenNumberOfBytes] = static_cast<uint8_t>(temporaryItem.type());
+    reinterpret_cast<uint64_t*>(m_writableBuffer.data + m_writtenNumberOfBytes)[1] = dataLength;
+    memcpy(m_writableBuffer.data + m_writtenNumberOfBytes + 2 * sizeof(uint64_t), data->dataAsUInt8Ptr(), dataLength);
+
+    m_writtenNumberOfBytes += additionalCapacityForEncodedItem;
 }
 
 } // namespace DisplayList

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.h (269639 => 269640)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.h	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.h	2020-11-10 20:30:46 UTC (rev 269640)
@@ -53,12 +53,15 @@
 
 using ItemBufferHandles = Vector<ItemBufferHandle, 2>;
 
-// An ItemHandle wraps a pointer to an ItemType, followed immediately by an item of that type.
+// An ItemHandle wraps a pointer to an ItemType followed immediately by an item of that type.
+// Each item handle data pointer is aligned to 8 bytes, and the item itself is also aligned
+// to 8 bytes. To ensure this, the item type consists of 8 bytes (1 byte for the type and 7
+// bytes of padding).
 struct ItemHandle {
     uint8_t* data { nullptr };
 
     void apply(GraphicsContext&);
-    void destroy();
+    WEBCORE_EXPORT void destroy();
     bool isDrawingItem() const { return WebCore::DisplayList::isDrawingItem(type()); }
 
     operator bool() const { return !!(*this); }
@@ -69,13 +72,12 @@
 #endif
 
     ItemType type() const { return static_cast<ItemType>(data[0]); }
-    size_t size() const { return sizeof(ItemType) + sizeOfItemInBytes(type()); }
 
     template<typename T> bool is() const { return type() == T::itemType; }
     template<typename T> T& get() const
     {
         ASSERT(is<T>());
-        return *reinterpret_cast<T*>(&data[sizeof(ItemType)]);
+        return *reinterpret_cast<T*>(&data[sizeof(uint64_t)]);
     }
 
     void copyTo(ItemHandle destination) const;
@@ -134,32 +136,28 @@
     // writable buffer handle; if remaining buffer capacity is insufficient to store the item, a
     // new buffer will be allocated (either by the ItemBufferWritingClient, if set, or by the item
     // buffer itself if there is no client). Items are placed back-to-back in these item buffers,
-    // with an ItemType separating each item.
+    // with padding after each item to ensure that all items are aligned to 8 bytes.
     //
     // If a writing client is present and requires custom encoding for the given item type T, the
     // item buffer will ask the client for an opaque SharedBuffer containing encoded data for the
     // item. This encoded data is then appended to the item buffer, with padding to ensure that
-    // the start of this data is aligned to 8 bytes, if necessary. When consuming encoded item
-    // data, a corresponding ItemBufferReadingClient will be required to convert this encoded data
-    // back into an item of type T.
+    // the start and end of this data are aligned to 8 bytes, if necessary. When consuming encoded
+    // item data, a corresponding ItemBufferReadingClient will be required to convert this encoded
+    // data back into an item of type T.
     template<typename T, class... Args> void append(Args&&... args)
     {
         static_assert(std::is_trivially_destructible<T>::value || !T::isInlineItem);
-        constexpr auto sizeOfTypeAndItem = sizeof(ItemType) + sizeof(T);
 
         if (!T::isInlineItem && m_writingClient) {
-            static uint8_t itemBuffer[sizeOfTypeAndItem];
-            itemBuffer[0] = static_cast<uint8_t>(T::itemType);
-            new (itemBuffer + sizeof(ItemType)) T(std::forward<Args>(args)...);
-            if (auto sharedBuffer = m_writingClient->encodeItem({ itemBuffer })) {
-                swapWritableBufferIfNeeded(sharedBuffer->size() + sizeof(ItemType) + sizeof(size_t) + alignof(uint64_t));
-                m_writableBuffer.data[m_writtenNumberOfBytes] = static_cast<uint8_t>(T::itemType);
-                appendDataAndLength(sharedBuffer.releaseNonNull(), m_writableBuffer.data + m_writtenNumberOfBytes + sizeof(ItemType));
-            }
+            static uint8_t temporaryItemBuffer[sizeof(uint64_t) + sizeof(T)];
+            temporaryItemBuffer[0] = static_cast<uint8_t>(T::itemType);
+            new (temporaryItemBuffer + sizeof(uint64_t)) T(std::forward<Args>(args)...);
+            appendEncodedData({ temporaryItemBuffer });
+            ItemHandle { temporaryItemBuffer }.destroy();
             return;
         }
 
-        swapWritableBufferIfNeeded(sizeOfTypeAndItem);
+        swapWritableBufferIfNeeded(paddedSizeOfTypeAndItemInBytes(T::itemType));
 
         if (!std::is_trivially_destructible<T>::value)
             m_itemsToDestroyInAllocatedBuffers.append({ &m_writableBuffer.data[m_writtenNumberOfBytes] });
@@ -175,13 +173,13 @@
     void forEachItemBuffer(Function<void(const ItemBufferHandle&)>&&) const;
 
     WEBCORE_EXPORT void swapWritableBufferIfNeeded(size_t numberOfBytes);
-    WEBCORE_EXPORT void appendDataAndLength(Ref<SharedBuffer>&&, uint8_t*);
+    WEBCORE_EXPORT void appendEncodedData(ItemHandle);
     template<typename T, class... Args> void uncheckedAppend(Args&&... args)
     {
         auto* startOfItem = &m_writableBuffer.data[m_writtenNumberOfBytes];
         startOfItem[0] = static_cast<uint8_t>(T::itemType);
-        new (startOfItem + sizeof(ItemType)) T(std::forward<Args>(args)...);
-        m_writtenNumberOfBytes += sizeof(ItemType) + sizeOfItemInBytes(T::itemType);
+        new (startOfItem + sizeof(uint64_t)) T(std::forward<Args>(args)...);
+        m_writtenNumberOfBytes += paddedSizeOfTypeAndItemInBytes(T::itemType);
     }
 
     ItemBufferReadingClient* m_readingClient { nullptr };

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemType.cpp (269639 => 269640)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemType.cpp	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemType.cpp	2020-11-10 20:30:46 UTC (rev 269640)
@@ -31,7 +31,7 @@
 namespace WebCore {
 namespace DisplayList {
 
-size_t sizeOfItemInBytes(ItemType type)
+static size_t sizeOfItemInBytes(ItemType type)
 {
     switch (type) {
     case ItemType::Save:
@@ -244,6 +244,11 @@
     return false;
 }
 
+size_t paddedSizeOfTypeAndItemInBytes(ItemType type)
+{
+    return sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), sizeOfItemInBytes(type));
+}
+
 bool isInlineItem(ItemType type)
 {
     switch (type) {

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h (269639 => 269640)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h	2020-11-10 20:30:46 UTC (rev 269640)
@@ -98,7 +98,7 @@
     ApplyDeviceScaleFactor,
 };
 
-WEBCORE_EXPORT size_t sizeOfItemInBytes(ItemType);
+WEBCORE_EXPORT size_t paddedSizeOfTypeAndItemInBytes(ItemType);
 WEBCORE_EXPORT bool isInlineItem(ItemType);
 WEBCORE_EXPORT bool isDrawingItem(ItemType);
 

Modified: trunk/Source/WebKit/ChangeLog (269639 => 269640)


--- trunk/Source/WebKit/ChangeLog	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebKit/ChangeLog	2020-11-10 20:30:46 UTC (rev 269640)
@@ -1,3 +1,17 @@
+2020-11-10  Wenson Hsieh  <wenson_hs...@apple.com>
+
+        REGRESSION (r269525): Many layout tests crash when run under ASan
+        https://bugs.webkit.org/show_bug.cgi?id=218733
+        <rdar://problem/71206273>
+
+        Reviewed by Tim Horton.
+
+        Construct the item 8 bytes after the handle offset, instead of 1 byte (also, add a FIXME indicating that we some
+        way of achieving this without requiring WebKit2 to be aware of an item handle's memory layout).
+
+        * GPUProcess/graphics/RemoteRenderingBackend.h:
+        (WebKit::RemoteRenderingBackend::decodeAndCreate):
+
 2020-11-10  Chris Dumez  <cdu...@apple.com>
 
         [GPUProcess] Regression(r268632) Garbage is rendered on speakers when using WebAudio

Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h (269639 => 269640)


--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h	2020-11-10 20:30:46 UTC (rev 269640)
@@ -84,7 +84,9 @@
     Optional<WebCore::DisplayList::ItemHandle> WARN_UNUSED_RETURN decodeAndCreate(const uint8_t* data, size_t length, uint8_t* handleLocation)
     {
         if (auto item = IPC::Decoder::decodeSingleObject<T>(data, length)) {
-            new (handleLocation + sizeof(WebCore::DisplayList::ItemType)) T(WTFMove(*item));
+            // FIXME: WebKit2 should not need to know that the first 8 bytes at the handle are reserved for the type.
+            // Need to figure out a way to keep this knowledge within display list code in WebCore.
+            new (handleLocation + sizeof(uint64_t)) T(WTFMove(*item));
             return {{ handleLocation }};
         }
         return WTF::nullopt;

Modified: trunk/Tools/ChangeLog (269639 => 269640)


--- trunk/Tools/ChangeLog	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Tools/ChangeLog	2020-11-10 20:30:46 UTC (rev 269640)
@@ -1,3 +1,14 @@
+2020-11-10  Wenson Hsieh  <wenson_hs...@apple.com>
+
+        REGRESSION (r269525): Many layout tests crash when run under ASan
+        https://bugs.webkit.org/show_bug.cgi?id=218733
+        <rdar://problem/71206273>
+
+        Reviewed by Tim Horton.
+
+        * TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:
+        (TestWebKitAPI::TEST):
+
 2020-11-10  Andres Gonzalez  <andresg...@apple.com>
 
         Fix for LayoutTests/accessibility/mac/search-predicate.html in isolated tree mode.

Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp (269639 => 269640)


--- trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp	2020-11-10 19:40:50 UTC (rev 269639)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp	2020-11-10 20:30:46 UTC (rev 269640)
@@ -177,7 +177,7 @@
         {
             EXPECT_EQ(type, ItemType::StrokePath);
             EXPECT_EQ(dataLength, sizeof(size_t));
-            new (handleLocation + sizeof(ItemType)) StrokePath(m_items[*reinterpret_cast<const size_t*>(data)]);
+            new (handleLocation + sizeof(uint64_t)) StrokePath(m_items[*reinterpret_cast<const size_t*>(data)]);
             return {{ handleLocation }};
         }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to