REGRESSION (r84225): Virtual function called from Node::parentNode()

[Antti Koivisto]

http://trac.webkit.org/changeset/84225 from bug 52788 added a virtual function call (isSVGShadowRoot()) to Node::parentNode():

 inline ContainerNode* Node::parentNode() const
 {
-    return getFlag(IsShadowRootFlag) ? 0 : parent();
+    return getFlag(IsShadowRootFlag) || isSVGShadowRoot() ? 0 : parent();
 }

This regresses everything that depends on walking up the tree, most crucially CSS selector matching.

[Antti Koivisto]

The fact that the call is carefully inlined should be a hint that you are not supposed to put a virtual call here.

[Dominic Cooney]

OK. Does SVGShadowRoot warrant its own flag to avoid this?

[Antti Koivisto]

Why does it need a separate test? It makes no logical sense that a "SVG shadow root" is not a "shadow root".

[Antti Koivisto]

We are running out of bits in Node so preferably not.

[Dominic Cooney]

Originally there was an attempt to merge ShadowRoot and SVGShadowRoot, but it turned out to be difficult—dglazkov should be able to provide context. I attempted to separate them which is what motivated this change.

[Dominic Cooney]

To add some commentary on what was difficult about merging ShadowRoot and SVGShadowRoot—IIRC:

- Rendering is different.
- SVGShadowRoot tries to behave like <svg:g>, which is not helpful when ShadowRoot appears in a HTML tree.
- Event dispatch is different.
- There’s a handful of special behavior for SVGShadowRoot, eg can’t have <script> descendants.
- There’s a handful of special behavior for ShadowRoot around editing.

CC’d Krit in case he is working on any refactoring around SVGShadowRoot.

[Roland Steiner]

I think that sums it up pretty nicely. One other approach we were discussing is to set the parent pointer to NULL, the host node holding an explicit ref on the (SVG) shadow root instead, and having a separate host element pointer on the (SVG) shadow root.

However, until we add ShadowRoot to SVG, this merely pushes the virtual function call from parentNode() to parentOrHostNode().

[Dominic Cooney]

Maybe right now we can move back to using the 'shadow root' flag for ShadowRoot and SVGShadowRoot. This will make parentNode() fast. Some editing code may slow down, although maybe we can make isShadowRoot do

getFlag(ShadowRoot) && !n->isSVGShadowRoot()

which is still fast in <input>, <textarea>, etc. when the result is false.

[Dominic Cooney]

Another awkward difference between ShadowRoot and SVGShadowRoot is ShadowRoot sets and clears the flag when it goes into and out of the tree, but isSVGShadowRoot is constant.

A big hacky, but parentNode can be trivially sped up in the non-SVG case with something like:

return getFlag(IsShadowRootFlag) || (isSVGElement() && isSVGShadowRoot()) ? 0 : parent();

which predicates the virtual call on another flag test.

For now I am going to look at using these pairs of flags:

IsShadowRootFlag, not IsSVGElementFlag => ShadowRoot
IsShadowRootFlag, IsSVGElementFlag => SVGShadowRoot

I will rename the schizoflag to IsShadowRootOrSVGShadowRootFlag.

[Dominic Cooney]

Created attachment 109801 [details]
Patch

Depends on patch on 69406.

[Roland Steiner]

(In reply to comment #10)

Looks good! :)

[WebKit Review Bot]

Comment on attachment 109801 [details]
Patch

Clearing flags on attachment: 109801

Committed r96803: <http://trac.webkit.org/changeset/96803>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.