Bug 224019 - List of extents should be bounds-checked when iterating display list items
Summary: List of extents should be bounds-checked when iterating display list items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-31 14:22 PDT by Wenson Hsieh
Modified: 2021-03-31 18:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.92 KB, patch)
2021-03-31 15:17 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-03-31 14:22:16 PDT
<rdar://problem/71851600>
Comment 1 Wenson Hsieh 2021-03-31 15:17:31 PDT
Created attachment 424828 [details]
Patch
Comment 2 Tim Horton 2021-03-31 15:21:55 PDT
Comment on attachment 424828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424828&action=review

> Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp:135
> -    EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItem);
> +    EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItemOrExtent);

Neither of these tests the extent validation, right? Is it possible to write a test?
Comment 3 Wenson Hsieh 2021-03-31 15:27:11 PDT
(In reply to Tim Horton from comment #2)
> Comment on attachment 424828 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424828&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp:135
> > -    EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItem);
> > +    EXPECT_EQ(result.reasonForStopping, StopReplayReason::InvalidItemOrExtent);
> 
> Neither of these tests the extent validation, right? Is it possible to write
> a test?

Thanks for the review!

As it turns out, I tried to write a test but unfortunately I don't think it's possible unless I expose `DisplayList::addDrawingItemExtent` as a public method (but that test would be a bit contrived anyways :P).

Using just the public interfaces of DisplayList, when appending a drawing item to a display list, we'll always keep the drawing item extents in sync. I also thought of perhaps enabling drawing extent tracking halfway through appending display list items, but then recalled that doing so would simply trip the release assertion that I adjusted slightly in this patch.
Comment 4 EWS 2021-03-31 18:39:41 PDT
Committed r275334: <https://commits.webkit.org/r275334>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424828 [details].