Bug 119886

Summary: PseudoElement is abusing parent node pointer
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, eoconnor, esprehn+autocc, glenn, kangil.han, kling, kondapallykalyan, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
another
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
another darin: review+

Description Antti Koivisto 2013-08-16 02:54:40 PDT
PseudoElement sets its host node as its parent. This is confusing and wrong as it breaks the basic tree consistency (a node is a child node of its parent node).
Comment 1 Antti Koivisto 2013-08-16 03:17:59 PDT
Created attachment 208902 [details]
patch
Comment 2 Build Bot 2013-08-16 04:35:57 PDT
Comment on attachment 208902 [details]
patch

Attachment 208902 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1478101

New failing tests:
css2.1/t1202-counter-08-b.html
css2.1/t1202-counters-14-b.html
accessibility/render-counter-text.html
css2.1/t1202-counters-06-b.html
css2.1/t1202-counters-16-c.html
css2.1/t1202-counter-07-b.html
css2.1/t1202-counters-12-b.html
css2.1/t1202-counter-15-b.html
css2.1/t1202-counters-09-b.html
css2.1/t1202-counters-11-b.html
css2.1/t1202-counters-13-b.html
css2.1/t1202-counter-16-f.html
css2.1/t1202-counters-04-b.html
css2.1/t1202-counters-03-b.html
css2.1/t1202-counter-12-b.html
css2.1/t1202-counters-07-b.html
css2.1/t1202-counters-15-b.html
css2.1/t1202-counter-09-b.html
css2.1/t1202-counters-08-b.html
css2.1/t1202-counter-14-b.html
css2.1/t1202-counter-11-b.html
css2.1/t1202-counters-01-b.html
css2.1/t1202-counter-13-b.html
css2.1/t1202-counters-17-d.html
css2.1/t1202-counter-05-b.html
css2.1/t1202-counter-06-b.html
css2.1/t1202-counters-02-b.html
css2.1/t1202-counters-00-b.html
css2.1/t1202-counter-00-b.html
css2.1/t1202-counters-05-b.html
Comment 3 Build Bot 2013-08-16 04:35:59 PDT
Created attachment 208911 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 4 Ryosuke Niwa 2013-08-16 08:46:39 PDT
Is this consistent with http://dev.w3.org/csswg/cssom/#pseudoelement ?
Comment 5 Antti Koivisto 2013-08-16 08:49:57 PDT
Yeah. That interface does not even make PseudoElement an Element.
Comment 6 Antti Koivisto 2013-08-16 08:51:01 PDT
Created attachment 208927 [details]
another
Comment 7 Antti Koivisto 2013-08-16 08:51:49 PDT
Now including fixes to the (messy) traversal functions used for resolving CSS counters.
Comment 8 Build Bot 2013-08-16 11:09:18 PDT
Comment on attachment 208927 [details]
another

Attachment 208927 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1479095

New failing tests:
fast/lists/anonymous-items.html
Comment 9 Build Bot 2013-08-16 11:09:21 PDT
Created attachment 208933 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 10 Build Bot 2013-08-16 12:50:33 PDT
Comment on attachment 208927 [details]
another

Attachment 208927 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1467646

New failing tests:
fast/lists/anonymous-items.html
Comment 11 Build Bot 2013-08-16 12:50:36 PDT
Created attachment 208947 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 12 Antti Koivisto 2013-08-17 05:13:28 PDT
Created attachment 208986 [details]
another
Comment 13 Darin Adler 2013-08-17 05:22:41 PDT
Comment on attachment 208986 [details]
another

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

The part of this patch I don’t understand is which calls to parentNode you decided to change to add checks for pseudo elements and shadow roots. I don’t understand why you had to do this in some places but not others and how you are sure you covered everything. The RenderCounter and RenderListItem cases are the puzzling ones. I don’t see how the code worked before. It was just calling parentNode like tons of other places that call parentNode. How did you know that these were the ones that needed to be changed?

> Source/WebCore/rendering/RenderCounter.cpp:72
> +    if (object->node()->isPseudoElement())
> +        return toPseudoElement(object->node())->hostElement();
> +    return toElement(object->node())->parentElement();

I would have written this with a local variable, but I guess we can count on the compiler to get this right?

> Source/WebCore/rendering/RenderListItem.cpp:108
> +    Node* parent = listItemNode->isPseudoElement() ? toPseudoElement(listItemNode)->hostElement() : listItemNode->parentNode();
>      // We use parentNode because the enclosing list could be a ShadowRoot that's not Element.
> -    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
> +    for (; parent; parent = parent->parentNode()) {

Why does the first step of walking up the parent tree need to traverse host element pointers, but not any other steps? Can’t there be a transition to a host later on? Is this covered by tests?
Comment 14 Antti Koivisto 2013-08-17 07:02:14 PDT
(In reply to comment #13)
> (From update of attachment 208986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208986&action=review
> 
> The part of this patch I don’t understand is which calls to parentNode you decided to change to add checks for pseudo elements and shadow roots. I don’t understand why you had to do this in some places but not others and how you are sure you covered everything. The RenderCounter and RenderListItem cases are the puzzling ones. I don’t see how the code worked before. It was just calling parentNode like tons of other places that call parentNode. How did you know that these were the ones that needed to be changed?

RenderCounter and RenderListItem do traversal where PseudoElements are treated as if they were real elements to compute counter values. The code for going to siblings and down the tree was already using explicit pseudo element tests to find the right node. Traversing up did not need the tests since the parentNode of the PseudoElement was set to point to the host (the asymmetry was a result of violating the "node is its parents child" consistency). 

I changed all call sites that are part of this special type of traversal. We have pretty good test coverage here, some that I had missed were spotted by the tests.

Making PseudoElements actual Elements was probably a mistake in the first place. Benefits are not obvious.
 
> > Source/WebCore/rendering/RenderCounter.cpp:72
> > +    if (object->node()->isPseudoElement())
> > +        return toPseudoElement(object->node())->hostElement();
> > +    return toElement(object->node())->parentElement();
> 
> I would have written this with a local variable, but I guess we can count on the compiler to get this right?

Everything that is being called is inlined so I think so. But yes, it would look nicer with variables.

> > Source/WebCore/rendering/RenderListItem.cpp:108
> > +    Node* parent = listItemNode->isPseudoElement() ? toPseudoElement(listItemNode)->hostElement() : listItemNode->parentNode();
> >      // We use parentNode because the enclosing list could be a ShadowRoot that's not Element.
> > -    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
> > +    for (; parent; parent = parent->parentNode()) {
> 
> Why does the first step of walking up the parent tree need to traverse host element pointers, but not any other steps? Can’t there be a transition to a host later on? Is this covered by tests?

PseudoElements can only ever be leaf nodes (for purpose of this traversal case, they are really not in the tree at all). This sort of tightening of logic is an useful side-effect.
Comment 15 Antti Koivisto 2013-08-17 08:42:41 PDT
http://trac.webkit.org/changeset/154232
Comment 16 Alexey Proskuryakov 2013-08-19 14:50:09 PDT
This caused bug 119969.