Bug 206705 - Crash in RenderElement::selectionPseudoStyle with detail element set to display: contents
Summary: Crash in RenderElement::selectionPseudoStyle with detail element set to displ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Mac macOS 10.15
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-23 15:16 PST by Doug Kelly
Modified: 2020-01-30 12:59 PST (History)
12 users (show)

See Also:


Attachments
Patch (4.06 KB, patch)
2020-01-23 15:19 PST, Doug Kelly
no flags Details | Formatted Diff | Diff
Patch (4.23 KB, patch)
2020-01-27 13:09 PST, Doug Kelly
no flags Details | Formatted Diff | Diff
Patch (4.22 KB, patch)
2020-01-27 13:37 PST, Doug Kelly
no flags Details | Formatted Diff | Diff
Patch (4.22 KB, patch)
2020-01-30 10:04 PST, Doug Kelly
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Doug Kelly 2020-01-23 15:16:25 PST
When setting a detail element to display: contents and selecting the text, the result is a crash in RenderElement::selectionPseudoStyle.

<rdar://problem/54544471>
Comment 1 Doug Kelly 2020-01-23 15:19:37 PST
Created attachment 388598 [details]
Patch
Comment 2 Darin Adler 2020-01-26 21:28:16 PST
Comment on attachment 388598 [details]
Patch

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

r=me but I have to admit the logic of the change is not obvious to me

> Source/WebCore/rendering/RenderElement.cpp:1419
> +            while (currentElement && currentElement->hasDisplayContents())
> +                currentElement = currentElement->parentElement();

It’s not obvious *why* this is the right thing to do. So we need a comment. Why are we skipping over any element that "has display contents"?
Comment 3 Doug Kelly 2020-01-27 10:02:51 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 388598 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388598&action=review
> 
> r=me but I have to admit the logic of the change is not obvious to me
> 
> > Source/WebCore/rendering/RenderElement.cpp:1419
> > +            while (currentElement && currentElement->hasDisplayContents())
> > +                currentElement = currentElement->parentElement();
> 
> It’s not obvious *why* this is the right thing to do. So we need a comment.
> Why are we skipping over any element that "has display contents"?

Ryosuke?  You were the one that mentioned that we needed to find the first parent without "display: contents" set -- but it ultimately has something to do with the next line, where we need a valid renderer.  Is there something special about "display: contents" that would need this?
Comment 4 Doug Kelly 2020-01-27 13:09:53 PST
Created attachment 388888 [details]
Patch
Comment 5 Doug Kelly 2020-01-27 13:12:18 PST
I managed to speak with Ryosuke, and he confirmed that if an element is set to "display: contents" it will not have a renderer.  Hopefully the improved comment clarifies this; I must admit that I'm still not terribly familiar with its behaviors.
Comment 6 Ryosuke Niwa 2020-01-27 13:29:52 PST
Comment on attachment 388888 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1419
> +            // If an Element hasDisplayContents(), it will not have a renderer.
> +            // Instead, we must walk up the tree to find a parent element with a valid renderer.

This comment more or less repeats what the code says.
I'd try giving more of a why comment like this:
// When an element has display: contents, this element doesn't have a renderer
// and its children will render as children of the parent element.

There is no need to say that we're looking for the first element without display: contents
since that's pretty self explanatory from the code.
Comment 7 Doug Kelly 2020-01-27 13:36:15 PST
(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 388888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388888&action=review
> 
> > Source/WebCore/rendering/RenderElement.cpp:1419
> > +            // If an Element hasDisplayContents(), it will not have a renderer.
> > +            // Instead, we must walk up the tree to find a parent element with a valid renderer.
> 
> This comment more or less repeats what the code says.
> I'd try giving more of a why comment like this:
> // When an element has display: contents, this element doesn't have a
> renderer
> // and its children will render as children of the parent element.
> 
> There is no need to say that we're looking for the first element without
> display: contents
> since that's pretty self explanatory from the code.

Agreed; I was trying to think of the best way to word it without adding anything which might be misleading.  This sounds much better. :)
Comment 8 Doug Kelly 2020-01-27 13:37:24 PST
Created attachment 388897 [details]
Patch
Comment 9 zalan 2020-01-30 09:31:01 PST
Comment on attachment 388897 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1417
> +            Element* currentElement = element()->shadowHost();

auto* please
Comment 10 Doug Kelly 2020-01-30 10:04:47 PST
Created attachment 389262 [details]
Patch
Comment 11 WebKit Commit Bot 2020-01-30 11:26:00 PST
The commit-queue encountered the following flaky tests while processing attachment 389262 [details]:

The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2020-01-30 11:26:27 PST
The commit-queue encountered the following flaky tests while processing attachment 389262 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2020-01-30 12:59:56 PST
Comment on attachment 389262 [details]
Patch

Clearing flags on attachment: 389262

Committed r255448: <https://trac.webkit.org/changeset/255448>
Comment 14 WebKit Commit Bot 2020-01-30 12:59:58 PST
All reviewed patches have been landed.  Closing bug.