Changeset 271757 in webkit


Ignore:
Timestamp:
Jan 22, 2021 12:40:21 PM (3 years ago)
Author:
Wenson Hsieh
Message:

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.

Source/WebCore:

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).

Tools:

  • 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.

Location:
trunk
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r271751 r271757  
     12021-01-21  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        DisplayList::Replayer should stop replay and inform clients after encountering an invalid item
     4        https://bugs.webkit.org/show_bug.cgi?id=220867
     5
     6        Reviewed by Chris Dumez.
     7
     8        Make the `DisplayList` item iterator emit `Optional<ItemHandle>` instead of just `ItemHandle`; in the case of
     9        client decoding or item validation failures (see #220710), this item handle will be `WTF::nullopt`. The display
     10        list replayer will then handle this by halting replay with `StopReplayReason::InvalidItem`.
     11
     12        This refactoring will eventually enable `RemoteRenderingBackend` (in the GPU process) to terminate the web
     13        content process when we fail to decode a display list item during playback (or encounter an invalid inline
     14        item).
     15
     16        Test:   DisplayListTests.InlineItemValidationFailure
     17                DisplayListTests.OutOfLineItemDecodingFailure
     18
     19        * platform/graphics/displaylists/DisplayList.cpp:
     20        (WebCore::DisplayList::DisplayList::asText const):
     21        (WebCore::DisplayList::DisplayList::dump const):
     22        (WebCore::DisplayList::DisplayList::iterator::atEnd const):
     23        (WebCore::DisplayList::DisplayList::iterator::updateCurrentItem):
     24
     25        Add an `m_isValid` flag. This flag is set to `true` initially, and is only set to `false` when we encounter an
     26        item that is either invalid or unable to be decoded. Additionally, remove release assertions and the FIXMEs
     27        regarding handling item decoding failures.
     28
     29        * platform/graphics/displaylists/DisplayList.h:
     30        (WebCore::DisplayList::DisplayList::iterator::operator* const):
     31
     32        If the `m_isValid` flag above is set, return `WTF::nullopt` for the item handle.
     33
     34        * platform/graphics/displaylists/DisplayListReplayer.cpp:
     35        (WebCore::DisplayList::Replayer::replay):
     36        * platform/graphics/displaylists/DisplayListReplayer.h:
     37
     38        Rename `DecodingFailure` to `InvalidItem`, since it now applies to both items that are invalid (e.g. contain
     39        identifier values of 0 when they shouldn't) as well as items that fail decoding (which, in the case of the GPU
     40        process, corresponds to IPC decoding failures).
     41
    1422021-01-22  Chris Dumez  <cdumez@apple.com>
    243

  • trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp

    r271741 r271757  
    116116    TextStream stream(TextStream::LineMode::MultipleLine, TextStream::Formatting::SVGStyleRect);
    117117    for (auto [item, extent, itemSizeInBuffer] : *this) {
    118         if (!shouldDumpForFlags(flags, item))
     118        if (!shouldDumpForFlags(flags, *item))
    119119            continue;
    120120
    121121        TextStream::GroupScope group(stream);
    122122        stream << item;
    123         if (item.isDrawingItem())
     123        if (item->isDrawingItem())
    124124            stream << " extent " << extent;
    125125    }
    … …  
    135135        TextStream::GroupScope group(ts);
    136136        ts << item;
    137         if (item.isDrawingItem())
     137        if (item->isDrawingItem())
    138138            ts << " extent " << extent;
    139139    }
    … …  
    323323bool DisplayList::iterator::atEnd() const
    324324{
    325     if (m_displayList.isEmpty())
     325    if (m_displayList.isEmpty() || !m_isValid)
    326326        return true;
    327327
    … …  
    353353        auto* startOfData = m_cursor + 2 * sizeof(uint64_t);
    354354        auto decodedItemHandle = client->decodeItem(startOfData, dataLength, itemType, m_currentBufferForItem);
    355         if (!decodedItemHandle) {
    356             // FIXME: Instead of crashing, this needs to fail gracefully and inform the caller.
    357             // For the time being, this assertion makes bugs in item buffer encoding logic easier
    358             // to catch by avoiding mysterious out-of-bounds reads and incorrectly aligned items
    359             // down the line.
    360             RELEASE_ASSERT_NOT_REACHED();
    361         }
     355        if (UNLIKELY(!decodedItemHandle))
     356            m_isValid = false;
     357
    362358        m_currentBufferForItem[0] = static_cast<uint8_t>(itemType);
    363359        m_currentItemSizeInBuffer = 2 * sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), dataLength);
    364360    } else {
    365         if (!ItemHandle { m_cursor }.safeCopy({ m_currentBufferForItem })) {
    366             // FIXME: Instead of crashing, this needs to fail gracefully and inform the caller.
    367             RELEASE_ASSERT_NOT_REACHED();
    368         }
     361        if (UNLIKELY(!ItemHandle { m_cursor }.safeCopy({ m_currentBufferForItem })))
     362            m_isValid = false;
     363
    369364        m_currentItemSizeInBuffer = paddedSizeOfTypeAndItem;
    370365    }

  • trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h

    r270796 r271757  
    119119
    120120        struct Value {
    121             ItemHandle item;
     121            Optional<ItemHandle> item;
    122122            Optional<FloatRect> extent;
    123123            size_t itemSizeInBuffer { 0 };
    … …  
    127127        {
    128128            return {
    129                 ItemHandle { m_currentBufferForItem },
     129                m_isValid ? makeOptional(ItemHandle { m_currentBufferForItem }) : WTF::nullopt,
    130130                m_currentExtent,
    131                 m_currentItemSizeInBuffer
     131                m_currentItemSizeInBuffer,
    132132            };
    133133        }
    … …  
    153153        Optional<FloatRect> m_currentExtent;
    154154        size_t m_currentItemSizeInBuffer { 0 };
     155        bool m_isValid { true };
    155156    };
    156157

  • trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp

    r270725 r271757  
    142142        }
    143143
    144         LOG_WITH_STREAM(DisplayLists, stream << "applying " << i++ << " " << item);
    145 
    146         if (item.is<MetaCommandChangeDestinationImageBuffer>()) {
    147             result.numberOfBytesRead += itemSizeInBuffer;
    148             result.reasonForStopping = StopReplayReason::ChangeDestinationImageBuffer;
    149             result.nextDestinationImageBuffer = item.get<MetaCommandChangeDestinationImageBuffer>().identifier();
     144        if (!item) {
     145            result.reasonForStopping = StopReplayReason::InvalidItem;
    150146            break;
    151147        }
    152148
    153         if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(item); reasonForStopping) {
     149        LOG_WITH_STREAM(DisplayLists, stream << "applying " << i++ << " " << item);
     150
     151        if (item->is<MetaCommandChangeDestinationImageBuffer>()) {
     152            result.numberOfBytesRead += itemSizeInBuffer;
     153            result.reasonForStopping = StopReplayReason::ChangeDestinationImageBuffer;
     154            result.nextDestinationImageBuffer = item->get<MetaCommandChangeDestinationImageBuffer>().identifier();
     155            break;
     156        }
     157
     158        if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(*item); reasonForStopping) {
    154159            result.reasonForStopping = *reasonForStopping;
    155160            result.missingCachedResourceIdentifier = WTFMove(missingCachedResourceIdentifier);
    … …  
    160165
    161166        if (UNLIKELY(trackReplayList)) {
    162             replayList->append(item);
    163             if (item.isDrawingItem())
     167            replayList->append(*item);
     168            if (item->isDrawingItem())
    164169                replayList->addDrawingItemExtent(WTFMove(extent));
    165170        }

  • trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h

    r270725 r271757  
    4343    MissingCachedResource,
    4444    ChangeDestinationImageBuffer,
    45     DecodingFailure // FIXME: Propagate decoding errors to display list replay clients through this enum as well.
     45    InvalidItem
    4646};
    4747

  • trunk/Tools/ChangeLog

    r271755 r271757  
     12021-01-21  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        DisplayList::Replayer should stop replay and inform clients after encountering an invalid item
     4        https://bugs.webkit.org/show_bug.cgi?id=220867
     5
     6        Reviewed by Chris Dumez.
     7
     8        * TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:
     9
     10        Adjust a few API tests, since the item handle in the `DisplayList` iterator is now optional.
     11
     12        * TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp:
     13
     14        Add a couple of new API tests to exercise display list item decoding and validation failures.
     15
    1162021-01-22  Kimmo Kinnunen  <kkinnunen@apple.com>
    217

  • trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp

    r270478 r271757  
    8080    bool observedUnexpectedItem = false;
    8181    for (auto [handle, extent, size] : list) {
    82         switch (handle.type()) {
     82        switch (handle->type()) {
    8383        case ItemType::SetStrokeThickness: {
    84             EXPECT_FALSE(handle.isDrawingItem());
    85             EXPECT_TRUE(handle.is<SetStrokeThickness>());
    86             auto& item = handle.get<SetStrokeThickness>();
     84            EXPECT_FALSE(handle->isDrawingItem());
     85            EXPECT_TRUE(handle->is<SetStrokeThickness>());
     86            auto& item = handle->get<SetStrokeThickness>();
    8787            EXPECT_EQ(item.thickness(), 1.5);
    8888            break;
    8989        }
    9090        case ItemType::FillPath: {
    91             EXPECT_TRUE(handle.isDrawingItem());
    92             EXPECT_TRUE(handle.is<FillPath>());
    93             auto& item = handle.get<FillPath>();
     91            EXPECT_TRUE(handle->isDrawingItem());
     92            EXPECT_TRUE(handle->is<FillPath>());
     93            auto& item = handle->get<FillPath>();
    9494            EXPECT_EQ(item.path().platformPath(), path.platformPath());
    9595            break;
    9696        }
    9797        case ItemType::FillRectWithGradient: {
    98             EXPECT_TRUE(handle.isDrawingItem());
    99             EXPECT_TRUE(handle.is<FillRectWithGradient>());
    100             auto& item = handle.get<FillRectWithGradient>();
     98            EXPECT_TRUE(handle->isDrawingItem());
     99            EXPECT_TRUE(handle->is<FillRectWithGradient>());
     100            auto& item = handle->get<FillRectWithGradient>();
    101101            EXPECT_EQ(item.rect(), FloatRect(1., 1., 10., 10.));
    102102            EXPECT_EQ(&item.gradient(), gradient.ptr());
    … …  
    104104        }
    105105        case ItemType::SetInlineFillColor: {
    106             EXPECT_FALSE(handle.isDrawingItem());
    107             EXPECT_TRUE(handle.is<SetInlineFillColor>());
    108             auto& item = handle.get<SetInlineFillColor>();
     106            EXPECT_FALSE(handle->isDrawingItem());
     107            EXPECT_TRUE(handle->is<SetInlineFillColor>());
     108            auto& item = handle->get<SetInlineFillColor>();
    109109            EXPECT_EQ(item.color(), Color::red);
    110110            break;
    … …  
    112112#if ENABLE(INLINE_PATH_DATA)
    113113        case ItemType::StrokeInlinePath: {
    114             EXPECT_TRUE(handle.isDrawingItem());
    115             EXPECT_TRUE(handle.is<StrokeInlinePath>());
    116             auto& item = handle.get<StrokeInlinePath>();
     114            EXPECT_TRUE(handle->isDrawingItem());
     115            EXPECT_TRUE(handle->is<StrokeInlinePath>());
     116            auto& item = handle->get<StrokeInlinePath>();
    117117            const auto path = item.path();
    118118            auto& line = path.inlineData<LineData>();
    … …  
    149149
    150150    for (auto [handle, extent, size] : list) {
    151         EXPECT_EQ(handle.type(), ItemType::FillRectWithColor);
    152         EXPECT_TRUE(handle.is<FillRectWithColor>());
    153 
    154         auto& item = handle.get<FillRectWithColor>();
     151        EXPECT_EQ(handle->type(), ItemType::FillRectWithColor);
     152        EXPECT_TRUE(handle->is<FillRectWithColor>());
     153
     154        auto& item = handle->get<FillRectWithColor>();
    155155        EXPECT_EQ(item.color().asInline(), Color::black);
    156156        EXPECT_EQ(item.rect(), FloatRect(0, 0, 100, 100));
    … …  
    227227    Vector<ItemType> itemTypes;
    228228    for (auto [handle, extent, size] : shallowCopy)
    229         itemTypes.append(handle.type());
     229        itemTypes.append(handle->type());
    230230
    231231    EXPECT_FALSE(shallowCopy.isEmpty());

  • trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp

    r270556 r271757  
    3838using namespace DisplayList;
    3939
     40constexpr size_t globalItemBufferCapacity = 1 << 12;
     41static uint8_t globalItemBuffer[globalItemBufferCapacity];
     42constexpr CGFloat contextWidth = 100;
     43constexpr CGFloat contextHeight = 100;
     44
    4045TEST(DisplayListTests, ReplayWithMissingResource)
    4146{
    42     constexpr CGFloat contextWidth = 100;
    43     constexpr CGFloat contextHeight = 100;
    44 
    4547    FloatRect contextBounds { 0, 0, contextWidth, contextHeight };
    4648
    … …  
    8082}
    8183
     84TEST(DisplayListTests, OutOfLineItemDecodingFailure)
     85{
     86    static ItemBufferIdentifier globalBufferIdentifier = ItemBufferIdentifier::generate();
     87
     88    class ReadingClient : public ItemBufferReadingClient {
     89    private:
     90        Optional<ItemHandle> WARN_UNUSED_RETURN decodeItem(const uint8_t*, size_t, ItemType type, uint8_t*) final
     91        {
     92            EXPECT_EQ(type, ItemType::FillPath);
     93            return WTF::nullopt;
     94        }
     95    };
     96
     97    class WritingClient : public ItemBufferWritingClient {
     98    private:
     99        ItemBufferHandle createItemBuffer(size_t capacity) final
     100        {
     101            EXPECT_LT(capacity, globalItemBufferCapacity);
     102            return { globalBufferIdentifier, globalItemBuffer, globalItemBufferCapacity };
     103        }
     104
     105        RefPtr<SharedBuffer> encodeItem(ItemHandle) const final { return SharedBuffer::create(); }
     106        void didAppendData(const ItemBufferHandle&, size_t, DidChangeItemBuffer) final { }
     107    };
     108
     109    FloatRect contextBounds { 0, 0, contextWidth, contextHeight };
     110
     111    auto colorSpace = adoptCF(CGColorSpaceCreateWithName(kCGColorSpaceSRGB));
     112    auto cgContext = adoptCF(CGBitmapContextCreate(nullptr, contextWidth, contextHeight, 8, 4 * contextWidth, colorSpace.get(), kCGImageAlphaPremultipliedLast));
     113    GraphicsContext context { cgContext.get() };
     114
     115    DisplayList list;
     116    WritingClient writer;
     117    list.setItemBufferClient(&writer);
     118
     119    Path path;
     120    path.moveTo({ 10., 10. });
     121    path.addLineTo({ 50., 50. });
     122    path.addLineTo({ 10., 10. });
     123    list.append<SetInlineStrokeColor>(Color::blue);
     124    list.append<FillPath>(WTFMove(path));
     125
     126    DisplayList shallowCopy {{ ItemBufferHandle { globalBufferIdentifier, globalItemBuffer, list.sizeInBytes() } }};
     127    ReadingClient reader;
     128    shallowCopy.setItemBufferClient(&reader);
     129
     130    Replayer replayer { context, list };
     131    auto result = replayer.replay();
     132    EXPECT_GT(result.numberOfBytesRead, 0U);
     133    EXPECT_EQ(result.nextDestinationImageBuffer, WTF::nullopt);
     134    EXPECT_EQ(result.missingCachedResourceIdentifier, WTF::nullopt);
     135    EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItem);
     136}
     137
     138TEST(DisplayListTests, InlineItemValidationFailure)
     139{
     140    FloatRect contextBounds { 0, 0, contextWidth, contextHeight };
     141
     142    auto colorSpace = adoptCF(CGColorSpaceCreateWithName(kCGColorSpaceSRGB));
     143    auto cgContext = adoptCF(CGBitmapContextCreate(nullptr, contextWidth, contextHeight, 8, 4 * contextWidth, colorSpace.get(), kCGImageAlphaPremultipliedLast));
     144    GraphicsContext context { cgContext.get() };
     145
     146    DisplayList list;
     147    list.append<FlushContext>(FlushIdentifier { });
     148
     149    Replayer replayer { context, list };
     150    auto result = replayer.replay();
     151    EXPECT_EQ(result.numberOfBytesRead, 0U);
     152    EXPECT_EQ(result.nextDestinationImageBuffer, WTF::nullopt);
     153    EXPECT_EQ(result.missingCachedResourceIdentifier, WTF::nullopt);
     154    EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItem);
     155}
     156
    82157} // namespace TestWebKitAPI
    83158
Note: See TracChangeset for help on using the changeset viewer.